Clean up primary-only snapshot DB records during volume deletion to prevent orphaned entries

Author: Damans227

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<SnapshotVO> snapshots = _snapshotDao.listByVolumeId(volume.getId());
+        if (CollectionUtils.isEmpty(snapshots)) {
+            return;
+        }
+        for (SnapshotVO snapshot : snapshots) {
+            if (Snapshot.State.Destroyed.equals(snapshot.getState())) {
+                continue;
+            }
+            List<SnapshotDataStoreVO> primaryRefs = _snapshotDataStoreDao.listBySnapshotAndDataStoreRole(snapshot.getId(), DataStoreRole.Primary);
+            List<SnapshotDataStoreVO> 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.
      *  <ul>

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<VMSnapshotVO> 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());
+    }
 }