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..893efd074a23 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"; @@ -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) { *
  • {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. @@ -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); } diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index 12258e2cc160..07b07b5abc99 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); @@ -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