Skip to content

Commit f4e0b79

Browse files
Damans227Daman Arora
authored and
dhslove
committed
Fix delete snapshot policy expunged volume (apache#12474)
* use findByIdIncludingRemoved for volume retrieval in snapshot policy validation * add unit tests * add cleanup for orphan snapshot policies * delete snapshot policies when expunging volumes * update orphan cleanup to remove policies for volumes that are in expunged state or null --------- Co-authored-by: Daman Arora <daman.arora@shapeblue.com>
1 parent 1acd240 commit f4e0b79

File tree

3 files changed

+112
-2
lines changed

3 files changed

+112
-2
lines changed

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -397,6 +397,7 @@ public AsyncCallFuture<VolumeApiResult> expungeVolumeAsync(VolumeInfo volume) {
397397
logger.info("Expunge volume with no data store specified");
398398
if (canVolumeBeRemoved(volume.getId())) {
399399
logger.info("Volume {} is not referred anywhere, remove it from volumes table", volume);
400+
snapshotMgr.deletePoliciesForVolume(volume.getId());
400401
volDao.remove(volume.getId());
401402
}
402403
future.complete(result);
@@ -432,6 +433,7 @@ public AsyncCallFuture<VolumeApiResult> expungeVolumeAsync(VolumeInfo volume) {
432433
}
433434
VMTemplateVO template = templateDao.findById(vol.getTemplateId());
434435
if (template != null && !template.isDeployAsIs()) {
436+
snapshotMgr.deletePoliciesForVolume(vol.getId());
435437
volDao.remove(vol.getId());
436438
future.complete(result);
437439
return future;
@@ -503,6 +505,7 @@ public Void deleteVolumeCallback(AsyncCallbackDispatcher<VolumeServiceImpl, Comm
503505

504506
if (canVolumeBeRemoved(vo.getId())) {
505507
logger.info("Volume {} is not referred anywhere, remove it from volumes table", vo);
508+
snapshotMgr.deletePoliciesForVolume(vo.getId());
506509
volDao.remove(vo.getId());
507510
}
508511

@@ -1668,7 +1671,6 @@ public void destroyVolume(long volumeId) {
16681671
// mark volume entry in volumes table as destroy state
16691672
VolumeInfo vol = volFactory.getVolume(volumeId);
16701673
vol.stateTransit(Volume.Event.DestroyRequested);
1671-
snapshotMgr.deletePoliciesForVolume(volumeId);
16721674
annotationDao.removeByEntityType(AnnotationService.EntityType.VOLUME.name(), vol.getUuid());
16731675

16741676
vol.stateTransit(Volume.Event.OperationSucceeded);

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

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1936,9 +1936,25 @@ public boolean start() {
19361936
logger.debug("Failed to delete snapshot in destroying state: {}", snapshotVO);
19371937
}
19381938
}
1939+
cleanupOrphanSnapshotPolicies();
1940+
19391941
return true;
19401942
}
19411943

1944+
private void cleanupOrphanSnapshotPolicies() {
1945+
List<SnapshotPolicyVO> policies = _snapshotPolicyDao.listActivePolicies();
1946+
if (CollectionUtils.isEmpty(policies)) {
1947+
return;
1948+
}
1949+
for (SnapshotPolicyVO policy : policies) {
1950+
VolumeVO volume = _volsDao.findByIdIncludingRemoved(policy.getVolumeId());
1951+
if (volume == null || volume.getState() == Volume.State.Expunged) {
1952+
logger.info("Removing orphan snapshot policy {} for non-existent volume {}", policy.getId(), policy.getVolumeId());
1953+
deletePolicy(policy.getId());
1954+
}
1955+
}
1956+
}
1957+
19421958
@Override
19431959
public boolean stop() {
19441960
backupSnapshotExecutor.shutdown();
@@ -1971,7 +1987,7 @@ public boolean deleteSnapshotPolicies(DeleteSnapshotPoliciesCmd cmd) {
19711987
if (snapshotPolicyVO == null) {
19721988
throw new InvalidParameterValueException("Policy id given: " + policy + " does not exist");
19731989
}
1974-
VolumeVO volume = _volsDao.findById(snapshotPolicyVO.getVolumeId());
1990+
VolumeVO volume = _volsDao.findByIdIncludingRemoved(snapshotPolicyVO.getVolumeId());
19751991
if (volume == null) {
19761992
throw new InvalidParameterValueException("Policy id given: " + policy + " does not belong to a valid volume");
19771993
}

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

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import com.cloud.storage.SnapshotPolicyVO;
3131
import com.cloud.storage.SnapshotVO;
3232
import com.cloud.storage.VolumeVO;
33+
import com.cloud.server.TaggedResourceService;
3334
import com.cloud.storage.dao.SnapshotDao;
3435
import com.cloud.storage.dao.SnapshotPolicyDao;
3536
import com.cloud.storage.dao.SnapshotZoneDao;
@@ -44,6 +45,7 @@
4445

4546
import com.cloud.utils.db.SearchBuilder;
4647
import com.cloud.utils.db.SearchCriteria;
48+
import org.apache.cloudstack.api.command.user.snapshot.DeleteSnapshotPoliciesCmd;
4749
import org.apache.cloudstack.api.command.user.snapshot.ListSnapshotPoliciesCmd;
4850
import org.apache.cloudstack.context.CallContext;
4951
import org.apache.cloudstack.engine.subsystem.api.storage.CreateCmdResult;
@@ -100,6 +102,10 @@ public class SnapshotManagerImplTest {
100102
VolumeDao volumeDao;
101103
@Mock
102104
SnapshotPolicyDao snapshotPolicyDao;
105+
@Mock
106+
SnapshotScheduler snapshotScheduler;
107+
@Mock
108+
TaggedResourceService taggedResourceService;
103109
@InjectMocks
104110
SnapshotManagerImpl snapshotManager = new SnapshotManagerImpl();
105111

@@ -108,6 +114,8 @@ public void setUp() {
108114
snapshotManager._snapshotPolicyDao = snapshotPolicyDao;
109115
snapshotManager._volsDao = volumeDao;
110116
snapshotManager._accountMgr = accountManager;
117+
snapshotManager._snapSchedMgr = snapshotScheduler;
118+
snapshotManager.taggedResourceService = taggedResourceService;
111119
}
112120

113121
@After
@@ -520,4 +528,88 @@ public void testListSnapshotPolicies_RootAdmin() {
520528
Assert.assertEquals(1, result.first().size());
521529
Assert.assertEquals(Integer.valueOf(1), result.second());
522530
}
531+
532+
@Test
533+
public void testDeleteSnapshotPoliciesForRemovedVolume() {
534+
Long policyId = 1L;
535+
Long volumeId = 10L;
536+
Long accountId = 2L;
537+
538+
DeleteSnapshotPoliciesCmd cmd = Mockito.mock(DeleteSnapshotPoliciesCmd.class);
539+
Mockito.when(cmd.getId()).thenReturn(policyId);
540+
Mockito.when(cmd.getIds()).thenReturn(null);
541+
542+
Account caller = Mockito.mock(Account.class);
543+
Mockito.when(caller.getId()).thenReturn(accountId);
544+
CallContext.register(Mockito.mock(User.class), caller);
545+
546+
SnapshotPolicyVO policyVO = Mockito.mock(SnapshotPolicyVO.class);
547+
Mockito.when(policyVO.getId()).thenReturn(policyId);
548+
Mockito.when(policyVO.getVolumeId()).thenReturn(volumeId);
549+
Mockito.when(policyVO.getUuid()).thenReturn("policy-uuid");
550+
Mockito.when(snapshotPolicyDao.findById(policyId)).thenReturn(policyVO);
551+
552+
// Volume is removed (expunged) but findByIdIncludingRemoved should still return it
553+
VolumeVO volumeVO = Mockito.mock(VolumeVO.class);
554+
Mockito.when(volumeDao.findByIdIncludingRemoved(volumeId)).thenReturn(volumeVO);
555+
556+
Mockito.when(snapshotPolicyDao.remove(policyId)).thenReturn(true);
557+
558+
boolean result = snapshotManager.deleteSnapshotPolicies(cmd);
559+
560+
Assert.assertTrue(result);
561+
Mockito.verify(volumeDao).findByIdIncludingRemoved(volumeId);
562+
Mockito.verify(snapshotScheduler).removeSchedule(volumeId, policyId);
563+
Mockito.verify(snapshotPolicyDao).remove(policyId);
564+
}
565+
566+
@Test(expected = InvalidParameterValueException.class)
567+
public void testDeleteSnapshotPoliciesNoPolicyId() {
568+
DeleteSnapshotPoliciesCmd cmd = Mockito.mock(DeleteSnapshotPoliciesCmd.class);
569+
Mockito.when(cmd.getId()).thenReturn(null);
570+
Mockito.when(cmd.getIds()).thenReturn(null);
571+
572+
snapshotManager.deleteSnapshotPolicies(cmd);
573+
}
574+
575+
@Test(expected = InvalidParameterValueException.class)
576+
public void testDeleteSnapshotPoliciesPolicyNotFound() {
577+
Long policyId = 1L;
578+
579+
DeleteSnapshotPoliciesCmd cmd = Mockito.mock(DeleteSnapshotPoliciesCmd.class);
580+
Mockito.when(cmd.getId()).thenReturn(policyId);
581+
Mockito.when(cmd.getIds()).thenReturn(null);
582+
583+
Mockito.when(snapshotPolicyDao.findById(policyId)).thenReturn(null);
584+
585+
snapshotManager.deleteSnapshotPolicies(cmd);
586+
}
587+
588+
@Test(expected = InvalidParameterValueException.class)
589+
public void testDeleteSnapshotPoliciesVolumeNotFound() {
590+
Long policyId = 1L;
591+
Long volumeId = 10L;
592+
593+
DeleteSnapshotPoliciesCmd cmd = Mockito.mock(DeleteSnapshotPoliciesCmd.class);
594+
Mockito.when(cmd.getId()).thenReturn(policyId);
595+
Mockito.when(cmd.getIds()).thenReturn(null);
596+
597+
SnapshotPolicyVO policyVO = Mockito.mock(SnapshotPolicyVO.class);
598+
Mockito.when(policyVO.getVolumeId()).thenReturn(volumeId);
599+
Mockito.when(snapshotPolicyDao.findById(policyId)).thenReturn(policyVO);
600+
601+
// Volume doesn't exist at all (even when including removed)
602+
Mockito.when(volumeDao.findByIdIncludingRemoved(volumeId)).thenReturn(null);
603+
604+
snapshotManager.deleteSnapshotPolicies(cmd);
605+
}
606+
607+
@Test(expected = InvalidParameterValueException.class)
608+
public void testDeleteSnapshotPoliciesManualPolicyId() {
609+
DeleteSnapshotPoliciesCmd cmd = Mockito.mock(DeleteSnapshotPoliciesCmd.class);
610+
Mockito.when(cmd.getId()).thenReturn(Snapshot.MANUAL_POLICY_ID);
611+
Mockito.when(cmd.getIds()).thenReturn(null);
612+
613+
snapshotManager.deleteSnapshotPolicies(cmd);
614+
}
523615
}

0 commit comments

Comments
 (0)