etan39
/
cloudstack
mirror of https://github.com/apache/cloudstack.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix delete snapshot policy expunged volume (#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>
This commit is contained in:
Daman Arora 2026-01-28 09:11:14 -05:00 committed by GitHub
parent 059debf212
commit 9956d32548
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 112 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -387,6 +387,7 @@ public class VolumeServiceImpl implements VolumeService {
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 class VolumeServiceImpl implements VolumeService {
}
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 class VolumeServiceImpl implements VolumeService {
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 class VolumeServiceImpl implements VolumeService {
// 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);

18
server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
View File

@ -1889,9 +1889,25 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement
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) {
VolumeVO volume = _volsDao.findByIdIncludingRemoved(policy.getVolumeId());
if (volume == null || volume.getState() == Volume.State.Expunged) {
logger.info("Removing orphan snapshot policy {} for non-existent volume {}", policy.getId(), policy.getVolumeId());
deletePolicy(policy.getId());
}
}
}
@Override
public boolean stop() {
backupSnapshotExecutor.shutdown();
@ -1924,7 +1940,7 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement
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");
}

92
server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerImplTest.java
View File

@ -30,6 +30,7 @@ import com.cloud.storage.Snapshot;
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.Pair;
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 class SnapshotManagerImplTest {
snapshotManager._snapshotPolicyDao = snapshotPolicyDao;
snapshotManager._volsDao = volumeDao;
snapshotManager._accountMgr = accountManager;
snapshotManager._snapSchedMgr = snapshotScheduler;
snapshotManager.taggedResourceService = taggedResourceService;
}
@After
@ -520,4 +528,88 @@ public class SnapshotManagerImplTest {
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);
}
}