From 2d61f60c4e223a29c29b965170fc49af101e6135 Mon Sep 17 00:00:00 2001 From: anishadas Date: Thu, 5 Feb 2026 14:18:59 -0800 Subject: [PATCH 1/5] Add logic for allocated volumes to go through expunge flow --- .../orchestration/VolumeOrchestrator.java | 10 +--------- .../com/cloud/storage/VolumeApiServiceImpl.java | 17 +---------------- 2 files changed, 2 insertions(+), 25 deletions(-) diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java index 2b9b41b6e8ac..a8f67631c4fd 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java @@ -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(), diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 8674014addd7..2ce5731caa68 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -429,7 +429,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic private final StateMachine2 _volStateMachine; - private static final Set 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 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"; @@ -1810,7 +1810,6 @@ protected VolumeVO retrieveAndValidateVolume(long volumeId, Account caller) { *
  • {code}Volume.State#Destroy{code}; *
  • {code}Volume.State#Expunging{code}; *
  • {code}Volume.State#Expunged{code}; - *
  • {code}Volume.State#Allocated{code}. * * * The volume is destroyed via {@link VolumeService#destroyVolume(long)} method. @@ -1931,20 +1930,6 @@ public Volume destroyVolume(long volumeId, Account caller, boolean expunge, bool 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; From b1e0ebbb2816bd86ed02f216ae7457e930fe1a03 Mon Sep 17 00:00:00 2001 From: anishadas Date: Mon, 2 Mar 2026 14:33:16 -0800 Subject: [PATCH 2/5] Fix unit test --- .../com/cloud/storage/VolumeApiServiceImplTest.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index 12258e2cc160..2e9ccf17a69e 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -1196,12 +1196,12 @@ public void deleteVolumeTestVolumeStateAllocated() throws InterruptedException, 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(0)).expungeVolumesInPrimaryStorageIfNeeded(volumeVoMock); - Mockito.verify(volumeApiServiceImpl, Mockito.times(0)).expungeVolumesInSecondaryStorageIfNeeded(volumeVoMock); - Mockito.verify(volumeApiServiceImpl, Mockito.times(0)).cleanVolumesCache(volumeVoMock); + 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 From d01a728463d67cf212dcce1765c75c01becffdb1 Mon Sep 17 00:00:00 2001 From: anishadas Date: Thu, 17 Sep 2026 17:13:37 -0700 Subject: [PATCH 3/5] addressed comments --- .../java/com/cloud/storage/VolumeApiServiceImpl.java | 4 ++-- .../com/cloud/storage/VolumeApiServiceImplTest.java | 10 ++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 2ce5731caa68..9aa3421c03ab 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -1741,8 +1741,8 @@ 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 are routed through the same destroy/expunge flow; storage cleanup is skipped only when the volume has no backing storage pool */ public boolean deleteVolume(long volumeId, Account caller) throws ConcurrentOperationException { Volume volume = destroyVolume(volumeId, caller, true, true, null); diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index 2e9ccf17a69e..fa18cca16c27 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -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); From a76263f5989fa1b8c312d3f5310785b113bbb9b9 Mon Sep 17 00:00:00 2001 From: anishadas Date: Thu, 1 Oct 2026 15:03:27 -0700 Subject: [PATCH 4/5] logic for allocated + no poolID to be removed directly --- .../cloud/storage/VolumeApiServiceImpl.java | 26 +++++++++- .../storage/VolumeApiServiceImplTest.java | 52 +++++++++++++++++++ 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 9aa3421c03ab..893efd074a23 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -1742,7 +1742,8 @@ private VolumeVO orchestrateResizeVolume(long volumeId, long currentSize, long n @ActionEvent(eventType = EventTypes.EVENT_VOLUME_DELETE, eventDescription = "deleting volume") /** * 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 are routed through the same destroy/expunge flow; storage cleanup is skipped only when the volume has no backing storage pool + * 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); @@ -1898,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); } @@ -1927,6 +1943,10 @@ 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) { @@ -1943,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); } diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index fa18cca16c27..b90809992c3d 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -1214,6 +1214,58 @@ public void deleteVolumeTestVolumeStateAllocated() throws InterruptedException, 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); + Mockito.verify(volumeApiServiceImpl, Mockito.times(0)).destroyVolumeIfPossible(volumeVoMock); + Mockito.verify(volumeApiServiceImpl, Mockito.times(0)).expungeVolumesInPrimaryStorageIfNeeded(volumeVoMock); + Mockito.verify(volumeApiServiceImpl, Mockito.times(0)).expungeVolumesInSecondaryStorageIfNeeded(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); + 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 public void deleteVolumeTestVolumeStateReady() throws InterruptedException, ExecutionException, NoTransitionException { Mockito.doReturn(Volume.State.Ready).when(volumeVoMock).getState(); From 0efb0d970816266942923cac4ed7d49e32998110 Mon Sep 17 00:00:00 2001 From: nvazquez Date: Sat, 24 Oct 2026 21:43:50 -0300 Subject: [PATCH 5/5] Fixes --- .../java/com/cloud/storage/VolumeApiServiceImplTest.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index b90809992c3d..07b07b5abc99 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -1197,10 +1197,6 @@ 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); @@ -1231,7 +1227,7 @@ public void deleteVolumeTestVolumeStateAllocatedWithoutStorage() throws Interrup Assert.assertTrue(result); Mockito.verify(volumeDaoMock, Mockito.times(1)).remove(volumeMockId); - Mockito.verify(resourceLimitServiceMock, Mockito.times(1)).decrementVolumeResourceCount(accountMockId, true, volumeSizeMock, newDiskOfferingMock); + 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); @@ -1251,7 +1247,7 @@ public void destroyVolumeByIdTestAllocatedWithoutStorageIsRemovedDirectly() { volumeApiServiceImpl.destroyVolume(volumeMockId); Mockito.verify(volumeDaoMock, Mockito.times(1)).remove(volumeMockId); - Mockito.verify(resourceLimitServiceMock, Mockito.times(1)).decrementVolumeResourceCount(accountMockId, true, volumeSizeMock, newDiskOfferingMock); + Mockito.verify(resourceLimitServiceMock, Mockito.times(1)).decrementVolumeResourceCount(accountMockId, true, volumeSizeMock, newDiskOfferingMock, null); Mockito.verify(volumeServiceMock, Mockito.times(0)).destroyVolume(volumeMockId); }