From e580af8875ea45016725d37331e09d6ffcb328bc Mon Sep 17 00:00:00 2001 From: Damans227 <61474540+Damans227@users.noreply.github.com> Date: Fri, 13 Mar 2026 13:43:59 -0400 Subject: [PATCH 1/6] Clean up primary-only snapshot DB records during volume deletion to prevent orphaned entries --- .../cloud/storage/VolumeApiServiceImpl.java | 29 +++++++++ .../storage/VolumeApiServiceImplTest.java | 64 +++++++++++++++++++ 2 files changed, 93 insertions(+) diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index ca3d31d4fad3..9131a14ee791 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -1640,6 +1640,7 @@ public boolean deleteVolume(long volumeId, Account caller) throws ConcurrentOper private boolean deleteVolumeFromStorage(VolumeVO volume, Account caller) throws ConcurrentOperationException { try { + cleanupSnapshotRecordsInPrimaryStorageOnly(volume); expungeVolumesInPrimaryStorageIfNeeded(volume); expungeVolumesInSecondaryStorageIfNeeded(volume); cleanVolumesCache(volume); @@ -1650,6 +1651,34 @@ private boolean deleteVolumeFromStorage(VolumeVO volume, Account caller) throws } } + /** + * Cleans up snapshot DB records for snapshots that exist only on primary storage (no secondary copy). + * This handles the case where snapshot.backup.to.secondary=false and the volume + * is being deleted during VM expunge; the RBD snapshots will be destroyed along with the image, + * so the DB records need to be cleaned up to avoid orphaned entries. + */ + protected void cleanupSnapshotRecordsInPrimaryStorageOnly(VolumeVO volume) { + List snapshots = _snapshotDao.listByVolumeId(volume.getId()); + if (CollectionUtils.isEmpty(snapshots)) { + return; + } + for (SnapshotVO snapshot : snapshots) { + if (Snapshot.State.Destroyed.equals(snapshot.getState())) { + continue; + } + List primaryRefs = _snapshotDataStoreDao.listBySnapshotAndDataStoreRole(snapshot.getId(), DataStoreRole.Primary); + List secondaryRefs = _snapshotDataStoreDao.listBySnapshotAndDataStoreRole(snapshot.getId(), DataStoreRole.Image); + if (CollectionUtils.isNotEmpty(primaryRefs) && CollectionUtils.isEmpty(secondaryRefs)) { + logger.info("Cleaning up snapshot {} (primary-only, no secondary copy) as volume {} is being deleted", snapshot, volume); + for (SnapshotDataStoreVO ref : primaryRefs) { + _snapshotDataStoreDao.expunge(ref.getId()); + } + snapshot.setState(Snapshot.State.Destroyed); + _snapshotDao.update(snapshot.getId(), snapshot); + } + } + } + /** * Retrieves and validates the volume for the {@link #deleteVolume(long, Account)} method. The following validation are executed. *
    diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index 0575b430ef10..5e10b2cb8508 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -122,6 +122,8 @@ import com.cloud.storage.Volume.Type; import com.cloud.storage.dao.DiskOfferingDao; import com.cloud.storage.dao.SnapshotDao; +import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao; +import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO; import com.cloud.storage.dao.StoragePoolTagsDao; import com.cloud.storage.dao.VMTemplateDao; import com.cloud.storage.dao.VolumeDao; @@ -253,6 +255,9 @@ public class VolumeApiServiceImplTest { @Mock private SnapshotDao snapshotDaoMock; + @Mock + private SnapshotDataStoreDao snapshotDataStoreDaoMock; + @Mock private SnapshotPolicyDetailsDao snapshotPolicyDetailsDao; @@ -2288,4 +2293,63 @@ private List generateVmSnapshotVoList(VMSnapshot.Type t1, VMSnapsh Mockito.doReturn(1L).when(mock2).getId(); return List.of(mock1, mock2); } + + @Test + public void testCleanupSnapshotRecordsInPrimaryStorageOnly() { + VolumeVO volume = Mockito.mock(VolumeVO.class); + Mockito.when(volume.getId()).thenReturn(1L); + + SnapshotVO snapshot = Mockito.mock(SnapshotVO.class); + Mockito.when(snapshot.getId()).thenReturn(10L); + Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.BackedUp); + Mockito.when(snapshotDaoMock.listByVolumeId(1L)).thenReturn(List.of(snapshot)); + + SnapshotDataStoreVO primaryRef = Mockito.mock(SnapshotDataStoreVO.class); + Mockito.when(primaryRef.getId()).thenReturn(100L); + Mockito.when(snapshotDataStoreDaoMock.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Primary)).thenReturn(List.of(primaryRef)); + Mockito.when(snapshotDataStoreDaoMock.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Image)).thenReturn(Collections.emptyList()); + + volumeApiServiceImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume); + + Mockito.verify(snapshotDataStoreDaoMock).expunge(100L); + Mockito.verify(snapshot).setState(Snapshot.State.Destroyed); + Mockito.verify(snapshotDaoMock).update(10L, snapshot); + } + + @Test + public void testCleanupSnapshotRecordsInPrimaryStorageOnlySkipsWhenSecondaryExists() { + VolumeVO volume = Mockito.mock(VolumeVO.class); + Mockito.when(volume.getId()).thenReturn(1L); + + SnapshotVO snapshot = Mockito.mock(SnapshotVO.class); + Mockito.when(snapshot.getId()).thenReturn(10L); + Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.BackedUp); + Mockito.when(snapshotDaoMock.listByVolumeId(1L)).thenReturn(List.of(snapshot)); + + SnapshotDataStoreVO primaryRef = Mockito.mock(SnapshotDataStoreVO.class); + SnapshotDataStoreVO secondaryRef = Mockito.mock(SnapshotDataStoreVO.class); + Mockito.when(snapshotDataStoreDaoMock.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Primary)).thenReturn(List.of(primaryRef)); + Mockito.when(snapshotDataStoreDaoMock.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Image)).thenReturn(List.of(secondaryRef)); + + volumeApiServiceImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume); + + Mockito.verify(snapshotDataStoreDaoMock, Mockito.never()).expunge(Mockito.anyLong()); + Mockito.verify(snapshotDaoMock, Mockito.never()).update(Mockito.anyLong(), Mockito.any(SnapshotVO.class)); + } + + @Test + public void testCleanupSnapshotRecordsInPrimaryStorageOnlySkipsDestroyedSnapshots() { + VolumeVO volume = Mockito.mock(VolumeVO.class); + Mockito.when(volume.getId()).thenReturn(1L); + + SnapshotVO snapshot = Mockito.mock(SnapshotVO.class); + Mockito.when(snapshot.getId()).thenReturn(10L); + Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.Destroyed); + Mockito.when(snapshotDaoMock.listByVolumeId(1L)).thenReturn(List.of(snapshot)); + + volumeApiServiceImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume); + + Mockito.verify(snapshotDataStoreDaoMock, Mockito.never()).listBySnapshotAndDataStoreRole(Mockito.anyLong(), Mockito.any()); + Mockito.verify(snapshotDataStoreDaoMock, Mockito.never()).expunge(Mockito.anyLong()); + } } From 77515d1fa4c5acd5fe46b20357db211923958ba8 Mon Sep 17 00:00:00 2001 From: Damans227 <61474540+Damans227@users.noreply.github.com> Date: Fri, 13 Mar 2026 14:32:32 -0400 Subject: [PATCH 2/6] Clean up primary-only snapshot in StorageManagerImpl scavenger instead --- .../com/cloud/storage/StorageManagerImpl.java | 29 ++++++++ .../cloud/storage/VolumeApiServiceImpl.java | 29 -------- .../cloud/storage/StorageManagerImplTest.java | 66 +++++++++++++++++++ .../storage/VolumeApiServiceImplTest.java | 63 ------------------ 4 files changed, 95 insertions(+), 92 deletions(-) diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index 9f9928bfb663..72d98b30cdfe 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java @@ -2145,6 +2145,7 @@ public void cleanupStorage(boolean recurring) { } try { + cleanupSnapshotRecordsInPrimaryStorageOnly(vol); VolumeInfo volumeInfo = volFactory.getVolume(vol.getId()); if (volumeInfo != null) { volService.ensureVolumeIsExpungeReady(vol.getId()); @@ -2292,6 +2293,34 @@ private void deleteSnapshot(SnapshotVO snapshot, SnapshotDataStoreVO snapshotDat } } + /** + * Cleans up snapshot DB records for snapshots that exist only on primary storage (no secondary copy). + * This handles the case where snapshot.backup.to.secondary=false and the volume + * is being deleted during VM expunge — the RBD snapshots will be destroyed along with the image, + * so the DB records need to be cleaned up to avoid orphaned entries. + */ + protected void cleanupSnapshotRecordsInPrimaryStorageOnly(VolumeVO volume) { + List snapshots = _snapshotDao.listByVolumeId(volume.getId()); + if (CollectionUtils.isEmpty(snapshots)) { + return; + } + for (SnapshotVO snapshot : snapshots) { + if (Snapshot.State.Destroyed.equals(snapshot.getState())) { + continue; + } + List primaryRefs = _snapshotStoreDao.listBySnapshotAndDataStoreRole(snapshot.getId(), DataStoreRole.Primary); + List secondaryRefs = _snapshotStoreDao.listBySnapshotAndDataStoreRole(snapshot.getId(), DataStoreRole.Image); + if (CollectionUtils.isNotEmpty(primaryRefs) && CollectionUtils.isEmpty(secondaryRefs)) { + logger.info("Cleaning up snapshot {} (primary-only, no secondary copy) as volume {} is being deleted", snapshot, volume); + for (SnapshotDataStoreVO ref : primaryRefs) { + _snapshotStoreDao.expunge(ref.getId()); + } + snapshot.setState(Snapshot.State.Destroyed); + _snapshotDao.update(snapshot.getId(), snapshot); + } + } + } + private void removeSnapshotsInErrorStatus() { List snapshotsInErrorStatus = _snapshotDao.listAllByStatusIncludingRemoved(Snapshot.State.Error); for (SnapshotVO snapshotVO : snapshotsInErrorStatus) { diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 9131a14ee791..ca3d31d4fad3 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -1640,7 +1640,6 @@ public boolean deleteVolume(long volumeId, Account caller) throws ConcurrentOper private boolean deleteVolumeFromStorage(VolumeVO volume, Account caller) throws ConcurrentOperationException { try { - cleanupSnapshotRecordsInPrimaryStorageOnly(volume); expungeVolumesInPrimaryStorageIfNeeded(volume); expungeVolumesInSecondaryStorageIfNeeded(volume); cleanVolumesCache(volume); @@ -1651,34 +1650,6 @@ private boolean deleteVolumeFromStorage(VolumeVO volume, Account caller) throws } } - /** - * Cleans up snapshot DB records for snapshots that exist only on primary storage (no secondary copy). - * This handles the case where snapshot.backup.to.secondary=false and the volume - * is being deleted during VM expunge; the RBD snapshots will be destroyed along with the image, - * so the DB records need to be cleaned up to avoid orphaned entries. - */ - protected void cleanupSnapshotRecordsInPrimaryStorageOnly(VolumeVO volume) { - List snapshots = _snapshotDao.listByVolumeId(volume.getId()); - if (CollectionUtils.isEmpty(snapshots)) { - return; - } - for (SnapshotVO snapshot : snapshots) { - if (Snapshot.State.Destroyed.equals(snapshot.getState())) { - continue; - } - List primaryRefs = _snapshotDataStoreDao.listBySnapshotAndDataStoreRole(snapshot.getId(), DataStoreRole.Primary); - List secondaryRefs = _snapshotDataStoreDao.listBySnapshotAndDataStoreRole(snapshot.getId(), DataStoreRole.Image); - if (CollectionUtils.isNotEmpty(primaryRefs) && CollectionUtils.isEmpty(secondaryRefs)) { - logger.info("Cleaning up snapshot {} (primary-only, no secondary copy) as volume {} is being deleted", snapshot, volume); - for (SnapshotDataStoreVO ref : primaryRefs) { - _snapshotDataStoreDao.expunge(ref.getId()); - } - snapshot.setState(Snapshot.State.Destroyed); - _snapshotDao.update(snapshot.getId(), snapshot); - } - } - } - /** * Retrieves and validates the volume for the {@link #deleteVolume(long, Account)} method. The following validation are executed. *
      diff --git a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java index 8f88800d549f..1fe5b9e827b2 100644 --- a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java +++ b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java @@ -18,6 +18,7 @@ import java.lang.reflect.Field; import java.util.ArrayList; +import java.util.Collections; import java.util.Arrays; import java.util.HashMap; import java.util.List; @@ -47,6 +48,8 @@ import org.apache.cloudstack.storage.datastore.db.ObjectStoreDao; import org.apache.cloudstack.storage.datastore.db.ObjectStoreVO; import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; +import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao; +import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO; import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailVO; import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; @@ -85,6 +88,7 @@ import com.cloud.host.Host; import com.cloud.hypervisor.Hypervisor.HypervisorType; import com.cloud.hypervisor.HypervisorGuruManager; +import com.cloud.storage.dao.SnapshotDao; import com.cloud.storage.dao.VolumeDao; import com.cloud.user.AccountManagerImpl; import com.cloud.utils.Pair; @@ -129,6 +133,10 @@ public class StorageManagerImplTest { AccountManagerImpl accountMgr; @Mock StoragePoolDetailsDao storagePoolDetailsDao; + @Mock + SnapshotDao snapshotDao; + @Mock + SnapshotDataStoreDao snapshotStoreDao; @Mock ClusterDao clusterDao; @@ -1716,4 +1724,62 @@ public void testDiscoverObjectStoreInitializationFailure() { storageManagerImpl.discoverObjectStore(name, url, size, providerName, details); } + + @Test + public void testCleanupSnapshotRecordsInPrimaryStorageOnly() { + VolumeVO volume = Mockito.mock(VolumeVO.class); + Mockito.when(volume.getId()).thenReturn(1L); + + SnapshotVO snapshot = Mockito.mock(SnapshotVO.class); + Mockito.when(snapshot.getId()).thenReturn(10L); + Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.BackedUp); + Mockito.when(snapshotDao.listByVolumeId(1L)).thenReturn(List.of(snapshot)); + + SnapshotDataStoreVO primaryRef = Mockito.mock(SnapshotDataStoreVO.class); + Mockito.when(primaryRef.getId()).thenReturn(100L); + Mockito.when(snapshotStoreDao.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Primary)).thenReturn(List.of(primaryRef)); + Mockito.when(snapshotStoreDao.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Image)).thenReturn(Collections.emptyList()); + + storageManagerImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume); + + Mockito.verify(snapshotStoreDao).expunge(100L); + Mockito.verify(snapshot).setState(Snapshot.State.Destroyed); + Mockito.verify(snapshotDao).update(10L, snapshot); + } + + @Test + public void testCleanupSnapshotRecordsInPrimaryStorageOnlySkipsWhenSecondaryExists() { + VolumeVO volume = Mockito.mock(VolumeVO.class); + Mockito.when(volume.getId()).thenReturn(1L); + + SnapshotVO snapshot = Mockito.mock(SnapshotVO.class); + Mockito.when(snapshot.getId()).thenReturn(10L); + Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.BackedUp); + Mockito.when(snapshotDao.listByVolumeId(1L)).thenReturn(List.of(snapshot)); + + SnapshotDataStoreVO primaryRef = Mockito.mock(SnapshotDataStoreVO.class); + SnapshotDataStoreVO secondaryRef = Mockito.mock(SnapshotDataStoreVO.class); + Mockito.when(snapshotStoreDao.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Primary)).thenReturn(List.of(primaryRef)); + Mockito.when(snapshotStoreDao.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Image)).thenReturn(List.of(secondaryRef)); + + storageManagerImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume); + + Mockito.verify(snapshotStoreDao, Mockito.never()).expunge(Mockito.anyLong()); + Mockito.verify(snapshotDao, Mockito.never()).update(Mockito.anyLong(), Mockito.any(SnapshotVO.class)); + } + + @Test + public void testCleanupSnapshotRecordsInPrimaryStorageOnlySkipsDestroyedSnapshots() { + VolumeVO volume = Mockito.mock(VolumeVO.class); + Mockito.when(volume.getId()).thenReturn(1L); + + SnapshotVO snapshot = Mockito.mock(SnapshotVO.class); + Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.Destroyed); + Mockito.when(snapshotDao.listByVolumeId(1L)).thenReturn(List.of(snapshot)); + + storageManagerImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume); + + Mockito.verify(snapshotStoreDao, Mockito.never()).listBySnapshotAndDataStoreRole(Mockito.anyLong(), Mockito.any()); + Mockito.verify(snapshotStoreDao, Mockito.never()).expunge(Mockito.anyLong()); + } } diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index 5e10b2cb8508..c476bdd9c423 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -122,8 +122,6 @@ import com.cloud.storage.Volume.Type; import com.cloud.storage.dao.DiskOfferingDao; import com.cloud.storage.dao.SnapshotDao; -import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao; -import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO; import com.cloud.storage.dao.StoragePoolTagsDao; import com.cloud.storage.dao.VMTemplateDao; import com.cloud.storage.dao.VolumeDao; @@ -255,9 +253,6 @@ public class VolumeApiServiceImplTest { @Mock private SnapshotDao snapshotDaoMock; - @Mock - private SnapshotDataStoreDao snapshotDataStoreDaoMock; - @Mock private SnapshotPolicyDetailsDao snapshotPolicyDetailsDao; @@ -2294,62 +2289,4 @@ private List generateVmSnapshotVoList(VMSnapshot.Type t1, VMSnapsh return List.of(mock1, mock2); } - @Test - public void testCleanupSnapshotRecordsInPrimaryStorageOnly() { - VolumeVO volume = Mockito.mock(VolumeVO.class); - Mockito.when(volume.getId()).thenReturn(1L); - - SnapshotVO snapshot = Mockito.mock(SnapshotVO.class); - Mockito.when(snapshot.getId()).thenReturn(10L); - Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.BackedUp); - Mockito.when(snapshotDaoMock.listByVolumeId(1L)).thenReturn(List.of(snapshot)); - - SnapshotDataStoreVO primaryRef = Mockito.mock(SnapshotDataStoreVO.class); - Mockito.when(primaryRef.getId()).thenReturn(100L); - Mockito.when(snapshotDataStoreDaoMock.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Primary)).thenReturn(List.of(primaryRef)); - Mockito.when(snapshotDataStoreDaoMock.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Image)).thenReturn(Collections.emptyList()); - - volumeApiServiceImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume); - - Mockito.verify(snapshotDataStoreDaoMock).expunge(100L); - Mockito.verify(snapshot).setState(Snapshot.State.Destroyed); - Mockito.verify(snapshotDaoMock).update(10L, snapshot); - } - - @Test - public void testCleanupSnapshotRecordsInPrimaryStorageOnlySkipsWhenSecondaryExists() { - VolumeVO volume = Mockito.mock(VolumeVO.class); - Mockito.when(volume.getId()).thenReturn(1L); - - SnapshotVO snapshot = Mockito.mock(SnapshotVO.class); - Mockito.when(snapshot.getId()).thenReturn(10L); - Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.BackedUp); - Mockito.when(snapshotDaoMock.listByVolumeId(1L)).thenReturn(List.of(snapshot)); - - SnapshotDataStoreVO primaryRef = Mockito.mock(SnapshotDataStoreVO.class); - SnapshotDataStoreVO secondaryRef = Mockito.mock(SnapshotDataStoreVO.class); - Mockito.when(snapshotDataStoreDaoMock.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Primary)).thenReturn(List.of(primaryRef)); - Mockito.when(snapshotDataStoreDaoMock.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Image)).thenReturn(List.of(secondaryRef)); - - volumeApiServiceImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume); - - Mockito.verify(snapshotDataStoreDaoMock, Mockito.never()).expunge(Mockito.anyLong()); - Mockito.verify(snapshotDaoMock, Mockito.never()).update(Mockito.anyLong(), Mockito.any(SnapshotVO.class)); - } - - @Test - public void testCleanupSnapshotRecordsInPrimaryStorageOnlySkipsDestroyedSnapshots() { - VolumeVO volume = Mockito.mock(VolumeVO.class); - Mockito.when(volume.getId()).thenReturn(1L); - - SnapshotVO snapshot = Mockito.mock(SnapshotVO.class); - Mockito.when(snapshot.getId()).thenReturn(10L); - Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.Destroyed); - Mockito.when(snapshotDaoMock.listByVolumeId(1L)).thenReturn(List.of(snapshot)); - - volumeApiServiceImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume); - - Mockito.verify(snapshotDataStoreDaoMock, Mockito.never()).listBySnapshotAndDataStoreRole(Mockito.anyLong(), Mockito.any()); - Mockito.verify(snapshotDataStoreDaoMock, Mockito.never()).expunge(Mockito.anyLong()); - } } From 393580349f645da8e6f2869e88b2599f01836906 Mon Sep 17 00:00:00 2001 From: Damans227 <61474540+Damans227@users.noreply.github.com> Date: Fri, 13 Mar 2026 14:34:03 -0400 Subject: [PATCH 3/6] cleanup --- .../test/java/com/cloud/storage/VolumeApiServiceImplTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index c476bdd9c423..0575b430ef10 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -2288,5 +2288,4 @@ private List generateVmSnapshotVoList(VMSnapshot.Type t1, VMSnapsh Mockito.doReturn(1L).when(mock2).getId(); return List.of(mock1, mock2); } - } From 79c00ba75bf002665abef1b96d3b38a8dedb4aba Mon Sep 17 00:00:00 2001 From: Damans227 <61474540+Damans227@users.noreply.github.com> Date: Fri, 13 Mar 2026 14:41:44 -0400 Subject: [PATCH 4/6] cleanup --- .../java/com/cloud/storage/StorageManagerImpl.java | 10 +++++----- .../com/cloud/storage/StorageManagerImplTest.java | 14 +++++++------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index 72d98b30cdfe..6cd77dc524e3 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java @@ -2308,12 +2308,12 @@ protected void cleanupSnapshotRecordsInPrimaryStorageOnly(VolumeVO volume) { if (Snapshot.State.Destroyed.equals(snapshot.getState())) { continue; } - List primaryRefs = _snapshotStoreDao.listBySnapshotAndDataStoreRole(snapshot.getId(), DataStoreRole.Primary); - List secondaryRefs = _snapshotStoreDao.listBySnapshotAndDataStoreRole(snapshot.getId(), DataStoreRole.Image); - if (CollectionUtils.isNotEmpty(primaryRefs) && CollectionUtils.isEmpty(secondaryRefs)) { + List snapshotsOnPrimaryStorage = _snapshotStoreDao.listBySnapshotAndDataStoreRole(snapshot.getId(), DataStoreRole.Primary); + List snapshotsOnSecondaryStorage = _snapshotStoreDao.listBySnapshotAndDataStoreRole(snapshot.getId(), DataStoreRole.Image); + if (CollectionUtils.isNotEmpty(snapshotsOnPrimaryStorage) && CollectionUtils.isEmpty(snapshotsOnSecondaryStorage)) { logger.info("Cleaning up snapshot {} (primary-only, no secondary copy) as volume {} is being deleted", snapshot, volume); - for (SnapshotDataStoreVO ref : primaryRefs) { - _snapshotStoreDao.expunge(ref.getId()); + for (SnapshotDataStoreVO snapshotOnPrimaryStorage : snapshotsOnPrimaryStorage) { + _snapshotStoreDao.expunge(snapshotOnPrimaryStorage.getId()); } snapshot.setState(Snapshot.State.Destroyed); _snapshotDao.update(snapshot.getId(), snapshot); diff --git a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java index 1fe5b9e827b2..8fc8bf0f4fc3 100644 --- a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java +++ b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java @@ -1735,9 +1735,9 @@ public void testCleanupSnapshotRecordsInPrimaryStorageOnly() { Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.BackedUp); Mockito.when(snapshotDao.listByVolumeId(1L)).thenReturn(List.of(snapshot)); - SnapshotDataStoreVO primaryRef = Mockito.mock(SnapshotDataStoreVO.class); - Mockito.when(primaryRef.getId()).thenReturn(100L); - Mockito.when(snapshotStoreDao.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Primary)).thenReturn(List.of(primaryRef)); + SnapshotDataStoreVO snapshotOnPrimaryStorage = Mockito.mock(SnapshotDataStoreVO.class); + Mockito.when(snapshotOnPrimaryStorage.getId()).thenReturn(100L); + Mockito.when(snapshotStoreDao.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Primary)).thenReturn(List.of(snapshotOnPrimaryStorage)); Mockito.when(snapshotStoreDao.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Image)).thenReturn(Collections.emptyList()); storageManagerImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume); @@ -1757,10 +1757,10 @@ public void testCleanupSnapshotRecordsInPrimaryStorageOnlySkipsWhenSecondaryExis Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.BackedUp); Mockito.when(snapshotDao.listByVolumeId(1L)).thenReturn(List.of(snapshot)); - SnapshotDataStoreVO primaryRef = Mockito.mock(SnapshotDataStoreVO.class); - SnapshotDataStoreVO secondaryRef = Mockito.mock(SnapshotDataStoreVO.class); - Mockito.when(snapshotStoreDao.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Primary)).thenReturn(List.of(primaryRef)); - Mockito.when(snapshotStoreDao.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Image)).thenReturn(List.of(secondaryRef)); + SnapshotDataStoreVO snapshotOnPrimaryStorage = Mockito.mock(SnapshotDataStoreVO.class); + SnapshotDataStoreVO snapshotOnSecondaryStorage = Mockito.mock(SnapshotDataStoreVO.class); + Mockito.when(snapshotStoreDao.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Primary)).thenReturn(List.of(snapshotOnPrimaryStorage)); + Mockito.when(snapshotStoreDao.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Image)).thenReturn(List.of(snapshotOnSecondaryStorage)); storageManagerImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume); From 66f1e644881275e3fbd581b616e3cd555836e0d6 Mon Sep 17 00:00:00 2001 From: Daman Arora Date: Tue, 19 May 2026 21:56:40 -0400 Subject: [PATCH 5/6] Refactor snapshot cleanup logic to use SnapshotService for deletion and improve error handling --- .../com/cloud/storage/StorageManagerImpl.java | 24 +++++--- .../cloud/storage/StorageManagerImplTest.java | 56 +++++++++++++++++-- 2 files changed, 68 insertions(+), 12 deletions(-) diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index 6cd77dc524e3..15ec75f6b02a 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java @@ -2295,9 +2295,11 @@ private void deleteSnapshot(SnapshotVO snapshot, SnapshotDataStoreVO snapshotDat /** * Cleans up snapshot DB records for snapshots that exist only on primary storage (no secondary copy). - * This handles the case where snapshot.backup.to.secondary=false and the volume - * is being deleted during VM expunge — the RBD snapshots will be destroyed along with the image, - * so the DB records need to be cleaned up to avoid orphaned entries. + * This handles the case where snapshot.backup.to.secondary=false and the volume is being deleted + * during VM expunge. Each primary-only snapshot is routed through the storage driver via + * {@link #deleteSnapshot(SnapshotVO, SnapshotDataStoreVO)} so the underlying snapshot data is + * actually removed before any DB state is advanced. The parent snapshot row is only marked + * Destroyed once no live store refs remain. */ protected void cleanupSnapshotRecordsInPrimaryStorageOnly(VolumeVO volume) { List snapshots = _snapshotDao.listByVolumeId(volume.getId()); @@ -2310,13 +2312,19 @@ protected void cleanupSnapshotRecordsInPrimaryStorageOnly(VolumeVO volume) { } List snapshotsOnPrimaryStorage = _snapshotStoreDao.listBySnapshotAndDataStoreRole(snapshot.getId(), DataStoreRole.Primary); List snapshotsOnSecondaryStorage = _snapshotStoreDao.listBySnapshotAndDataStoreRole(snapshot.getId(), DataStoreRole.Image); - if (CollectionUtils.isNotEmpty(snapshotsOnPrimaryStorage) && CollectionUtils.isEmpty(snapshotsOnSecondaryStorage)) { - logger.info("Cleaning up snapshot {} (primary-only, no secondary copy) as volume {} is being deleted", snapshot, volume); - for (SnapshotDataStoreVO snapshotOnPrimaryStorage : snapshotsOnPrimaryStorage) { - _snapshotStoreDao.expunge(snapshotOnPrimaryStorage.getId()); - } + if (CollectionUtils.isEmpty(snapshotsOnPrimaryStorage) || CollectionUtils.isNotEmpty(snapshotsOnSecondaryStorage)) { + continue; + } + logger.info("Cleaning up snapshot {} (primary-only, no secondary copy) as volume {} is being deleted", snapshot, volume); + for (SnapshotDataStoreVO snapshotOnPrimaryStorage : snapshotsOnPrimaryStorage) { + deleteSnapshot(snapshot, snapshotOnPrimaryStorage); + } + List remaining = _snapshotStoreDao.findBySnapshotIdAndNotInDestroyedHiddenState(snapshot.getId()); + if (CollectionUtils.isEmpty(remaining)) { snapshot.setState(Snapshot.State.Destroyed); _snapshotDao.update(snapshot.getId(), snapshot); + } else { + logger.warn("Storage driver did not remove all primary store refs for snapshot {}; leaving parent record for the next scavenger pass to retry", snapshot); } } } diff --git a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java index 8fc8bf0f4fc3..a1b6902c7cbb 100644 --- a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java +++ b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java @@ -137,6 +137,10 @@ public class StorageManagerImplTest { SnapshotDao snapshotDao; @Mock SnapshotDataStoreDao snapshotStoreDao; + @Mock + org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory snapshotFactory; + @Mock + org.apache.cloudstack.engine.subsystem.api.storage.SnapshotService _snapshotService; @Mock ClusterDao clusterDao; @@ -1736,17 +1740,61 @@ public void testCleanupSnapshotRecordsInPrimaryStorageOnly() { Mockito.when(snapshotDao.listByVolumeId(1L)).thenReturn(List.of(snapshot)); SnapshotDataStoreVO snapshotOnPrimaryStorage = Mockito.mock(SnapshotDataStoreVO.class); - Mockito.when(snapshotOnPrimaryStorage.getId()).thenReturn(100L); + Mockito.when(snapshotOnPrimaryStorage.getSnapshotId()).thenReturn(10L); + Mockito.when(snapshotOnPrimaryStorage.getDataStoreId()).thenReturn(50L); + Mockito.when(snapshotOnPrimaryStorage.getRole()).thenReturn(DataStoreRole.Primary); Mockito.when(snapshotStoreDao.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Primary)).thenReturn(List.of(snapshotOnPrimaryStorage)); Mockito.when(snapshotStoreDao.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Image)).thenReturn(Collections.emptyList()); + org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo snapshotInfo = + Mockito.mock(org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo.class); + DataStore dataStore = Mockito.mock(DataStore.class); + Mockito.when(dataStore.getUuid()).thenReturn("ds-uuid"); + Mockito.when(snapshotInfo.getDataStore()).thenReturn(dataStore); + Mockito.when(snapshotFactory.getSnapshotIncludingRemoved(10L, 50L, DataStoreRole.Primary)).thenReturn(snapshotInfo); + Mockito.when(_snapshotService.deleteSnapshot(snapshotInfo)).thenReturn(true); + Mockito.when(snapshotStoreDao.findBySnapshotIdAndNotInDestroyedHiddenState(10L)).thenReturn(Collections.emptyList()); + storageManagerImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume); - Mockito.verify(snapshotStoreDao).expunge(100L); + Mockito.verify(_snapshotService).deleteSnapshot(snapshotInfo); Mockito.verify(snapshot).setState(Snapshot.State.Destroyed); Mockito.verify(snapshotDao).update(10L, snapshot); } + @Test + public void testCleanupSnapshotRecordsInPrimaryStorageOnlyLeavesParentWhenStorageDeleteFails() { + VolumeVO volume = Mockito.mock(VolumeVO.class); + Mockito.when(volume.getId()).thenReturn(1L); + + SnapshotVO snapshot = Mockito.mock(SnapshotVO.class); + Mockito.when(snapshot.getId()).thenReturn(10L); + Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.BackedUp); + Mockito.when(snapshotDao.listByVolumeId(1L)).thenReturn(List.of(snapshot)); + + SnapshotDataStoreVO snapshotOnPrimaryStorage = Mockito.mock(SnapshotDataStoreVO.class); + Mockito.when(snapshotOnPrimaryStorage.getSnapshotId()).thenReturn(10L); + Mockito.when(snapshotOnPrimaryStorage.getDataStoreId()).thenReturn(50L); + Mockito.when(snapshotOnPrimaryStorage.getRole()).thenReturn(DataStoreRole.Primary); + Mockito.when(snapshotStoreDao.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Primary)).thenReturn(List.of(snapshotOnPrimaryStorage)); + Mockito.when(snapshotStoreDao.listBySnapshotAndDataStoreRole(10L, DataStoreRole.Image)).thenReturn(Collections.emptyList()); + + org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo snapshotInfo = + Mockito.mock(org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo.class); + DataStore dataStore = Mockito.mock(DataStore.class); + Mockito.when(dataStore.getUuid()).thenReturn("ds-uuid"); + Mockito.when(snapshotInfo.getDataStore()).thenReturn(dataStore); + Mockito.when(snapshotFactory.getSnapshotIncludingRemoved(10L, 50L, DataStoreRole.Primary)).thenReturn(snapshotInfo); + Mockito.when(_snapshotService.deleteSnapshot(snapshotInfo)).thenReturn(false); + Mockito.when(snapshotStoreDao.findBySnapshotIdAndNotInDestroyedHiddenState(10L)).thenReturn(List.of(snapshotOnPrimaryStorage)); + + storageManagerImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume); + + Mockito.verify(_snapshotService).deleteSnapshot(snapshotInfo); + Mockito.verify(snapshot, Mockito.never()).setState(Snapshot.State.Destroyed); + Mockito.verify(snapshotDao, Mockito.never()).update(Mockito.anyLong(), Mockito.any(SnapshotVO.class)); + } + @Test public void testCleanupSnapshotRecordsInPrimaryStorageOnlySkipsWhenSecondaryExists() { VolumeVO volume = Mockito.mock(VolumeVO.class); @@ -1764,7 +1812,7 @@ public void testCleanupSnapshotRecordsInPrimaryStorageOnlySkipsWhenSecondaryExis storageManagerImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume); - Mockito.verify(snapshotStoreDao, Mockito.never()).expunge(Mockito.anyLong()); + Mockito.verifyNoInteractions(_snapshotService); Mockito.verify(snapshotDao, Mockito.never()).update(Mockito.anyLong(), Mockito.any(SnapshotVO.class)); } @@ -1780,6 +1828,6 @@ public void testCleanupSnapshotRecordsInPrimaryStorageOnlySkipsDestroyedSnapshot storageManagerImpl.cleanupSnapshotRecordsInPrimaryStorageOnly(volume); Mockito.verify(snapshotStoreDao, Mockito.never()).listBySnapshotAndDataStoreRole(Mockito.anyLong(), Mockito.any()); - Mockito.verify(snapshotStoreDao, Mockito.never()).expunge(Mockito.anyLong()); + Mockito.verifyNoInteractions(_snapshotService); } } From 28548b2c4f92ee40dc9319df31bcd48a2279917d Mon Sep 17 00:00:00 2001 From: Daman Arora Date: Tue, 19 May 2026 22:05:53 -0400 Subject: [PATCH 6/6] Enhance snapshot cleanup process by updating state, managing resource limits, and removing annotations --- .../java/com/cloud/storage/StorageManagerImpl.java | 5 +++++ .../com/cloud/storage/StorageManagerImplTest.java | 11 +++++++++++ 2 files changed, 16 insertions(+) diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java index 15ec75f6b02a..d43940800e03 100644 --- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java @@ -2321,8 +2321,13 @@ protected void cleanupSnapshotRecordsInPrimaryStorageOnly(VolumeVO volume) { } List remaining = _snapshotStoreDao.findBySnapshotIdAndNotInDestroyedHiddenState(snapshot.getId()); if (CollectionUtils.isEmpty(remaining)) { + Snapshot.State previousState = snapshot.getState(); snapshot.setState(Snapshot.State.Destroyed); _snapshotDao.update(snapshot.getId(), snapshot); + annotationDao.removeByEntityType(AnnotationService.EntityType.SNAPSHOT.name(), snapshot.getUuid()); + if (previousState != Snapshot.State.Error && previousState != Snapshot.State.Destroyed) { + _resourceLimitMgr.decrementResourceCount(snapshot.getAccountId(), ResourceType.snapshot); + } } else { logger.warn("Storage driver did not remove all primary store refs for snapshot {}; leaving parent record for the next scavenger pass to retry", snapshot); } diff --git a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java index a1b6902c7cbb..4bdc2ff067f7 100644 --- a/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java +++ b/server/src/test/java/com/cloud/storage/StorageManagerImplTest.java @@ -141,6 +141,10 @@ public class StorageManagerImplTest { org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory snapshotFactory; @Mock org.apache.cloudstack.engine.subsystem.api.storage.SnapshotService _snapshotService; + @Mock + com.cloud.user.ResourceLimitService _resourceLimitMgr; + @Mock + org.apache.cloudstack.annotation.dao.AnnotationDao annotationDao; @Mock ClusterDao clusterDao; @@ -1736,6 +1740,8 @@ public void testCleanupSnapshotRecordsInPrimaryStorageOnly() { SnapshotVO snapshot = Mockito.mock(SnapshotVO.class); Mockito.when(snapshot.getId()).thenReturn(10L); + Mockito.when(snapshot.getAccountId()).thenReturn(7L); + Mockito.when(snapshot.getUuid()).thenReturn("snap-uuid"); Mockito.when(snapshot.getState()).thenReturn(Snapshot.State.BackedUp); Mockito.when(snapshotDao.listByVolumeId(1L)).thenReturn(List.of(snapshot)); @@ -1760,6 +1766,9 @@ public void testCleanupSnapshotRecordsInPrimaryStorageOnly() { Mockito.verify(_snapshotService).deleteSnapshot(snapshotInfo); Mockito.verify(snapshot).setState(Snapshot.State.Destroyed); Mockito.verify(snapshotDao).update(10L, snapshot); + Mockito.verify(_resourceLimitMgr).decrementResourceCount(7L, com.cloud.configuration.Resource.ResourceType.snapshot); + Mockito.verify(annotationDao).removeByEntityType( + org.apache.cloudstack.annotation.AnnotationService.EntityType.SNAPSHOT.name(), "snap-uuid"); } @Test @@ -1793,6 +1802,8 @@ public void testCleanupSnapshotRecordsInPrimaryStorageOnlyLeavesParentWhenStorag Mockito.verify(_snapshotService).deleteSnapshot(snapshotInfo); Mockito.verify(snapshot, Mockito.never()).setState(Snapshot.State.Destroyed); Mockito.verify(snapshotDao, Mockito.never()).update(Mockito.anyLong(), Mockito.any(SnapshotVO.class)); + Mockito.verifyNoInteractions(_resourceLimitMgr); + Mockito.verifyNoInteractions(annotationDao); } @Test