diff --git a/core/src/com/cloud/agent/api/BackupSnapshotCommand.java b/core/src/com/cloud/agent/api/BackupSnapshotCommand.java index bd9315c5a4a..0146269f2f2 100644 --- a/core/src/com/cloud/agent/api/BackupSnapshotCommand.java +++ b/core/src/com/cloud/agent/api/BackupSnapshotCommand.java @@ -28,7 +28,6 @@ public class BackupSnapshotCommand extends SnapshotCommand { private String prevSnapshotUuid; private String prevBackupUuid; private boolean isVolumeInactive; - private String firstBackupUuid; private String vmName; protected BackupSnapshotCommand() { @@ -55,19 +54,17 @@ public class BackupSnapshotCommand extends SnapshotCommand { String snapshotName, String prevSnapshotUuid, String prevBackupUuid, - String firstBackupUuid, boolean isVolumeInactive, String vmName) { super(primaryStoragePoolNameLabel, secondaryStoragePoolURL, snapshotUuid, snapshotName, dcId, accountId, volumeId); this.prevSnapshotUuid = prevSnapshotUuid; this.prevBackupUuid = prevBackupUuid; - this.firstBackupUuid = firstBackupUuid; this.isVolumeInactive = isVolumeInactive; this.vmName = vmName; setVolumePath(volumePath); } - + public String getPrevSnapshotUuid() { return prevSnapshotUuid; } @@ -75,11 +72,7 @@ public class BackupSnapshotCommand extends SnapshotCommand { public String getPrevBackupUuid() { return prevBackupUuid; } - - public String getFirstBackupUuid() { - return firstBackupUuid; - } - + public boolean isVolumeInactive() { return isVolumeInactive; } diff --git a/core/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java b/core/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java index 11dc634a4c1..7d967dc844c 100644 --- a/core/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java +++ b/core/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java @@ -5604,7 +5604,6 @@ public abstract class CitrixResourceBase implements StoragePoolResource, ServerR Long volumeId = cmd.getVolumeId(); String secondaryStoragePoolURL = cmd.getSecondaryStoragePoolURL(); String snapshotUuid = cmd.getSnapshotUuid(); // not null: Precondition. - String prevSnapshotUuid = cmd.getPrevSnapshotUuid(); String prevBackupUuid = cmd.getPrevBackupUuid(); // By default assume failure String details = null; @@ -5647,32 +5646,16 @@ public abstract class CitrixResourceBase implements StoragePoolResource, ServerR } } else { snapshotBackupUuid = backupSnapshot(primaryStorageSRUuid, dcId, accountId, volumeId, secondaryStorageMountPath, - snapshotUuid, prevSnapshotUuid, prevBackupUuid, isISCSI); + snapshotUuid, prevBackupUuid, isISCSI); success = (snapshotBackupUuid != null); } if (success) { details = "Successfully backedUp the snapshotUuid: " + snapshotUuid + " to secondary storage."; - // Mark the snapshot as removed in the database. - // When the next snapshot is taken, it will be - // 1) deleted from the DB 2) The snapshotUuid will be deleted from the primary - // 3) the snapshotBackupUuid will be copied to secondary - // 4) if possible it will be coalesced with the next snapshot. + String volumeUuid = cmd.getVolumePath(); + destroySnapshotOnPrimaryStorageExceptThis(volumeUuid, snapshotUuid); - if (prevSnapshotUuid != null) { - // Destroy the previous snapshot, if it exists. - // We destroy the previous snapshot only if the current snapshot - // backup succeeds. - // The aim is to keep the VDI of the last 'successful' snapshot - // so that it doesn't get merged with the - // new one - // and muddle the vhd chain on the secondary storage. - - String volumeUuid = cmd.getVolumePath(); - destroySnapshotOnPrimaryStorageExceptThis(volumeUuid, snapshotUuid); - - } } } catch (XenAPIException e) { @@ -6028,21 +6011,18 @@ public abstract class CitrixResourceBase implements StoragePoolResource, ServerR // Each argument is put in a separate line for readability. // Using more lines does not harm the environment. protected String backupSnapshot(String primaryStorageSRUuid, Long dcId, Long accountId, Long volumeId, String secondaryStorageMountPath, - String snapshotUuid, String prevSnapshotUuid, String prevBackupUuid, Boolean isISCSI) { + String snapshotUuid, String prevBackupUuid, Boolean isISCSI) { String backupSnapshotUuid = null; - if (prevSnapshotUuid == null) { - prevSnapshotUuid = ""; - } if (prevBackupUuid == null) { prevBackupUuid = ""; } // Each argument is put in a separate line for readability. // Using more lines does not harm the environment. - String results = callHostPluginWithTimeOut("vmopsSnapshot", "backupSnapshot", 110*60, "primaryStorageSRUuid", primaryStorageSRUuid, "dcId", dcId.toString(), "accountId", accountId.toString(), "volumeId", - volumeId.toString(), "secondaryStorageMountPath", secondaryStorageMountPath, "snapshotUuid", snapshotUuid, "prevSnapshotUuid", prevSnapshotUuid, "prevBackupUuid", - prevBackupUuid, "isISCSI", isISCSI.toString()); + String results = callHostPluginWithTimeOut("vmopsSnapshot", "backupSnapshot", 110*60, "primaryStorageSRUuid", primaryStorageSRUuid, "dcId", + dcId.toString(), "accountId", accountId.toString(), "volumeId", volumeId.toString(), "secondaryStorageMountPath", + secondaryStorageMountPath, "snapshotUuid", snapshotUuid, "prevBackupUuid", prevBackupUuid, "isISCSI", isISCSI.toString()); if (results == null || results.isEmpty()) { // errString is already logged. diff --git a/scripts/vm/hypervisor/xenserver/vmopsSnapshot b/scripts/vm/hypervisor/xenserver/vmopsSnapshot index 802556a6028..ca2f21e8bc0 100755 --- a/scripts/vm/hypervisor/xenserver/vmopsSnapshot +++ b/scripts/vm/hypervisor/xenserver/vmopsSnapshot @@ -463,7 +463,6 @@ def backupSnapshot(session, args): volumeId = args['volumeId'] secondaryStorageMountPath = args['secondaryStorageMountPath'] snapshotUuid = args['snapshotUuid'] - prevSnapshotUuid = args['prevSnapshotUuid'] prevBackupUuid = args['prevBackupUuid'] isISCSI = getIsTrueString(args['isISCSI']) @@ -475,9 +474,6 @@ def backupSnapshot(session, args): baseCopyPath = os.path.join(primarySRPath, baseCopyVHD) util.SMlog("Base copy path: " + baseCopyPath) - prevBaseCopyUuid = '' - if prevSnapshotUuid: - prevBaseCopyUuid = getParentOfSnapshot(prevSnapshotUuid, primarySRPath, isISCSI) # Mount secondary storage mount path on XenServer along the path # /var/run/sr-mount//snapshots/ and create / dir @@ -485,13 +481,6 @@ def backupSnapshot(session, args): backupsDir = mountSnapshotsDir(secondaryStorageMountPath, "snapshots", dcId, accountId, volumeId) util.SMlog("Backups dir " + backupsDir) - if baseCopyUuid == prevBaseCopyUuid: - # There has been no change since the last snapshot so no need to backup - util.SMlog("There has been no change since the last snapshot with backup: " + prevBaseCopyUuid) - # Set the uuid of the current backup to that of last backup - txt = "1#" + prevBaseCopyUuid - return txt - # Check existence of snapshot on primary storage isfile(baseCopyPath, isISCSI) # copy baseCopyPath to backupsDir with new uuid @@ -505,9 +494,6 @@ def backupSnapshot(session, args): # Now set the availability of the snapshotPath and the baseCopyPath to false makeUnavailable(snapshotUuid, primarySRPath, isISCSI) manageAvailability(baseCopyPath, '-an') - if prevSnapshotUuid: - makeUnavailable(prevSnapshotUuid, primarySRPath, isISCSI) - makeUnavailable(prevBaseCopyUuid, primarySRPath, isISCSI) # Because the primary storage is always scanned, the parent of this base copy is always the first base copy. # We don't want that, we want a chain of VHDs each of which is a delta from the previous. diff --git a/server/src/com/cloud/api/commands/CreateSnapshotCmd.java b/server/src/com/cloud/api/commands/CreateSnapshotCmd.java index 9b018b31696..cd1e8b79a5e 100644 --- a/server/src/com/cloud/api/commands/CreateSnapshotCmd.java +++ b/server/src/com/cloud/api/commands/CreateSnapshotCmd.java @@ -23,7 +23,7 @@ import org.apache.log4j.Logger; import com.cloud.api.ApiConstants; import com.cloud.api.ApiDBUtils; import com.cloud.api.ApiResponseHelper; -import com.cloud.api.BaseAsyncCreateCmd; +import com.cloud.api.BaseAsyncCmd; import com.cloud.api.BaseCmd; import com.cloud.api.Implementation; import com.cloud.api.Parameter; @@ -35,8 +35,8 @@ import com.cloud.storage.VolumeVO; import com.cloud.storage.snapshot.SnapshotManager; import com.cloud.user.Account; -@Implementation(createMethod="createSnapshotDB", method="createSnapshot", manager=SnapshotManager.class, description="Creates an instant snapshot of a volume.") -public class CreateSnapshotCmd extends BaseAsyncCreateCmd { +@Implementation(method="createSnapshot", manager=SnapshotManager.class, description="Creates an instant snapshot of a volume.") +public class CreateSnapshotCmd extends BaseAsyncCmd { public static final Logger s_logger = Logger.getLogger(CreateSnapshotCmd.class.getName()); private static final String s_name = "createsnapshotresponse"; diff --git a/server/src/com/cloud/storage/snapshot/SnapshotManager.java b/server/src/com/cloud/storage/snapshot/SnapshotManager.java index a4427b5adea..ca4b1f81005 100755 --- a/server/src/com/cloud/storage/snapshot/SnapshotManager.java +++ b/server/src/com/cloud/storage/snapshot/SnapshotManager.java @@ -31,6 +31,7 @@ import com.cloud.exception.ResourceAllocationException; import com.cloud.storage.SnapshotPolicyVO; import com.cloud.storage.SnapshotScheduleVO; import com.cloud.storage.SnapshotVO; +import com.cloud.storage.VolumeVO; import com.cloud.storage.Storage.ImageFormat; import com.cloud.utils.component.Manager; @@ -53,13 +54,6 @@ public interface SnapshotManager extends Manager { */ SnapshotVO createSnapshotImpl(long volumeId, long policyId) throws ResourceAllocationException; - /** - * Create a snapshot of a volume - * @param cmd the API command wrapping the parameters for creating the snapshot (mainly volumeId) - * @return the Snapshot that was created - */ - SnapshotVO createSnapshotDB(CreateSnapshotCmd cmd) throws ResourceAllocationException; - /** * Create a snapshot of a volume * @param cmd the API command wrapping the parameters for creating the snapshot (mainly volumeId) @@ -181,4 +175,11 @@ public interface SnapshotManager extends Manager { SnapshotPolicyVO getPolicyForVolume(long volumeId); boolean destroySnapshotBackUp(long userId, long snapshotId, long policyId); + + /** + * Create a snapshot of a volume + * @param cmd the API command wrapping the parameters for creating the snapshot (mainly volumeId) + * @return the Snapshot that was created + */ + SnapshotVO createSnapshotOnPrimary(VolumeVO volume) throws ResourceAllocationException; } diff --git a/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java b/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java index 5dac9c7a837..899c867744c 100755 --- a/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java +++ b/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java @@ -206,11 +206,8 @@ public class SnapshotManagerImpl implements SnapshotManager { } @Override - public SnapshotVO createSnapshotDB(CreateSnapshotCmd cmd) throws ResourceAllocationException { - // FIXME: When a valid snapshot is returned, the snapshot must have been created on the storage server side so that the caller - // knows it is safe to once again resume normal operations on the volume in question. - Long volumeId = cmd.getVolumeId(); - VolumeVO volume = _volsDao.findById(volumeId); // not null, precondition. + public SnapshotVO createSnapshotOnPrimary(VolumeVO volume) throws ResourceAllocationException { + Long volumeId = volume.getId(); if (volume.getStatus() != AsyncInstanceCreateStatus.Created) { throw new InvalidParameterValueException("VolumeId: " + volumeId + " is not in Created state but " + volume.getStatus() + ". Cannot take snapshot."); } @@ -230,7 +227,6 @@ public class SnapshotManagerImpl implements SnapshotManager { throw new InvalidParameterValueException("Snapshots of volumes attached to System or router VM are not allowed"); } } - return createSnapshotImpl(volumeId, Snapshot.MANUAL_POLICY_ID); } @@ -365,57 +361,54 @@ public class SnapshotManagerImpl implements SnapshotManager { return createdSnapshot; } + - @Override - public SnapshotVO createSnapshot(CreateSnapshotCmd cmd) throws ResourceAllocationException { - SnapshotVO snapshot = _snapshotDao.findById(cmd.getId()); - Long snapshotId = null; - boolean backedUp = false; - if (snapshot != null) { - if (snapshot.getStatus() == Snapshot.Status.CreatedOnPrimary) { - snapshotId = snapshot.getId(); - backedUp = backupSnapshotToSecondaryStorage(snapshot, cmd.getStartEventId()); - if (!backedUp) { - throw new CloudRuntimeException("Created snapshot: " + snapshotId + " on primary but failed to backup on secondary"); - } + public SnapshotVO createSnapshotPublic(Long volumeId, Long policyId, Long startEventId) throws ResourceAllocationException { + VolumeVO volume = _volsDao.acquireInLockTable(volumeId, 10); + if( volume == null ) { + volume = _volsDao.findById(volumeId); + if( volume == null ){ + throw new CloudRuntimeException("Creating snapshot failed due to volume:" + volumeId + " doesn't exist"); + } else { + throw new CloudRuntimeException("Creating snapshot failed due to volume:" + volumeId + " is being used, try it later "); } - + } + SnapshotVO snapshot = null; + boolean backedUp = false; + Long snapshotId = null; + try { + snapshot = createSnapshotOnPrimary( volume); + if (snapshot != null && snapshot.getStatus() == Snapshot.Status.CreatedOnPrimary ) { + snapshotId = snapshot.getId(); + backedUp = backupSnapshotToSecondaryStorage(snapshot, startEventId); + if (!backedUp) { + throw new CloudRuntimeException("Created snapshot: " + snapshotId + " on primary but failed to backup on secondary"); + } + } + } finally { // Cleanup jobs to do after the snapshot has been created. - postCreateSnapshot(cmd.getVolumeId(), snapshotId, Snapshot.MANUAL_POLICY_ID, backedUp); + postCreateSnapshot(volumeId, snapshotId, policyId, backedUp); + _volsDao.releaseFromLockTable(volumeId); } return snapshot; } + @Override + public SnapshotVO createSnapshot(CreateSnapshotCmd cmd) throws ResourceAllocationException { + Long volumeId = cmd.getVolumeId(); + Long policyId = Snapshot.MANUAL_POLICY_ID ; + Long startEventId = cmd.getStartEventId(); + return createSnapshotPublic(volumeId, policyId, startEventId); + } + @Override public SnapshotVO createSnapshotInternal(CreateSnapshotInternalCmd cmd) throws ResourceAllocationException { Long volumeId = cmd.getVolumeId(); Long policyId = cmd.getPolicyId(); - SnapshotVO snapshot = null; - Long snapshotId = null; - try { - snapshot = createSnapshotImpl(volumeId, policyId); - } catch (InvalidParameterValueException ex) { - s_logger.warn("Received invalid parameter exception creating a recurring snapshot that should've already had validated params: " + ex.getMessage()); - } - - boolean backedUp = false; - if (snapshot != null && snapshot.getStatus() == Snapshot.Status.CreatedOnPrimary) { - snapshotId = snapshot.getId(); - backedUp = backupSnapshotToSecondaryStorage(snapshot, cmd.getStartEventId()); - if (!backedUp) { - throw new CloudRuntimeException("Created snapshot: " + snapshotId + " on primary but failed to backup on secondary"); - } - } else { - // if the snapshot wasn't created properly, just return now and avoid problems in the postCreate step - return snapshot; - } - - // Cleanup jobs to do after the snapshot has been created. - postCreateSnapshot(cmd.getVolumeId(), snapshotId, policyId, backedUp); - - return snapshot; - } + Long startEventId = cmd.getStartEventId(); + return createSnapshotPublic(volumeId, policyId, startEventId); + } private SnapshotVO updateDBOnCreate(Long id, String snapshotPath, long preSnapshotId) { SnapshotVO createdSnapshot = _snapshotDao.findById(id); @@ -487,7 +480,6 @@ public class SnapshotManagerImpl implements SnapshotManager { prevBackupUuid = prevSnapshot.getBackupSnapshotId(); } - String firstBackupUuid = volume.getFirstSnapshotBackupUuid(); boolean isVolumeInactive = _storageMgr.volumeInactive(volume); String vmName = _storageMgr.getVmNameOnVolume(volume); BackupSnapshotCommand backupSnapshotCommand = @@ -501,7 +493,6 @@ public class SnapshotManagerImpl implements SnapshotManager { snapshot.getName(), prevSnapshotUuid, prevBackupUuid, - firstBackupUuid, isVolumeInactive, vmName); @@ -737,30 +728,39 @@ public class SnapshotManagerImpl implements SnapshotManager { SnapshotVO lastSnapshot = null; _snapshotDao.remove(snapshotId); long lastId = snapshotId; + boolean destroy = false; while( true ) { lastSnapshot = _snapshotDao.findNextSnapshot(lastId); // prevsnapshotId equal 0, means it is a full snapshot - if( lastSnapshot == null || lastSnapshot.getPrevSnapshotId() == 0) + if( lastSnapshot == null ) { + break; + } + if( lastSnapshot.getPrevSnapshotId() == 0) { + // have another full snapshot, then we may delete previous delta snapshots + destroy = true; break; + } lastId = lastSnapshot.getId(); } - lastSnapshot = _snapshotDao.findByIdIncludingRemoved(lastId); - while( lastSnapshot.getRemoved() != null ) { - String BackupSnapshotId = lastSnapshot.getBackupSnapshotId(); - if( BackupSnapshotId != null ) { - if( destroySnapshotBackUp(userId, lastId, policyId) ) { + if (destroy) { + lastSnapshot = _snapshotDao.findByIdIncludingRemoved(lastId); + while (lastSnapshot.getRemoved() != null) { + String BackupSnapshotId = lastSnapshot.getBackupSnapshotId(); + if (BackupSnapshotId != null) { + if (destroySnapshotBackUp(userId, lastId, policyId)) { - } else { - s_logger.debug("Destroying snapshot backup failed " + lastSnapshot); + } else { + s_logger.debug("Destroying snapshot backup failed " + lastSnapshot); + break; + } + } + postDeleteSnapshot(userId, lastId, policyId); + lastId = lastSnapshot.getPrevSnapshotId(); + if (lastId == 0) { break; } + lastSnapshot = _snapshotDao.findById(lastId); } - postDeleteSnapshot(userId, lastId, policyId); - lastId = lastSnapshot.getPrevSnapshotId(); - if( lastId == 0 ) { - break; - } - lastSnapshot = _snapshotDao.findById(lastId); } return true; }