From cd0f5e7c2043e5c0a455b368d9d2a65122b996c3 Mon Sep 17 00:00:00 2001 From: anthony Date: Thu, 20 Jan 2011 18:11:20 -0800 Subject: [PATCH] bug 7976: if the snapshot is empty, still create a snapshot entry which has the same backupSnapshotId even though nothing is backed up to secondary storage status 7976: resolved fixed --- .../computing/LibvirtComputingResource.java | 10 +-- .../cloud/agent/api/BackupSnapshotAnswer.java | 8 ++- .../xen/resource/CitrixResourceBase.java | 37 ++++++----- .../com/cloud/storage/dao/SnapshotDao.java | 1 + .../cloud/storage/dao/SnapshotDaoImpl.java | 13 ++++ .../storage/snapshot/SnapshotManagerImpl.java | 65 +++++++++++-------- 6 files changed, 85 insertions(+), 49 deletions(-) diff --git a/agent/src/com/cloud/agent/resource/computing/LibvirtComputingResource.java b/agent/src/com/cloud/agent/resource/computing/LibvirtComputingResource.java index d9bacc7e84e..87adefa412d 100644 --- a/agent/src/com/cloud/agent/resource/computing/LibvirtComputingResource.java +++ b/agent/src/com/cloud/agent/resource/computing/LibvirtComputingResource.java @@ -1080,7 +1080,7 @@ public class LibvirtComputingResource extends ServerResourceBase implements Serv String result = command.execute(); if (result != null) { s_logger.debug("Failed to backup snaptshot: " + result); - return new BackupSnapshotAnswer(cmd, false, result, null); + return new BackupSnapshotAnswer(cmd, false, result, null, true); } /*Delete the snapshot on primary*/ @@ -1116,15 +1116,15 @@ public class LibvirtComputingResource extends ServerResourceBase implements Serv result = command.execute(); if (result != null) { s_logger.debug("Failed to backup snapshot: " + result); - return new BackupSnapshotAnswer(cmd, false, "Failed to backup snapshot: " + result, null); + return new BackupSnapshotAnswer(cmd, false, "Failed to backup snapshot: " + result, null, true); } } } catch (LibvirtException e) { - return new BackupSnapshotAnswer(cmd, false, e.toString(), null); + return new BackupSnapshotAnswer(cmd, false, e.toString(), null, true); } catch (URISyntaxException e) { - return new BackupSnapshotAnswer(cmd, false, e.toString(), null); + return new BackupSnapshotAnswer(cmd, false, e.toString(), null, true); } - return new BackupSnapshotAnswer(cmd, true, null, snapshotDestPath + File.separator + snapshotName); + return new BackupSnapshotAnswer(cmd, true, null, snapshotDestPath + File.separator + snapshotName, true); } protected DeleteSnapshotBackupAnswer execute(final DeleteSnapshotBackupCommand cmd) { diff --git a/api/src/com/cloud/agent/api/BackupSnapshotAnswer.java b/api/src/com/cloud/agent/api/BackupSnapshotAnswer.java index 93b5fe78520..807ce0d3851 100644 --- a/api/src/com/cloud/agent/api/BackupSnapshotAnswer.java +++ b/api/src/com/cloud/agent/api/BackupSnapshotAnswer.java @@ -20,14 +20,16 @@ package com.cloud.agent.api; public class BackupSnapshotAnswer extends Answer { private String backupSnapshotName; + private boolean full; protected BackupSnapshotAnswer() { } - public BackupSnapshotAnswer(BackupSnapshotCommand cmd, boolean success, String result, String backupSnapshotName) { + public BackupSnapshotAnswer(BackupSnapshotCommand cmd, boolean success, String result, String backupSnapshotName, boolean full) { super(cmd, success, result); this.backupSnapshotName = backupSnapshotName; + this.full = full; } /** @@ -36,4 +38,8 @@ public class BackupSnapshotAnswer extends Answer { public String getBackupSnapshotName() { return backupSnapshotName; } + + public boolean isFull() { + return full; + } } diff --git a/core/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java b/core/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java index f6b6e524e4b..792f1b31bf9 100644 --- a/core/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java +++ b/core/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java @@ -5013,10 +5013,12 @@ public abstract class CitrixResourceBase implements ServerResource { String secondaryStoragePoolURL = cmd.getSecondaryStoragePoolURL(); String snapshotUuid = cmd.getSnapshotUuid(); // not null: Precondition. String prevBackupUuid = cmd.getPrevBackupUuid(); + String prevSnapshotUuid = cmd.getPrevSnapshotUuid(); // By default assume failure String details = null; boolean success = false; String snapshotBackupUuid = null; + boolean fullbackup = true; try { SR primaryStorageSR = getSRByNameLabelandHost(conn, primaryStorageNameLabel); if (primaryStorageSR == null) { @@ -5025,22 +5027,30 @@ public abstract class CitrixResourceBase implements ServerResource { URI uri = new URI(secondaryStoragePoolURL); String secondaryStorageMountPath = uri.getHost() + ":" + uri.getPath(); - + VDI snapshotVdi = getVDIbyUuid(conn, snapshotUuid); - if (prevBackupUuid == null) { + if ( prevBackupUuid != null ) { + try { + VDI preSnapshotVdi = getVDIbyUuid(conn, prevSnapshotUuid); + if ( snapshotVdi.getParent(conn).getParent(conn) == preSnapshotVdi.getParent(conn) ) { + fullbackup = false; + } + } catch (Exception e) { + } + } + + if (fullbackup) { // the first snapshot is always a full snapshot String folder = "snapshots/" + accountId + "/" + volumeId; if( !createSecondaryStorageFolder(conn, secondaryStorageMountPath, folder)) { details = " Filed to create folder " + folder + " in secondary storage"; s_logger.warn(details); - return new BackupSnapshotAnswer(cmd, success, details, snapshotBackupUuid); + return new BackupSnapshotAnswer(cmd, false, details, null, false); } - String snapshotMountpoint = secondaryStoragePoolURL + "/" + folder; SR snapshotSr = null; try { snapshotSr = createNfsSRbyURI(conn, new URI(snapshotMountpoint), false); - VDI snapshotVdi = getVDIbyUuid(conn, snapshotUuid); VDI backedVdi = cloudVDIcopy(conn, snapshotVdi, snapshotSr); snapshotBackupUuid = backedVdi.getUuid(conn); success = true; @@ -5050,21 +5060,16 @@ public abstract class CitrixResourceBase implements ServerResource { } } } else { - String primaryStorageSRUuid = primaryStorageSR.getUuid(conn); - Boolean isISCSI = IsISCSI(primaryStorageSR.getType(conn)); - snapshotBackupUuid = backupSnapshot(conn, primaryStorageSRUuid, dcId, accountId, volumeId, secondaryStorageMountPath, - snapshotUuid, prevBackupUuid, isISCSI); - success = (snapshotBackupUuid != null); + String primaryStorageSRUuid = primaryStorageSR.getUuid(conn); + Boolean isISCSI = IsISCSI(primaryStorageSR.getType(conn)); + snapshotBackupUuid = backupSnapshot(conn, primaryStorageSRUuid, dcId, accountId, volumeId, secondaryStorageMountPath, snapshotUuid, prevBackupUuid, isISCSI); + success = (snapshotBackupUuid != null); } - if (success) { details = "Successfully backedUp the snapshotUuid: " + snapshotUuid + " to secondary storage."; - String volumeUuid = cmd.getVolumePath(); destroySnapshotOnPrimaryStorageExceptThis(conn, volumeUuid, snapshotUuid); - } - } catch (XenAPIException e) { details = "BackupSnapshot Failed due to " + e.toString(); s_logger.warn(details, e); @@ -5073,7 +5078,7 @@ public abstract class CitrixResourceBase implements ServerResource { s_logger.warn(details, e); } - return new BackupSnapshotAnswer(cmd, success, details, snapshotBackupUuid); + return new BackupSnapshotAnswer(cmd, success, details, snapshotBackupUuid, fullbackup); } protected CreateVolumeFromSnapshotAnswer execute(final CreateVolumeFromSnapshotCommand cmd) { @@ -5168,7 +5173,7 @@ public abstract class CitrixResourceBase implements ServerResource { } } } - return new DeleteSnapshotBackupAnswer(cmd, success, details); + return new DeleteSnapshotBackupAnswer(cmd, true, details); } protected Answer execute(DeleteSnapshotsDirCommand cmd) { diff --git a/server/src/com/cloud/storage/dao/SnapshotDao.java b/server/src/com/cloud/storage/dao/SnapshotDao.java index fcd6d6dd064..061d3adbd30 100644 --- a/server/src/com/cloud/storage/dao/SnapshotDao.java +++ b/server/src/com/cloud/storage/dao/SnapshotDao.java @@ -32,5 +32,6 @@ public interface SnapshotDao extends GenericDao { long getLastSnapshot(long volumeId, long snapId); List listByVolumeIdType(long volumeId, String type); List listByVolumeIdIncludingRemoved(long volumeId); + List listByBackupUuid(long volumeId, String backupUuid); } diff --git a/server/src/com/cloud/storage/dao/SnapshotDaoImpl.java b/server/src/com/cloud/storage/dao/SnapshotDaoImpl.java index fe7f44dc68b..49b00ad9566 100644 --- a/server/src/com/cloud/storage/dao/SnapshotDaoImpl.java +++ b/server/src/com/cloud/storage/dao/SnapshotDaoImpl.java @@ -41,6 +41,8 @@ public class SnapshotDaoImpl extends GenericDaoBase implements private final SearchBuilder VolumeIdSearch; private final SearchBuilder VolumeIdTypeSearch; private final SearchBuilder ParentIdSearch; + private final SearchBuilder backupUuidSearch; + @Override public SnapshotVO findNextSnapshot(long snapshotId) { @@ -49,6 +51,13 @@ public class SnapshotDaoImpl extends GenericDaoBase implements return findOneIncludingRemovedBy(sc); } + @Override + public List listByBackupUuid(long volumeId, String backupUuid) { + SearchCriteria sc = backupUuidSearch.create(); + sc.setParameters("backupUuid", backupUuid); + return listBy(sc, null); + } + @Override public List listByVolumeIdType(long volumeId, String type ) { return listByVolumeIdType(null, volumeId, type); @@ -93,6 +102,10 @@ public class SnapshotDaoImpl extends GenericDaoBase implements ParentIdSearch = createSearchBuilder(); ParentIdSearch.and("prevSnapshotId", ParentIdSearch.entity().getPrevSnapshotId(), SearchCriteria.Op.EQ); ParentIdSearch.done(); + + backupUuidSearch = createSearchBuilder(); + backupUuidSearch.and("backupUuid", backupUuidSearch.entity().getBackupSnapshotId(), SearchCriteria.Op.EQ); + backupUuidSearch.done(); } diff --git a/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java b/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java index b45281c9679..fbe01b3b9df 100755 --- a/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java +++ b/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java @@ -209,9 +209,6 @@ public class SnapshotManagerImpl implements SnapshotManager, SnapshotService, Ma if (storagePoolVO == null) { throw new InvalidParameterValueException("VolumeId: " + volumeId + " does not have a valid storage pool. Is it destroyed?"); } - if (!isVolumeDirty(volumeId, policyId)) { - throw new CloudRuntimeException("There is no change for volume " + volumeId + " since last snapshot, please use the last snapshot instead."); - } Long id = null; @@ -242,11 +239,10 @@ public class SnapshotManagerImpl implements SnapshotManager, SnapshotService, Ma long preId = _snapshotDao.getLastSnapshot(volumeId, id); String preSnapshotPath = null; - SnapshotVO preSnapshotVO = null; if( preId != 0) { preSnapshotVO = _snapshotDao.findByIdIncludingRemoved(preId); - if (preSnapshotVO != null) { + if (preSnapshotVO != null && preSnapshotVO.getBackupSnapshotId() != null ) { preSnapshotPath = preSnapshotVO.getPath(); } } @@ -262,10 +258,11 @@ public class SnapshotManagerImpl implements SnapshotManager, SnapshotService, Ma //empty snapshot s_logger.debug("CreateSnapshot: this is empty snapshot, remove it "); createdSnapshot = _snapshotDao.findByIdIncludingRemoved(id); - // delete from the snapshots table - _snapshotDao.expunge(id); - throw new CloudRuntimeException("There is no change for volume " + volumeId + " since last snapshot, please use last snapshot instead."); - + createdSnapshot.setPath(preSnapshotPath); + createdSnapshot.setBackupSnapshotId(preSnapshotVO.getBackupSnapshotId()); + createdSnapshot.setStatus(Snapshot.Status.BackedUp); + createdSnapshot.setPrevSnapshotId(preId); + _snapshotDao.update(id, createdSnapshot); } else { long preSnapshotId = 0; if( preSnapshotVO != null && preSnapshotVO.getBackupSnapshotId() != null) { @@ -292,16 +289,17 @@ public class SnapshotManagerImpl implements SnapshotManager, SnapshotService, Ma } } createdSnapshot = updateDBOnCreate(id, answer.getSnapshotPath(), preSnapshotId); - // Get the snapshot_schedule table entry for this snapshot and - // policy id. - // Set the snapshotId to retrieve it back later. - if( policyId != Snapshot.MANUAL_POLICY_ID) { - SnapshotScheduleVO snapshotSchedule = _snapshotScheduleDao.getCurrentSchedule(volumeId, policyId, true); - assert snapshotSchedule != null; - snapshotSchedule.setSnapshotId(id); - _snapshotScheduleDao.update(snapshotSchedule.getId(), snapshotSchedule); - } } + // Get the snapshot_schedule table entry for this snapshot and + // policy id. + // Set the snapshotId to retrieve it back later. + if (policyId != Snapshot.MANUAL_POLICY_ID) { + SnapshotScheduleVO snapshotSchedule = _snapshotScheduleDao.getCurrentSchedule(volumeId, policyId, true); + assert snapshotSchedule != null; + snapshotSchedule.setSnapshotId(id); + _snapshotScheduleDao.update(snapshotSchedule.getId(), snapshotSchedule); + } + } else { if (answer != null) { s_logger.error(answer.getDetails()); @@ -430,7 +428,6 @@ public class SnapshotManagerImpl implements SnapshotManager, SnapshotService, Ma @Override @DB public boolean backupSnapshotToSecondaryStorage(SnapshotVO ss) { - Long userId = getSnapshotUserId(); long snapshotId = ss.getId(); SnapshotVO snapshot = _snapshotDao.acquireInLockTable(snapshotId); if( snapshot == null) { @@ -460,8 +457,10 @@ public class SnapshotManagerImpl implements SnapshotManager, SnapshotService, Ma long prevSnapshotId = snapshot.getPrevSnapshotId(); if (prevSnapshotId > 0) { prevSnapshot = _snapshotDao.findByIdIncludingRemoved(prevSnapshotId); - prevSnapshotUuid = prevSnapshot.getPath(); prevBackupUuid = prevSnapshot.getBackupSnapshotId(); + if( prevBackupUuid != null ) { + prevSnapshotUuid = prevSnapshot.getPath(); + } } boolean isVolumeInactive = _storageMgr.volumeInactive(volume); @@ -505,6 +504,9 @@ public class SnapshotManagerImpl implements SnapshotManager, SnapshotService, Ma if (backedUp) { snapshot.setBackupSnapshotId(backedUpSnapshotUuid); + if( answer.isFull()) { + snapshot.setPrevSnapshotId(0); + } snapshot.setStatus(Snapshot.Status.BackedUp); _snapshotDao.update(snapshotId, snapshot); UsageEventVO usageEvent = new UsageEventVO(EventTypes.EVENT_SNAPSHOT_CREATE, snapshot.getAccountId(), volume.getDataCenterId(), snapshotId, snapshot.getName(), null, null, volume.getSize()); @@ -675,11 +677,17 @@ public class SnapshotManagerImpl implements SnapshotManager, SnapshotService, Ma while (lastSnapshot.getRemoved() != null) { String BackupSnapshotId = lastSnapshot.getBackupSnapshotId(); if (BackupSnapshotId != null) { - if (destroySnapshotBackUp(lastId, policyId)) { - + List snaps = _snapshotDao.listByBackupUuid(lastSnapshot.getVolumeId(), BackupSnapshotId); + if ( snaps.size() > 1 ) { + lastSnapshot.setBackupSnapshotId(null); + _snapshotDao.update(lastSnapshot.getId(), lastSnapshot); } else { - s_logger.debug("Destroying snapshot backup failed " + lastSnapshot); - break; + if (destroySnapshotBackUp(lastId, policyId)) { + + } else { + s_logger.debug("Destroying snapshot backup failed " + lastSnapshot); + break; + } } } postDeleteSnapshot(lastId, policyId); @@ -715,17 +723,20 @@ public class SnapshotManagerImpl implements SnapshotManager, SnapshotService, Ma Long volumeId = volume.getId(); String backupOfSnapshot = snapshot.getBackupSnapshotId(); - + if ( backupOfSnapshot == null ) { + return true; + } DeleteSnapshotBackupCommand cmd = new DeleteSnapshotBackupCommand(primaryStoragePoolNameLabel, secondaryStoragePoolUrl, dcId, accountId, volumeId, backupOfSnapshot, snapshot.getName()); + snapshot.setBackupSnapshotId(null); + _snapshotDao.update(snapshotId, snapshot); details = "Failed to destroy snapshot id:" + snapshotId + " for volume: " + volume.getId(); Answer answer = _storageMgr.sendToHostsOnStoragePool(volume.getPoolId(), cmd, details, _totalRetries, _pauseInterval, _shouldBeSnapshotCapable, volume.getInstanceId()); if ((answer != null) && answer.getResult()) { - snapshot.setBackupSnapshotId(null); - _snapshotDao.update(snapshotId, snapshot); + // This is not the last snapshot. success = true; details = "Successfully deleted snapshot " + snapshotId + " for volumeId: " + volumeId + " and policyId "