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

sureshanaparti · web-flow · commit f1f779a08d94 · 2026-01-15T12:12:32.000+01:00
diff --git a/engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotDataFactory.java b/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);
diff --git a/engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDao.java b/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);
diff --git a/engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDaoImpl.java b/engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDaoImpl.java
@@ -252,6 +252,13 @@ public List<SnapshotVO> listAllByStatus(Snapshot.State... status) {
         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();
diff --git a/engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDao.java b/engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDao.java
@@ -56,6 +56,8 @@ public interface SnapshotDataStoreDao extends GenericDao<SnapshotDataStoreVO, Lo
 
     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
diff --git a/engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java b/engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java
@@ -340,6 +340,13 @@ public List<SnapshotDataStoreVO> findByVolume(long snapshotId, long volumeId, Da
 
     @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);
diff --git a/engine/storage/integration-test/src/test/java/org/apache/cloudstack/storage/test/SnapshotTest.java b/engine/storage/integration-test/src/test/java/org/apache/cloudstack/storage/test/SnapshotTest.java
@@ -268,7 +268,7 @@ public void setUp() {
         to.setSize(1000L);
         CopyCmdAnswer answer = new CopyCmdAnswer(to);
         templateOnStore.processEvent(Event.CreateOnlyRequested);
-        templateOnStore.processEvent(Event.OperationSuccessed, answer);
+        templateOnStore.processEvent(Event.OperationSucceeded, answer);
 
     }
 
diff --git a/engine/storage/integration-test/src/test/java/org/apache/cloudstack/storage/test/VolumeTest.java b/engine/storage/integration-test/src/test/java/org/apache/cloudstack/storage/test/VolumeTest.java
@@ -244,7 +244,7 @@ public void setUp() {
         to.setSize(100L);
         CopyCmdAnswer answer = new CopyCmdAnswer(to);
         templateOnStore.processEvent(Event.CreateOnlyRequested);
-        templateOnStore.processEvent(Event.OperationSuccessed, answer);
+        templateOnStore.processEvent(Event.OperationSucceeded, answer);
 
     }
 
diff --git a/engine/storage/integration-test/src/test/java/org/apache/cloudstack/storage/test/VolumeTestVmware.java b/engine/storage/integration-test/src/test/java/org/apache/cloudstack/storage/test/VolumeTestVmware.java
@@ -246,7 +246,7 @@ public void setUp() {
         to.setPath(this.getImageInstallPath());
         CopyCmdAnswer answer = new CopyCmdAnswer(to);
         templateOnStore.processEvent(Event.CreateOnlyRequested);
-        templateOnStore.processEvent(Event.OperationSuccessed, answer);
+        templateOnStore.processEvent(Event.OperationSucceeded, answer);
 
     }
 
diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
@@ -270,7 +270,7 @@ public boolean deleteSnapshot(Long snapshotId, Long zoneId) {
         }
 
         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 @@ protected boolean deleteSnapshotInfos(SnapshotVO snapshotVo, Long zoneId) {
     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);
diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImpl.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImpl.java
@@ -94,7 +94,7 @@ public List<SnapshotInfo> getSnapshots(long snapshotId, Long zoneId) {
         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 SnapshotInfo getSnapshot(long snapshotId, long storeId, DataStoreRole rol
         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 List<SnapshotInfo> listSnapshotOnCache(long snapshotId) {
 
     @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) {
diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java
@@ -382,16 +382,14 @@ public SnapshotInfo backupSnapshot(SnapshotInfo snapshot) {
             if (res.isFailed()) {
                 throw new CloudRuntimeException(res.getResult());
             }
-            SnapshotInfo destSnapshot = res.getSnapshot();
-            return destSnapshot;
+            return res.getSnapshot();
         } catch (InterruptedException e) {
             logger.debug("failed copy snapshot", e);
             throw new CloudRuntimeException("Failed to copy snapshot", e);
         } catch (ExecutionException e) {
             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 @@ protected Void prepareCopySnapshotZoneAsyncCallback(AsyncCallbackDispatcher<Snap
     }
 
     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 void cleanupVolumeDuringSnapshotFailure(Long volumeId, Long snapshotId) {
 
         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 void doInTransactionWithoutResult(TransactionStatus status) {
                     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 void doInTransactionWithoutResult(TransactionStatus status) {
                 }
             }
         });
-
     }
 
     @Override
diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/StorageSystemSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/StorageSystemSnapshotStrategy.java
@@ -540,7 +540,6 @@ public void postSnapshotCreation(SnapshotInfo snapshot) {
                 logger.warn("Failed to clean up snapshot '" + snapshot.getId() + "' on primary storage: " + e.getMessage());
             }
         }
-
     }
 
     private VMSnapshot takeHypervisorSnapshot(VolumeInfo volumeInfo) {
diff --git a/engine/storage/src/main/java/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java b/engine/storage/src/main/java/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java
@@ -101,6 +101,9 @@ public ObjectInDataStoreManagerImpl() {
         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
diff --git a/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java b/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java
@@ -704,7 +704,7 @@ protected Void managedCopyBaseImageCallback(AsyncCallbackDispatcher<VolumeServic
         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();
diff --git a/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/util/StorPoolHelper.java b/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/util/StorPoolHelper.java
@@ -102,7 +102,7 @@ public static String getSnapshotName(Long snapshotId, String snapshotUuid, Snaps
         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);
diff --git a/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/snapshot/StorPoolSnapshotStrategy.java b/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/snapshot/StorPoolSnapshotStrategy.java
@@ -240,7 +240,7 @@ private boolean deleteSnapshotChain(SnapshotInfo snapshot) {
     }
 
     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 @@ private boolean deleteSnapshotFromDbIfNeeded(SnapshotVO snapshotVO, Long zoneId)
         }
 
         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;
diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java
diff --git a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
diff --git a/server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerImplTest.java b/server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerImplTest.java

Original file line number	Diff line number	Diff line change
`@@ -268,7 +268,7 @@ public void setUp() {`
`268`	`268`	`to.setSize(1000L);`
`269`	`269`	`CopyCmdAnswer answer = new CopyCmdAnswer(to);`
`270`	`270`	`templateOnStore.processEvent(Event.CreateOnlyRequested);`
`271`		`- templateOnStore.processEvent(Event.OperationSuccessed, answer);`
	`271`	`+ templateOnStore.processEvent(Event.OperationSucceeded, answer);`
`272`	`272`
`273`	`273`	`}`
`274`	`274`
Original file line number	Diff line number	Diff line change
`@@ -244,7 +244,7 @@ public void setUp() {`
`244`	`244`	`to.setSize(100L);`
`245`	`245`	`CopyCmdAnswer answer = new CopyCmdAnswer(to);`
`246`	`246`	`templateOnStore.processEvent(Event.CreateOnlyRequested);`
`247`		`- templateOnStore.processEvent(Event.OperationSuccessed, answer);`
	`247`	`+ templateOnStore.processEvent(Event.OperationSucceeded, answer);`
`248`	`248`
`249`	`249`	`}`
`250`	`250`
Original file line number	Diff line number	Diff line change
`@@ -246,7 +246,7 @@ public void setUp() {`
`246`	`246`	`to.setPath(this.getImageInstallPath());`
`247`	`247`	`CopyCmdAnswer answer = new CopyCmdAnswer(to);`
`248`	`248`	`templateOnStore.processEvent(Event.CreateOnlyRequested);`
`249`		`- templateOnStore.processEvent(Event.OperationSuccessed, answer);`
	`249`	`+ templateOnStore.processEvent(Event.OperationSucceeded, answer);`
`250`	`250`
`251`	`251`	`}`
`252`	`252`