Skip to content

[Enhancement] Cleanup up leftover volumes on Allocated volumes deletion #14497

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
nvazquez wants to merge 5 commits into
base: main
Choose a base branch
from
+93 −34
Draft

[Enhancement] Cleanup up leftover volumes on Allocated volumes deletion #14497

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2d61f60
Add logic for allocated volumes to go through expunge flow
Feb 5, 2026
b1e0ebb
Fix unit test
Mar 2, 2026
d01a728
addressed comments
Sep 18, 2026
a76263f
logic for allocated + no poolID to be removed directly
Oct 1, 2026
0efb0d9
Fixes
nvazquez Oct 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions ...stration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2565,15 +2565,7 @@ public void destroyVolume(Volume volume) {
String volumeToString = getReflectOnlySelectedFields(volume);

try {
// Mark volume as removed if volume has not been created on primary
if (volume.getState() == Volume.State.Allocated) {
_volsDao.remove(volume.getId());
stateTransitTo(volume, Volume.Event.DestroyRequested);
_resourceLimitMgr.decrementVolumeResourceCount(volume.getAccountId(), volume.isDisplay(), volume.getSize(), diskOfferingDao.findByIdIncludingRemoved(volume.getDiskOfferingId()),
null);
} else {
destroyVolumeInContext(volume);
}
destroyVolumeInContext(volume);
// FIXME - All this is boiler plate code and should be done as part of state transition. This shouldn't be part of orchestrator.
// publish usage event for the volume
UsageEventUtils.publishUsageEvent(EventTypes.EVENT_VOLUME_DELETE, volume.getAccountId(), volume.getDataCenterId(), volume.getId(), volume.getName(), Volume.class.getName(),
Expand Down
45 changes: 27 additions & 18 deletions server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic

private final StateMachine2<Volume.State, Volume.Event, Volume> _volStateMachine;

private static final Set<Volume.State> STATES_VOLUME_CANNOT_BE_DESTROYED = new HashSet<>(Arrays.asList(Volume.State.Destroy, Volume.State.Expunging, Volume.State.Expunged, Volume.State.Allocated));
private static final Set<Volume.State> STATES_VOLUME_CANNOT_BE_DESTROYED = new HashSet<>(Arrays.asList(Volume.State.Destroy, Volume.State.Expunging, Volume.State.Expunged));
private static final long GiB_TO_BYTES = 1024 * 1024 * 1024;

private static final String CUSTOM_DISK_OFFERING_UNIQUE_NAME = "Cloud.com-Custom";
Expand Down Expand Up @@ -1741,8 +1741,9 @@ private VolumeVO orchestrateResizeVolume(long volumeId, long currentSize, long n
@Override
@ActionEvent(eventType = EventTypes.EVENT_VOLUME_DELETE, eventDescription = "deleting volume")
/**
* Executes the removal of the volume. If the volume is only allocated we do not try to remove it from primary and secondary storage.
* Otherwise, after the removal in the database, we will try to remove the volume from both primary and secondary storage.
* Executes the removal of the volume. After the removal in the database, we will try to remove the volume from both primary and secondary storage.
* Volumes in the {@link Volume.State#Allocated} state that have a backing storage pool are routed through the same destroy/expunge flow so their
* backend storage is removed; those without a pool are soft-deleted directly.
*/
public boolean deleteVolume(long volumeId, Account caller) throws ConcurrentOperationException {
Volume volume = destroyVolume(volumeId, caller, true, true, null);
Expand Down Expand Up @@ -1810,7 +1811,6 @@ protected VolumeVO retrieveAndValidateVolume(long volumeId, Account caller) {
* <li> {code}Volume.State#Destroy{code};
* <li> {code}Volume.State#Expunging{code};
* <li> {code}Volume.State#Expunged{code};
* <li> {code}Volume.State#Allocated{code}.
* </ul>
*
* The volume is destroyed via {@link VolumeService#destroyVolume(long)} method.
Expand Down Expand Up @@ -1899,6 +1899,21 @@ private void removeVolume(long volumeId) {
}
}

/**
* Directly soft-deletes an {@link Volume.State#Allocated} volume that has no backing storage pool and releases its
* resource count. Volumes with poolID are left to the normal destroy/expunge flow
* @return true when the volume was handled here and no further destroy processing is required.
*/
protected boolean removeAllocatedVolumeWithoutStorage(VolumeVO volume, Boolean enforceResourceLimitOnDisplayFalse) {
if (volume.getState() != Volume.State.Allocated || volume.getPoolId() != null) {
return false;
}
removeVolume(volume.getId());
_resourceLimitMgr.decrementVolumeResourceCount(volume.getAccountId(), volume.isDisplay(),
volume.getSize(), _diskOfferingDao.findByIdIncludingRemoved(volume.getDiskOfferingId()), enforceResourceLimitOnDisplayFalse);
return true;
}

public boolean stateTransitTo(Volume vol, Volume.Event event) throws NoTransitionException {
return _volStateMachine.transitTo(vol, event, null, _volsDao);
}
Expand Down Expand Up @@ -1928,23 +1943,13 @@ public Volume destroyVolume(long volumeId, Account caller, boolean expunge, bool

validateDestroyVolume(volume, caller, expunge, forceExpunge);

if (expunge && removeAllocatedVolumeWithoutStorage(volume, enforceResourceLimitOnDisplayFalse)) {
return volume;
}

destroyVolumeIfPossible(volume);

if (expunge) {
// Mark volume as removed if volume has not been created on primary or secondary
if (volume.getState() == Volume.State.Allocated) {
_volsDao.remove(volume.getId());
try {
stateTransitTo(volume, Volume.Event.DestroyRequested);
stateTransitTo(volume, Volume.Event.OperationSucceeded);
} catch (NoTransitionException e) {
logger.debug("Failed to destroy volume {}", volume, e);
return null;
}
_resourceLimitMgr.decrementVolumeResourceCount(volume.getAccountId(), volume.isDisplay(),
volume.getSize(), _diskOfferingDao.findByIdIncludingRemoved(volume.getDiskOfferingId()), enforceResourceLimitOnDisplayFalse);
return volume;
}
if (!deleteVolumeFromStorage(volume, caller)) {
logger.warn("Failed to expunge volume: {}", volume);
return null;
Expand All @@ -1958,6 +1963,10 @@ public Volume destroyVolume(long volumeId, Account caller, boolean expunge, bool
@Override
@ActionEvent(eventType = EventTypes.EVENT_VOLUME_DESTROY, eventDescription = "destroying a volume")
public void destroyVolume(long volumeId) {
VolumeVO volume = _volsDao.findById(volumeId);
if (volume != null && removeAllocatedVolumeWithoutStorage(volume, null)) {
return;
}
volService.destroyVolume(volumeId);
}

Expand Down
72 changes: 65 additions & 7 deletions server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1042,6 +1042,16 @@ public void destroyVolumeIfPossibleTestVolumeStateReady() {
Mockito.verify(volumeServiceMock, Mockito.times(1)).destroyVolume(volumeMockId);
}

@Test
public void destroyVolumeIfPossibleTestVolumeStateAllocated() {
Mockito.doReturn(Volume.State.Allocated).when(volumeVoMock).getState();
configureMocksForTestDestroyVolumeWhenVolume();

volumeApiServiceImpl.destroyVolumeIfPossible(volumeVoMock);

Mockito.verify(volumeServiceMock, Mockito.times(1)).destroyVolume(volumeMockId);
}

private void verifyMocksForTestDestroyVolumeWhenVolumeIsNotInRightState() {
Mockito.verify(volumeServiceMock, Mockito.times(0)).destroyVolume(volumeMockId);
Mockito.verify(resourceLimitServiceMock, Mockito.times(0)).decrementVolumeResourceCount(accountMockId, true, volumeSizeMock, newDiskOfferingMock, null);
Expand Down Expand Up @@ -1187,21 +1197,69 @@ public void deleteVolumeTestVolumeStateAllocated() throws InterruptedException,
Mockito.lenient().doNothing().when(volumeApiServiceImpl).expungeVolumesInPrimaryStorageIfNeeded(volumeVoMock);
Mockito.lenient().doNothing().when(volumeApiServiceImpl).expungeVolumesInSecondaryStorageIfNeeded(volumeVoMock);
Mockito.lenient().doNothing().when(volumeApiServiceImpl).cleanVolumesCache(volumeVoMock);

Mockito.doReturn(true).when(volumeDaoMock).remove(volumeMockId);
Mockito.doReturn(true).when(volumeApiServiceImpl).stateTransitTo(volumeVoMock, Volume.Event.DestroyRequested);

boolean result = volumeApiServiceImpl.deleteVolume(volumeMockId, accountMock);

Assert.assertTrue(result);
Mockito.verify(volumeApiServiceImpl).retrieveAndValidateVolume(volumeMockId, accountMock);
Mockito.verify(volumeApiServiceImpl).destroyVolumeIfPossible(volumeVoMock);
Mockito.verify(volumeDaoMock).remove(volumeMockId);
Mockito.verify(volumeApiServiceImpl).stateTransitTo(volumeVoMock, Volume.Event.DestroyRequested);
Mockito.verify(volumeDaoMock, Mockito.times(0)).remove(volumeMockId);
Mockito.verify(volumeApiServiceImpl, Mockito.times(0)).stateTransitTo(volumeVoMock, Volume.Event.DestroyRequested);

Mockito.verify(volumeApiServiceImpl, Mockito.times(1)).expungeVolumesInPrimaryStorageIfNeeded(volumeVoMock);
Mockito.verify(volumeApiServiceImpl, Mockito.times(1)).expungeVolumesInSecondaryStorageIfNeeded(volumeVoMock);
Mockito.verify(volumeApiServiceImpl, Mockito.times(1)).cleanVolumesCache(volumeVoMock);
}

@Test
public void deleteVolumeTestVolumeStateAllocatedWithoutStorage() throws InterruptedException, ExecutionException, NoTransitionException {
Mockito.doReturn(Volume.State.Allocated).when(volumeVoMock).getState();
Mockito.doReturn(null).when(volumeVoMock).getPoolId();
Mockito.doReturn(accountMockId).when(volumeVoMock).getAccountId();
Mockito.doReturn(true).when(volumeVoMock).isDisplay();
Mockito.doReturn(diskOfferingMockId).when(volumeVoMock).getDiskOfferingId();
Mockito.doReturn(newDiskOfferingMock).when(_diskOfferingDao).findByIdIncludingRemoved(diskOfferingMockId);

Mockito.doReturn(volumeVoMock).when(volumeApiServiceImpl).retrieveAndValidateVolume(volumeMockId, accountMock);
Mockito.doReturn(volumeVoMock).when(volumeDaoMock).findById(volumeMockId);
Mockito.doReturn(true).when(volumeDaoMock).remove(volumeMockId);

boolean result = volumeApiServiceImpl.deleteVolume(volumeMockId, accountMock);

Assert.assertTrue(result);
Mockito.verify(volumeDaoMock, Mockito.times(1)).remove(volumeMockId);
Mockito.verify(resourceLimitServiceMock, Mockito.times(1)).decrementVolumeResourceCount(accountMockId, true, volumeSizeMock, newDiskOfferingMock, null);
Mockito.verify(volumeApiServiceImpl, Mockito.times(0)).destroyVolumeIfPossible(volumeVoMock);
Mockito.verify(volumeApiServiceImpl, Mockito.times(0)).expungeVolumesInPrimaryStorageIfNeeded(volumeVoMock);
Mockito.verify(volumeApiServiceImpl, Mockito.times(0)).expungeVolumesInSecondaryStorageIfNeeded(volumeVoMock);
Mockito.verify(volumeApiServiceImpl, Mockito.times(0)).cleanVolumesCache(volumeVoMock);
}

@Test
public void destroyVolumeByIdTestAllocatedWithoutStorageIsRemovedDirectly() {
Mockito.doReturn(Volume.State.Allocated).when(volumeVoMock).getState();
Mockito.doReturn(null).when(volumeVoMock).getPoolId();
Mockito.doReturn(accountMockId).when(volumeVoMock).getAccountId();
Mockito.doReturn(true).when(volumeVoMock).isDisplay();
Mockito.doReturn(diskOfferingMockId).when(volumeVoMock).getDiskOfferingId();
Mockito.doReturn(newDiskOfferingMock).when(_diskOfferingDao).findByIdIncludingRemoved(diskOfferingMockId);
Mockito.doReturn(volumeVoMock).when(volumeDaoMock).findById(volumeMockId);
Mockito.doReturn(true).when(volumeDaoMock).remove(volumeMockId);

volumeApiServiceImpl.destroyVolume(volumeMockId);

Mockito.verify(volumeDaoMock, Mockito.times(1)).remove(volumeMockId);
Mockito.verify(resourceLimitServiceMock, Mockito.times(1)).decrementVolumeResourceCount(accountMockId, true, volumeSizeMock, newDiskOfferingMock, null);
Mockito.verify(volumeServiceMock, Mockito.times(0)).destroyVolume(volumeMockId);
}

@Test
public void destroyVolumeByIdTestAllocatedWithStorageGoesThroughStorageFlow() {
Mockito.doReturn(Volume.State.Allocated).when(volumeVoMock).getState();
Mockito.doReturn(volumeVoMock).when(volumeDaoMock).findById(volumeMockId);

volumeApiServiceImpl.destroyVolume(volumeMockId);

Mockito.verify(volumeServiceMock, Mockito.times(1)).destroyVolume(volumeMockId);
Mockito.verify(volumeDaoMock, Mockito.times(0)).remove(volumeMockId);
}

@Test
Expand Down
Loading