Cleanup snapshot files in datastores for Error-ed snapshots, and some code improvements (#12347)

engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotDataFactory.java

@@ -30,6 +30,8 @@ public interface SnapshotDataFactory {
 
     SnapshotInfo getSnapshot(long snapshotId, long storeId, DataStoreRole role);
 
+    SnapshotInfo getSnapshotIncludingRemoved(long snapshotId, long storeId, DataStoreRole role);
+
     SnapshotInfo getSnapshotWithRoleAndZone(long snapshotId, DataStoreRole role, long zoneId);
 
     SnapshotInfo getSnapshotOnPrimaryStore(long snapshotId);

engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDao.java

@@ -47,6 +47,8 @@ public interface SnapshotDao extends GenericDao<SnapshotVO, Long>, StateDao<Snap
 
     List<SnapshotVO> listAllByStatus(Snapshot.State... status);
 
+    List<SnapshotVO> listAllByStatusIncludingRemoved(Snapshot.State... status);
+
     void updateVolumeIds(long oldVolId, long newVolId);
 
     List<SnapshotVO> listByStatusNotIn(long volumeId, Snapshot.State... status);

engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDaoImpl.java

@@ -252,6 +252,13 @@ public class SnapshotDaoImpl extends GenericDaoBase<SnapshotVO, Long> implements
         return listBy(sc, null);
     }
 
+    @Override
+    public List<SnapshotVO> listAllByStatusIncludingRemoved(Snapshot.State... status) {
+        SearchCriteria<SnapshotVO> sc = StatusSearch.create();
+        sc.setParameters("status", (Object[])status);
+        return listIncludingRemovedBy(sc, null);
+    }
+
     @Override
     public List<SnapshotVO> listByIds(Object... ids) {
         SearchCriteria<SnapshotVO> sc = snapshotIdsSearch.create();

engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDao.java

@@ -56,6 +56,8 @@ StateDao<ObjectInDataStoreStateMachine.State, ObjectInDataStoreStateMachine.Even
 
     List<SnapshotDataStoreVO> findBySnapshotId(long snapshotId);
 
+    List<SnapshotDataStoreVO> findBySnapshotIdWithNonDestroyedState(long snapshotId);
+
     void duplicateCacheRecordsOnRegionStore(long storeId);
 
     // delete the snapshot entry on primary data store to make sure that next snapshot will be full snapshot

engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java

@@ -340,6 +340,13 @@ public class SnapshotDataStoreDaoImpl extends GenericDaoBase<SnapshotDataStoreVO
 
     @Override
     public List<SnapshotDataStoreVO> findBySnapshotId(long snapshotId) {
+        SearchCriteria<SnapshotDataStoreVO> sc = searchFilteringStoreIdEqStateEqStoreRoleEqIdEqUpdateCountEqSnapshotIdEqVolumeIdEq.create();
+        sc.setParameters(SNAPSHOT_ID, snapshotId);
+        return listBy(sc);
+    }
+
+    @Override
+    public List<SnapshotDataStoreVO> findBySnapshotIdWithNonDestroyedState(long snapshotId) {
         SearchCriteria<SnapshotDataStoreVO> sc = idStateNeqSearch.create();
         sc.setParameters(SNAPSHOT_ID, snapshotId);
         sc.setParameters(STATE, State.Destroyed);

engine/storage/integration-test/src/test/java/org/apache/cloudstack/storage/test/SnapshotTest.java

@@ -268,7 +268,7 @@ public class SnapshotTest extends CloudStackTestNGBase {
         to.setSize(1000L);
         CopyCmdAnswer answer = new CopyCmdAnswer(to);
         templateOnStore.processEvent(Event.CreateOnlyRequested);
-        templateOnStore.processEvent(Event.OperationSuccessed, answer);
+        templateOnStore.processEvent(Event.OperationSucceeded, answer);
 
     }
 

engine/storage/integration-test/src/test/java/org/apache/cloudstack/storage/test/VolumeTest.java

@@ -244,7 +244,7 @@ public class VolumeTest extends CloudStackTestNGBase {
         to.setSize(100L);
         CopyCmdAnswer answer = new CopyCmdAnswer(to);
         templateOnStore.processEvent(Event.CreateOnlyRequested);
-        templateOnStore.processEvent(Event.OperationSuccessed, answer);
+        templateOnStore.processEvent(Event.OperationSucceeded, answer);
 
     }
 

engine/storage/integration-test/src/test/java/org/apache/cloudstack/storage/test/VolumeTestVmware.java

@@ -246,7 +246,7 @@ public class VolumeTestVmware extends CloudStackTestNGBase {
         to.setPath(this.getImageInstallPath());
         CopyCmdAnswer answer = new CopyCmdAnswer(to);
         templateOnStore.processEvent(Event.CreateOnlyRequested);
-        templateOnStore.processEvent(Event.OperationSuccessed, answer);
+        templateOnStore.processEvent(Event.OperationSucceeded, answer);
 
     }
 

engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java

@@ -270,7 +270,7 @@ public class DefaultSnapshotStrategy extends SnapshotStrategyBase {
         }
 
         if (Snapshot.State.Error.equals(snapshotVO.getState())) {
-            List<SnapshotDataStoreVO> storeRefs = snapshotStoreDao.findBySnapshotId(snapshotId);
+            List<SnapshotDataStoreVO> storeRefs = snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshotId);
             List<Long> deletedRefs = new ArrayList<>();
             for (SnapshotDataStoreVO ref : storeRefs) {
                 boolean refZoneIdMatch = false;
@@ -351,7 +351,7 @@ public class DefaultSnapshotStrategy extends SnapshotStrategyBase {
     protected Boolean deleteSnapshotInfo(SnapshotInfo snapshotInfo, SnapshotVO snapshotVo) {
         DataStore dataStore = snapshotInfo.getDataStore();
         String storageToString = String.format("%s {uuid: \"%s\", name: \"%s\"}", dataStore.getRole().name(), dataStore.getUuid(), dataStore.getName());
-        List<SnapshotDataStoreVO> snapshotStoreRefs = snapshotStoreDao.findBySnapshotId(snapshotVo.getId());
+        List<SnapshotDataStoreVO> snapshotStoreRefs = snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshotVo.getId());
         boolean isLastSnapshotRef = CollectionUtils.isEmpty(snapshotStoreRefs) || snapshotStoreRefs.size() == 1;
         try {
             SnapshotObject snapshotObject = castSnapshotInfoToSnapshotObject(snapshotInfo);

engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImpl.java

@@ -94,7 +94,7 @@ public class SnapshotDataFactoryImpl implements SnapshotDataFactory {
         if (snapshot == null) { //snapshot may have been removed;
             return new ArrayList<>();
         }
-        List<SnapshotDataStoreVO> allSnapshotsAndDataStore = snapshotStoreDao.findBySnapshotId(snapshotId);
+        List<SnapshotDataStoreVO> allSnapshotsAndDataStore = snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshotId);
         if (CollectionUtils.isEmpty(allSnapshotsAndDataStore)) {
             return new ArrayList<>();
         }
@@ -118,7 +118,23 @@ public class SnapshotDataFactoryImpl implements SnapshotDataFactory {
         if (snapshot == null) {
             return null;
         }
-        SnapshotDataStoreVO snapshotStore = snapshotStoreDao.findByStoreSnapshot(role, storeId, snapshotId);
+        return getSnapshotOnStore(snapshot, storeId, role);
+    }
+
+    @Override
+    public SnapshotInfo getSnapshotIncludingRemoved(long snapshotId, long storeId, DataStoreRole role) {
+        SnapshotVO snapshot = snapshotDao.findByIdIncludingRemoved(snapshotId);
+        if (snapshot == null) {
+            return null;
+        }
+        return getSnapshotOnStore(snapshot, storeId, role);
+    }
+
+    private SnapshotInfo getSnapshotOnStore(SnapshotVO snapshot, long storeId, DataStoreRole role) {
+        if (snapshot == null) {
+            return null;
+        }
+        SnapshotDataStoreVO snapshotStore = snapshotStoreDao.findByStoreSnapshot(role, storeId, snapshot.getId());
         if (snapshotStore == null) {
             return null;
         }
@@ -207,7 +223,7 @@ public class SnapshotDataFactoryImpl implements SnapshotDataFactory {
 
     @Override
     public void updateOperationFailed(long snapshotId) throws NoTransitionException {
-        List<SnapshotDataStoreVO> snapshotStoreRefs = snapshotStoreDao.findBySnapshotId(snapshotId);
+        List<SnapshotDataStoreVO> snapshotStoreRefs = snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshotId);
         for (SnapshotDataStoreVO snapshotStoreRef : snapshotStoreRefs) {
             SnapshotInfo snapshotInfo = getSnapshot(snapshotStoreRef.getSnapshotId(), snapshotStoreRef.getDataStoreId(), snapshotStoreRef.getRole());
             if (snapshotInfo != null) {

engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java

@@ -382,8 +382,7 @@ public class SnapshotServiceImpl implements SnapshotService {
             if (res.isFailed()) {
                 throw new CloudRuntimeException(res.getResult());
             }
-            SnapshotInfo destSnapshot = res.getSnapshot();
+            return res.getSnapshot();
-            return destSnapshot;
         } catch (InterruptedException e) {
             logger.debug("failed copy snapshot", e);
             throw new CloudRuntimeException("Failed to copy snapshot", e);
@@ -391,7 +390,6 @@ public class SnapshotServiceImpl implements SnapshotService {
             logger.debug("Failed to copy snapshot", e);
             throw new CloudRuntimeException("Failed to copy snapshot", e);
         }
-
     }
 
     protected Void copySnapshotAsyncCallback(AsyncCallbackDispatcher<SnapshotServiceImpl, CopyCommandResult> callback, CopySnapshotContext<CommandResult> context) {
@@ -479,7 +477,6 @@ public class SnapshotServiceImpl implements SnapshotService {
     }
 
     protected Void deleteSnapshotCallback(AsyncCallbackDispatcher<SnapshotServiceImpl, CommandResult> callback, DeleteSnapshotContext<CommandResult> context) {
-
         CommandResult result = callback.getResult();
         AsyncCallFuture<SnapshotResult> future = context.future;
         SnapshotInfo snapshot = context.snapshot;
@@ -607,7 +604,7 @@ public class SnapshotServiceImpl implements SnapshotService {
 
         if (snapshot != null) {
             if (snapshot.getState() != Snapshot.State.BackedUp) {
-                List<SnapshotDataStoreVO> snapshotDataStoreVOs = _snapshotStoreDao.findBySnapshotId(snapshotId);
+                List<SnapshotDataStoreVO> snapshotDataStoreVOs = _snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshotId);
                 for (SnapshotDataStoreVO snapshotDataStoreVO : snapshotDataStoreVOs) {
                     logger.debug("Remove snapshot {}, status {} on snapshot_store_ref table with id: {}", snapshot, snapshotDataStoreVO.getState(), snapshotDataStoreVO.getId());
 
@@ -712,7 +709,6 @@ public class SnapshotServiceImpl implements SnapshotService {
                     SnapshotObject srcSnapshot = (SnapshotObject)snapshot;
                     srcSnapshot.processEvent(Event.DestroyRequested);
                     srcSnapshot.processEvent(Event.OperationSucceeded);
-
                     srcSnapshot.processEvent(Snapshot.Event.OperationFailed);
 
                     _snapshotDetailsDao.removeDetail(srcSnapshot.getId(), AsyncJob.Constants.MS_ID);
@@ -723,7 +719,6 @@ public class SnapshotServiceImpl implements SnapshotService {
                 }
             }
         });
-
     }
 
     @Override

engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/StorageSystemSnapshotStrategy.java

@@ -540,7 +540,6 @@ public class StorageSystemSnapshotStrategy extends SnapshotStrategyBase {
                 logger.warn("Failed to clean up snapshot '" + snapshot.getId() + "' on primary storage: " + e.getMessage());
             }
         }
-
     }
 
     private VMSnapshot takeHypervisorSnapshot(VolumeInfo volumeInfo) {

engine/storage/src/main/java/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java

@@ -101,6 +101,9 @@ public class ObjectInDataStoreManagerImpl implements ObjectInDataStoreManager {
         stateMachines.addTransition(State.Destroying, Event.DestroyRequested, State.Destroying);
         stateMachines.addTransition(State.Destroying, Event.OperationSucceeded, State.Destroyed);
         stateMachines.addTransition(State.Destroying, Event.OperationFailed, State.Destroying);
+        stateMachines.addTransition(State.Destroyed, Event.DestroyRequested, State.Destroyed);
+        stateMachines.addTransition(State.Destroyed, Event.OperationSucceeded, State.Destroyed);
+        stateMachines.addTransition(State.Destroyed, Event.OperationFailed, State.Destroyed);
         stateMachines.addTransition(State.Failed, Event.DestroyRequested, State.Destroying);
         // TODO: further investigate why an extra event is sent when it is
         // already Ready for DownloadListener

engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java

@@ -704,7 +704,7 @@ public class VolumeServiceImpl implements VolumeService {
         VolumeApiResult res = new VolumeApiResult(volumeInfo);
 
         if (result.isSuccess()) {
-            // volumeInfo.processEvent(Event.OperationSuccessed, result.getAnswer());
+            // volumeInfo.processEvent(Event.OperationSucceeded, result.getAnswer());
 
             VolumeVO volume = volDao.findById(volumeInfo.getId());
             CopyCmdAnswer answer = (CopyCmdAnswer)result.getAnswer();

plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/util/StorPoolHelper.java

@@ -102,7 +102,7 @@ public class StorPoolHelper {
         if (snapshotDetails != null) {
             return StorPoolStorageAdaptor.getVolumeNameFromPath(snapshotDetails.getValue(), true);
         } else {
-            List<SnapshotDataStoreVO> snapshots = snapshotStoreDao.findBySnapshotId(snapshotId);
+            List<SnapshotDataStoreVO> snapshots = snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshotId);
             if (!CollectionUtils.isEmpty(snapshots)) {
                 for (SnapshotDataStoreVO snapshotDataStoreVO : snapshots) {
                     String name = StorPoolStorageAdaptor.getVolumeNameFromPath(snapshotDataStoreVO.getInstallPath(), true);

plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/snapshot/StorPoolSnapshotStrategy.java

@@ -240,7 +240,7 @@ public class StorPoolSnapshotStrategy implements SnapshotStrategy {
     }
 
     protected boolean areLastSnapshotRef(long snapshotId) {
-        List<SnapshotDataStoreVO> snapshotStoreRefs = _snapshotStoreDao.findBySnapshotId(snapshotId);
+        List<SnapshotDataStoreVO> snapshotStoreRefs = _snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshotId);
         if (CollectionUtils.isEmpty(snapshotStoreRefs) || snapshotStoreRefs.size() == 1) {
             return true;
         }
@@ -308,7 +308,7 @@ public class StorPoolSnapshotStrategy implements SnapshotStrategy {
         }
 
         if (Snapshot.State.Error.equals(snapshotVO.getState())) {
-            List<SnapshotDataStoreVO> storeRefs = _snapshotStoreDao.findBySnapshotId(snapshotId);
+            List<SnapshotDataStoreVO> storeRefs = _snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshotId);
             List<Long> deletedRefs = new ArrayList<>();
             for (SnapshotDataStoreVO ref : storeRefs) {
                 boolean refZoneIdMatch = false;

server/src/main/java/com/cloud/storage/StorageManagerImpl.java

@@ -1867,41 +1867,7 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C
                         }
                     }
 
-                    //destroy snapshots in destroying state in snapshot_store_ref
+                    cleanupSnapshotsFromStoreRefInDestroyingState();
-                    List<SnapshotDataStoreVO> ssSnapshots = _snapshotStoreDao.listByState(ObjectInDataStoreStateMachine.State.Destroying);
-                    for (SnapshotDataStoreVO snapshotDataStoreVO : ssSnapshots) {
-                        String snapshotUuid = null;
-                        SnapshotVO snapshot = null;
-                        final String storeRole = snapshotDataStoreVO.getRole().toString().toLowerCase();
-                        if (logger.isDebugEnabled()) {
-                            snapshot = _snapshotDao.findById(snapshotDataStoreVO.getSnapshotId());
-                            if (snapshot == null) {
-                                logger.warn(String.format("Did not find snapshot [ID: %d] for which store reference is in destroying state; therefore, it cannot be destroyed.", snapshotDataStoreVO.getSnapshotId()));
-                                continue;
-                            }
-                            snapshotUuid = snapshot.getUuid();
-                        }
-
-                        try {
-                            if (logger.isDebugEnabled()) {
-                                logger.debug(String.format("Verifying if snapshot [%s] is in destroying state in %s data store ID: %d.", snapshotUuid, storeRole, snapshotDataStoreVO.getDataStoreId()));
-                            }
-                            SnapshotInfo snapshotInfo = snapshotFactory.getSnapshot(snapshotDataStoreVO.getSnapshotId(), snapshotDataStoreVO.getDataStoreId(), snapshotDataStoreVO.getRole());
-                            if (snapshotInfo != null) {
-                                if (logger.isDebugEnabled()) {
-                                    logger.debug(String.format("Snapshot [%s] in destroying state found in %s data store [%s]; therefore, it will be destroyed.", snapshotUuid, storeRole, snapshotInfo.getDataStore().getUuid()));
-                                }
-                                _snapshotService.deleteSnapshot(snapshotInfo);
-                            } else if (logger.isDebugEnabled()) {
-                                logger.debug(String.format("Did not find snapshot [%s] in destroying state in %s data store ID: %d.", snapshotUuid, storeRole, snapshotDataStoreVO.getDataStoreId()));
-                            }
-                        } catch (Exception e) {
-                            logger.error("Failed to delete snapshot [{}] from storage due to: [{}].", snapshot, e.getMessage());
-                            if (logger.isDebugEnabled()) {
-                                logger.debug("Failed to delete snapshot [{}] from storage.", snapshot, e);
-                            }
-                        }
-                    }
                     cleanupSecondaryStorage(recurring);
 
                     List<VolumeVO> vols = volumeDao.listVolumesToBeDestroyed(new Date(System.currentTimeMillis() - ((long)StorageCleanupDelay.value() << 10)));
@@ -1941,20 +1907,7 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C
                         }
                     }
 
-                    // remove snapshots in Error state
+                    removeSnapshotsInErrorStatus();
-                    List<SnapshotVO> snapshots = _snapshotDao.listAllByStatus(Snapshot.State.Error);
-                    for (SnapshotVO snapshotVO : snapshots) {
-                        try {
-                            List<SnapshotDataStoreVO> storeRefs = _snapshotStoreDao.findBySnapshotId(snapshotVO.getId());
-                            for (SnapshotDataStoreVO ref : storeRefs) {
-                                _snapshotStoreDao.expunge(ref.getId());
-                            }
-                            _snapshotDao.expunge(snapshotVO.getId());
-                        } catch (Exception e) {
-                            logger.error("Unable to destroy snapshot [{}] due to: [{}].", snapshotVO, e.getMessage());
-                            logger.debug("Unable to destroy snapshot [{}].", snapshotVO, e);
-                        }
-                    }
 
                     // destroy uploaded volumes in abandoned/error state
                     List<VolumeDataStoreVO> volumeDataStores = _volumeDataStoreDao.listByVolumeState(Volume.State.UploadError, Volume.State.UploadAbandoned);
@@ -2055,6 +2008,56 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C
         }
     }
 
+    private void cleanupSnapshotsFromStoreRefInDestroyingState() {
+        List<SnapshotDataStoreVO> storeRefSnapshotsInDestroyingState = _snapshotStoreDao.listByState(ObjectInDataStoreStateMachine.State.Destroying);
+        for (SnapshotDataStoreVO snapshotDataStoreVO : storeRefSnapshotsInDestroyingState) {
+            SnapshotVO snapshot = _snapshotDao.findById(snapshotDataStoreVO.getSnapshotId());
+            if (snapshot == null) {
+                logger.warn("Did not find snapshot [ID: {}] for which store reference is in destroying state; therefore, it cannot be destroyed.", snapshotDataStoreVO.getSnapshotId());
+                continue;
+            }
+            deleteSnapshot(snapshot, snapshotDataStoreVO);
+        }
+    }
+
+    private void deleteSnapshot(SnapshotVO snapshot, SnapshotDataStoreVO snapshotDataStoreVO) {
+        if (snapshot == null || snapshotDataStoreVO == null) {
+            return;
+        }
+
+        try {
+            final String snapshotUuid = snapshot.getUuid();
+            final String storeRole = snapshotDataStoreVO.getRole().toString().toLowerCase();
+            logger.debug("Snapshot [{}] is in {} state on {} data store ID: {}.", snapshotUuid, snapshotDataStoreVO.getState(), storeRole, snapshotDataStoreVO.getDataStoreId());
+            SnapshotInfo snapshotInfo = snapshotFactory.getSnapshotIncludingRemoved(snapshotDataStoreVO.getSnapshotId(), snapshotDataStoreVO.getDataStoreId(), snapshotDataStoreVO.getRole());
+            if (snapshotInfo != null) {
+                logger.debug("Snapshot [{}] in {} state found on {} data store [{}], it will be deleted.", snapshotUuid, snapshotDataStoreVO.getState(), storeRole, snapshotInfo.getDataStore().getUuid());
+                _snapshotService.deleteSnapshot(snapshotInfo);
+            } else {
+                logger.debug("Did not find snapshot [{}] in {} state on {} data store ID: {}.", snapshotUuid, snapshotDataStoreVO.getState(), storeRole, snapshotDataStoreVO.getDataStoreId());
+            }
+        } catch (Exception e) {
+            logger.error("Failed to delete snapshot [{}] from storage due to: [{}].", snapshot, e.getMessage(), e);
+        }
+    }
+
+    private void removeSnapshotsInErrorStatus() {
+        List<SnapshotVO> snapshotsInErrorStatus = _snapshotDao.listAllByStatusIncludingRemoved(Snapshot.State.Error);
+        for (SnapshotVO snapshotVO : snapshotsInErrorStatus) {
+            try {
+                List<SnapshotDataStoreVO> storeRefSnapshotsInErrorStatus = _snapshotStoreDao.findBySnapshotId(snapshotVO.getId());
+                for (SnapshotDataStoreVO snapshotDataStoreVO : storeRefSnapshotsInErrorStatus) {
+                    deleteSnapshot(snapshotVO, snapshotDataStoreVO);
+                    _snapshotStoreDao.expunge(snapshotDataStoreVO.getId());
+                }
+                _snapshotDao.expunge(snapshotVO.getId());
+            } catch (Exception e) {
+                logger.error("Unable to destroy snapshot [{}] due to: [{}].", snapshotVO, e.getMessage());
+                logger.debug("Unable to destroy snapshot [{}].", snapshotVO, e);
+            }
+        }
+    }
+
     protected boolean isVolumeSuspectedDestroyDuplicateOfVmVolume(VolumeVO gcVolume) {
         if (gcVolume.getPath() == null) {
             return false;

server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java

@@ -721,7 +721,7 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement
 
     protected Pair<List<SnapshotDataStoreVO>, List<Long>> getStoreRefsAndZonesForSnapshotDelete(long snapshotId, Long zoneId) {
         List<SnapshotDataStoreVO> snapshotStoreRefs = new ArrayList<>();
-        List<SnapshotDataStoreVO> allSnapshotStoreRefs = _snapshotStoreDao.findBySnapshotId(snapshotId);
+        List<SnapshotDataStoreVO> allSnapshotStoreRefs = _snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshotId);
         List<Long> zoneIds = new ArrayList<>();
         if (zoneId != null) {
             DataCenterVO zone = dataCenterDao.findById(zoneId);
@@ -1503,22 +1503,22 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement
         if (asyncBackup) {
             backupSnapshotExecutor.schedule(new BackupSnapshotTask(snapshotOnPrimary, snapshotBackupRetries - 1, snapshotStrategy, zoneIds), 0, TimeUnit.SECONDS);
         } else {
-            SnapshotInfo backupedSnapshot = snapshotStrategy.backupSnapshot(snapshotOnPrimary);
+            SnapshotInfo backedUpSnapshot = snapshotStrategy.backupSnapshot(snapshotOnPrimary);
-            if (backupedSnapshot != null) {
+            if (backedUpSnapshot != null) {
                 snapshotStrategy.postSnapshotCreation(snapshotOnPrimary);
             }
         }
     }
 
     protected class BackupSnapshotTask extends ManagedContextRunnable {
-        SnapshotInfo snapshot;
+        SnapshotInfo snapshotOnPrimary;
         int attempts;
         SnapshotStrategy snapshotStrategy;
 
         List<Long> zoneIds;
 
-        public BackupSnapshotTask(SnapshotInfo snap, int maxRetries, SnapshotStrategy strategy, List<Long> zoneIds) {
+        public BackupSnapshotTask(SnapshotInfo snapshot, int maxRetries, SnapshotStrategy strategy, List<Long> zoneIds) {
-            snapshot = snap;
+            snapshotOnPrimary = snapshot;
             attempts = maxRetries;
             snapshotStrategy = strategy;
             this.zoneIds = zoneIds;
@@ -1529,19 +1529,18 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement
             try {
                 logger.debug("Value of attempts is " + (snapshotBackupRetries - attempts));
 
-                SnapshotInfo backupedSnapshot = snapshotStrategy.backupSnapshot(snapshot);
+                SnapshotInfo backedUpSnapshot = snapshotStrategy.backupSnapshot(snapshotOnPrimary);
-
+                if (backedUpSnapshot != null) {
-                if (backupedSnapshot != null) {
+                    snapshotStrategy.postSnapshotCreation(snapshotOnPrimary);
-                    snapshotStrategy.postSnapshotCreation(snapshot);
+                    copyNewSnapshotToZones(snapshotOnPrimary.getId(), snapshotOnPrimary.getDataCenterId(), zoneIds);
-                    copyNewSnapshotToZones(snapshot.getId(), snapshot.getDataCenterId(), zoneIds);
                 }
             } catch (final Exception e) {
                 if (attempts >= 0) {
-                    logger.debug("Backing up of snapshot failed, for snapshot {}, left with {} more attempts", snapshot, attempts);
+                    logger.debug("Backing up of snapshot failed, for snapshot {}, left with {} more attempts", snapshotOnPrimary, attempts);
-                    backupSnapshotExecutor.schedule(new BackupSnapshotTask(snapshot, --attempts, snapshotStrategy, zoneIds), snapshotBackupRetryInterval, TimeUnit.SECONDS);
+                    backupSnapshotExecutor.schedule(new BackupSnapshotTask(snapshotOnPrimary, --attempts, snapshotStrategy, zoneIds), snapshotBackupRetryInterval, TimeUnit.SECONDS);
                 } else {
-                    logger.debug("Done with {} attempts in  backing up of snapshot {}", snapshotBackupRetries, snapshot.getSnapshotVO());
+                    logger.debug("Done with {} attempts in  backing up of snapshot {}", snapshotBackupRetries, snapshotOnPrimary.getSnapshotVO());
-                    snapshotSrv.cleanupOnSnapshotBackupFailure(snapshot);
+                    snapshotSrv.cleanupOnSnapshotBackupFailure(snapshotOnPrimary);
                 }
             }
         }
@@ -1762,7 +1761,7 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement
     public void markVolumeSnapshotsAsDestroyed(Volume volume) {
         List<SnapshotVO> snapshots = _snapshotDao.listByVolumeId(volume.getId());
         for (SnapshotVO snapshot: snapshots) {
-            List<SnapshotDataStoreVO> snapshotDataStoreVOs = _snapshotStoreDao.findBySnapshotId(snapshot.getId());
+            List<SnapshotDataStoreVO> snapshotDataStoreVOs = _snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshot.getId());
             if (CollectionUtils.isEmpty(snapshotDataStoreVOs)) {
                 snapshot.setState(Snapshot.State.Destroyed);
                 _snapshotDao.update(snapshot.getId(), snapshot);

server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerImplTest.java

@@ -139,7 +139,7 @@ public class SnapshotManagerImplTest {
         Mockito.when(ref1.getDataStoreId()).thenReturn(2L);
         Mockito.when(ref1.getRole()).thenReturn(DataStoreRole.Image);
         List<SnapshotDataStoreVO> snapshotStoreList = List.of(ref, ref1);
-        Mockito.when(snapshotStoreDao.findBySnapshotId(snapshotId)).thenReturn(snapshotStoreList);
+        Mockito.when(snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshotId)).thenReturn(snapshotStoreList);
         Mockito.when(dataStoreManager.getStoreZoneId(1L, DataStoreRole.Image)).thenReturn(100L);
         Mockito.when(dataStoreManager.getStoreZoneId(2L, DataStoreRole.Image)).thenReturn(101L);
         Pair<List<SnapshotDataStoreVO>, List<Long>> pair = snapshotManager.getStoreRefsAndZonesForSnapshotDelete(snapshotId, null);
@@ -164,7 +164,7 @@ public class SnapshotManagerImplTest {
         Mockito.when(ref2.getDataStoreId()).thenReturn(3L);
         Mockito.when(ref2.getRole()).thenReturn(DataStoreRole.Image);
         List<SnapshotDataStoreVO> snapshotStoreList = List.of(ref, ref1, ref2);
-        Mockito.when(snapshotStoreDao.findBySnapshotId(snapshotId)).thenReturn(snapshotStoreList);
+        Mockito.when(snapshotStoreDao.findBySnapshotIdWithNonDestroyedState(snapshotId)).thenReturn(snapshotStoreList);
         Mockito.when(dataStoreManager.getStoreZoneId(1L, DataStoreRole.Image)).thenReturn(zoneId);
         Mockito.when(dataStoreManager.getStoreZoneId(2L, DataStoreRole.Primary)).thenReturn(zoneId);
         Mockito.when(dataStoreManager.getStoreZoneId(3L, DataStoreRole.Image)).thenReturn(2L);