Fix delete snapshot policy expunged volume

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

@@ -387,6 +387,7 @@ public AsyncCallFuture<VolumeApiResult> expungeVolumeAsync(VolumeInfo volume) {
             logger.info("Expunge volume with no data store specified");
             if (canVolumeBeRemoved(volume.getId())) {
                 logger.info("Volume {} is not referred anywhere, remove it from volumes table", volume);
+                snapshotMgr.deletePoliciesForVolume(volume.getId());
                 volDao.remove(volume.getId());
             }
             future.complete(result);
@@ -422,6 +423,7 @@ public AsyncCallFuture<VolumeApiResult> expungeVolumeAsync(VolumeInfo volume) {
                 }
                 VMTemplateVO template = templateDao.findById(vol.getTemplateId());
                 if (template != null && !template.isDeployAsIs()) {
+                    snapshotMgr.deletePoliciesForVolume(vol.getId());
                     volDao.remove(vol.getId());
                     future.complete(result);
                     return future;
@@ -493,6 +495,7 @@ public Void deleteVolumeCallback(AsyncCallbackDispatcher<VolumeServiceImpl, Comm
 
                 if (canVolumeBeRemoved(vo.getId())) {
                     logger.info("Volume {} is not referred anywhere, remove it from volumes table", vo);
+                    snapshotMgr.deletePoliciesForVolume(vo.getId());
                     volDao.remove(vo.getId());
                 }
 
@@ -1657,7 +1660,6 @@ public void destroyVolume(long volumeId) {
         // mark volume entry in volumes table as destroy state
         VolumeInfo vol = volFactory.getVolume(volumeId);
         vol.stateTransit(Volume.Event.DestroyRequested);
-        snapshotMgr.deletePoliciesForVolume(volumeId);
         annotationDao.removeByEntityType(AnnotationService.EntityType.VOLUME.name(), vol.getUuid());
 
         vol.stateTransit(Volume.Event.OperationSucceeded);

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

@@ -1890,9 +1890,24 @@ public boolean start() {
                 logger.debug("Failed to delete snapshot in destroying state: {}", snapshotVO);
             }
         }
+        cleanupOrphanSnapshotPolicies();
+
         return true;
     }
 
+    private void cleanupOrphanSnapshotPolicies() {
+        List<SnapshotPolicyVO> policies = _snapshotPolicyDao.listActivePolicies();
+        if (CollectionUtils.isEmpty(policies)) {
+            return;
+        }
+        for (SnapshotPolicyVO policy : policies) {
+            if (_volsDao.findByIdIncludingRemoved(policy.getVolumeId()) == null) {
+                logger.info("Removing orphan snapshot policy {} for non-existent volume {}", policy.getId(), policy.getVolumeId());
+                deletePolicy(policy.getId());
+            }
+        }
+    }
+
     @Override
     public boolean stop() {
         backupSnapshotExecutor.shutdown();
@@ -1925,7 +1940,7 @@ public boolean deleteSnapshotPolicies(DeleteSnapshotPoliciesCmd cmd) {
             if (snapshotPolicyVO == null) {
                 throw new InvalidParameterValueException("Policy id given: " + policy + " does not exist");
             }
-            VolumeVO volume = _volsDao.findById(snapshotPolicyVO.getVolumeId());
+            VolumeVO volume = _volsDao.findByIdIncludingRemoved(snapshotPolicyVO.getVolumeId());
             if (volume == null) {
                 throw new InvalidParameterValueException("Policy id given: " + policy + " does not belong to a valid volume");
             }

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

@@ -30,6 +30,7 @@
 import com.cloud.storage.SnapshotPolicyVO;
 import com.cloud.storage.SnapshotVO;
 import com.cloud.storage.VolumeVO;
+import com.cloud.server.TaggedResourceService;
 import com.cloud.storage.dao.SnapshotDao;
 import com.cloud.storage.dao.SnapshotPolicyDao;
 import com.cloud.storage.dao.SnapshotZoneDao;
@@ -44,6 +45,7 @@
 
 import com.cloud.utils.db.SearchBuilder;
 import com.cloud.utils.db.SearchCriteria;
+import org.apache.cloudstack.api.command.user.snapshot.DeleteSnapshotPoliciesCmd;
 import org.apache.cloudstack.api.command.user.snapshot.ListSnapshotPoliciesCmd;
 import org.apache.cloudstack.context.CallContext;
 import org.apache.cloudstack.engine.subsystem.api.storage.CreateCmdResult;
@@ -100,6 +102,10 @@ public class SnapshotManagerImplTest {
     VolumeDao volumeDao;
     @Mock
     SnapshotPolicyDao snapshotPolicyDao;
+    @Mock
+    SnapshotScheduler snapshotScheduler;
+    @Mock
+    TaggedResourceService taggedResourceService;
     @InjectMocks
     SnapshotManagerImpl snapshotManager = new SnapshotManagerImpl();
 
@@ -108,6 +114,8 @@ public void setUp() {
         snapshotManager._snapshotPolicyDao = snapshotPolicyDao;
         snapshotManager._volsDao = volumeDao;
         snapshotManager._accountMgr = accountManager;
+        snapshotManager._snapSchedMgr = snapshotScheduler;
+        snapshotManager.taggedResourceService = taggedResourceService;
     }
 
     @After
@@ -520,4 +528,88 @@ public void testListSnapshotPolicies_RootAdmin() {
         Assert.assertEquals(1, result.first().size());
         Assert.assertEquals(Integer.valueOf(1), result.second());
     }
+
+    @Test
+    public void testDeleteSnapshotPoliciesForRemovedVolume() {
+        Long policyId = 1L;
+        Long volumeId = 10L;
+        Long accountId = 2L;
+
+        DeleteSnapshotPoliciesCmd cmd = Mockito.mock(DeleteSnapshotPoliciesCmd.class);
+        Mockito.when(cmd.getId()).thenReturn(policyId);
+        Mockito.when(cmd.getIds()).thenReturn(null);
+
+        Account caller = Mockito.mock(Account.class);
+        Mockito.when(caller.getId()).thenReturn(accountId);
+        CallContext.register(Mockito.mock(User.class), caller);
+
+        SnapshotPolicyVO policyVO = Mockito.mock(SnapshotPolicyVO.class);
+        Mockito.when(policyVO.getId()).thenReturn(policyId);
+        Mockito.when(policyVO.getVolumeId()).thenReturn(volumeId);
+        Mockito.when(policyVO.getUuid()).thenReturn("policy-uuid");
+        Mockito.when(snapshotPolicyDao.findById(policyId)).thenReturn(policyVO);
+
+        // Volume is removed (expunged) but findByIdIncludingRemoved should still return it
+        VolumeVO volumeVO = Mockito.mock(VolumeVO.class);
+        Mockito.when(volumeDao.findByIdIncludingRemoved(volumeId)).thenReturn(volumeVO);
+
+        Mockito.when(snapshotPolicyDao.remove(policyId)).thenReturn(true);
+
+        boolean result = snapshotManager.deleteSnapshotPolicies(cmd);
+
+        Assert.assertTrue(result);
+        Mockito.verify(volumeDao).findByIdIncludingRemoved(volumeId);
+        Mockito.verify(snapshotScheduler).removeSchedule(volumeId, policyId);
+        Mockito.verify(snapshotPolicyDao).remove(policyId);
+    }
+
+    @Test(expected = InvalidParameterValueException.class)
+    public void testDeleteSnapshotPoliciesNoPolicyId() {
+        DeleteSnapshotPoliciesCmd cmd = Mockito.mock(DeleteSnapshotPoliciesCmd.class);
+        Mockito.when(cmd.getId()).thenReturn(null);
+        Mockito.when(cmd.getIds()).thenReturn(null);
+
+        snapshotManager.deleteSnapshotPolicies(cmd);
+    }
+
+    @Test(expected = InvalidParameterValueException.class)
+    public void testDeleteSnapshotPoliciesPolicyNotFound() {
+        Long policyId = 1L;
+
+        DeleteSnapshotPoliciesCmd cmd = Mockito.mock(DeleteSnapshotPoliciesCmd.class);
+        Mockito.when(cmd.getId()).thenReturn(policyId);
+        Mockito.when(cmd.getIds()).thenReturn(null);
+
+        Mockito.when(snapshotPolicyDao.findById(policyId)).thenReturn(null);
+
+        snapshotManager.deleteSnapshotPolicies(cmd);
+    }
+
+    @Test(expected = InvalidParameterValueException.class)
+    public void testDeleteSnapshotPoliciesVolumeNotFound() {
+        Long policyId = 1L;
+        Long volumeId = 10L;
+
+        DeleteSnapshotPoliciesCmd cmd = Mockito.mock(DeleteSnapshotPoliciesCmd.class);
+        Mockito.when(cmd.getId()).thenReturn(policyId);
+        Mockito.when(cmd.getIds()).thenReturn(null);
+
+        SnapshotPolicyVO policyVO = Mockito.mock(SnapshotPolicyVO.class);
+        Mockito.when(policyVO.getVolumeId()).thenReturn(volumeId);
+        Mockito.when(snapshotPolicyDao.findById(policyId)).thenReturn(policyVO);
+
+        // Volume doesn't exist at all (even when including removed)
+        Mockito.when(volumeDao.findByIdIncludingRemoved(volumeId)).thenReturn(null);
+
+        snapshotManager.deleteSnapshotPolicies(cmd);
+    }
+
+    @Test(expected = InvalidParameterValueException.class)
+    public void testDeleteSnapshotPoliciesManualPolicyId() {
+        DeleteSnapshotPoliciesCmd cmd = Mockito.mock(DeleteSnapshotPoliciesCmd.class);
+        Mockito.when(cmd.getId()).thenReturn(Snapshot.MANUAL_POLICY_ID);
+        Mockito.when(cmd.getIds()).thenReturn(null);
+
+        snapshotManager.deleteSnapshotPolicies(cmd);
+    }
 }