[Enhancement] Cleanup up leftover volumes on Allocated volumes deletion

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(),

server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java

@@ -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";
@@ -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);
@@ -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.
@@ -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);
     }
@@ -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;
@@ -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);
     }
 

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);
@@ -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