From 63010c970a13a84896ffbac3cbf4cf58477b856c Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Thu, 28 May 2026 11:05:04 +0200 Subject: [PATCH 1/2] check state instead of power state on change offering for ROOT volume --- .../com/cloud/storage/VolumeApiServiceImpl.java | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 687e432f166b..20d92d743209 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -2316,9 +2316,11 @@ private VolumeVO resizeVolumeInternal(VolumeVO volume, DiskOfferingVO newDiskOff HypervisorType hypervisorType = _volsDao.getHypervisorType(volume.getId()); if (userVm != null) { - if (volume.getVolumeType().equals(Volume.Type.ROOT) && userVm.getPowerState() != VirtualMachine.PowerState.PowerOff && hypervisorType == HypervisorType.VMware) { - logger.error(" For ROOT volume resize VM should be in Power Off state."); - throw new InvalidParameterValueException("VM current state is : " + userVm.getPowerState() + ". But VM should be in " + VirtualMachine.PowerState.PowerOff + " state."); + if (Volume.Type.ROOT.equals(volume.getVolumeType()) + && ! State.Stopped.equals(userVm.getState()) + && HypervisorType.VMware.equals(hypervisorType)) { + logger.error("For ROOT volume resize VM should be in Stopped state."); + throw new InvalidParameterValueException("VM current state is : " + userVm.getState() + ". But VM should be in " + State.Stopped + " state."); } // serialize VM operation AsyncJobExecutionContext jobContext = AsyncJobExecutionContext.getCurrentExecutionContext(); @@ -2396,9 +2398,11 @@ private void validateVolumeReadyStateAndHypervisorChecks(VolumeVO volume, long c UserVmVO userVm = _userVmDao.findById(volume.getInstanceId()); if (userVm != null) { - if (volume.getVolumeType().equals(Volume.Type.ROOT) && userVm.getPowerState() != VirtualMachine.PowerState.PowerOff && hypervisorType == HypervisorType.VMware) { - logger.error(" For ROOT volume resize VM should be in Power Off state."); - throw new InvalidParameterValueException("VM current state is : " + userVm.getPowerState() + ". But VM should be in " + VirtualMachine.PowerState.PowerOff + " state."); + if (Volume.Type.ROOT.equals(volume.getVolumeType()) + && !State.Stopped.equals(userVm.getState()) + && hypervisorType == HypervisorType.VMware) { + logger.error("For ROOT volume resize VM should be in Stopped state."); + throw new InvalidParameterValueException("VM current state is : " + userVm.getState() + ". But VM should be in " + State.Stopped + " state."); } } } From 69def684274d42e3c151bacbad7a82b1a4f0b3c4 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Thu, 28 May 2026 15:03:43 +0200 Subject: [PATCH 2/2] tests --- .../cloud/storage/VolumeApiServiceImpl.java | 6 +- .../storage/VolumeApiServiceImplTest.java | 232 +++++++++++++++++- 2 files changed, 226 insertions(+), 12 deletions(-) diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 20d92d743209..1dd108633caa 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -2317,10 +2317,10 @@ private VolumeVO resizeVolumeInternal(VolumeVO volume, DiskOfferingVO newDiskOff if (userVm != null) { if (Volume.Type.ROOT.equals(volume.getVolumeType()) - && ! State.Stopped.equals(userVm.getState()) + && !State.Stopped.equals(userVm.getState()) && HypervisorType.VMware.equals(hypervisorType)) { logger.error("For ROOT volume resize VM should be in Stopped state."); - throw new InvalidParameterValueException("VM current state is : " + userVm.getState() + ". But VM should be in " + State.Stopped + " state."); + throw new InvalidParameterValueException("The current VM state is '" + userVm.getState() + "'. But the VM should be in " + State.Stopped + " state."); } // serialize VM operation AsyncJobExecutionContext jobContext = AsyncJobExecutionContext.getCurrentExecutionContext(); @@ -2402,7 +2402,7 @@ private void validateVolumeReadyStateAndHypervisorChecks(VolumeVO volume, long c && !State.Stopped.equals(userVm.getState()) && hypervisorType == HypervisorType.VMware) { logger.error("For ROOT volume resize VM should be in Stopped state."); - throw new InvalidParameterValueException("VM current state is : " + userVm.getState() + ". But VM should be in " + State.Stopped + " state."); + throw new InvalidParameterValueException("The current VM state is '" + userVm.getState() + "'. But VM should be in " + State.Stopped + " state."); } } } diff --git a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java index 77e321ab8591..241efd6652a5 100644 --- a/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java +++ b/server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java @@ -33,6 +33,8 @@ import static org.mockito.Mockito.when; import java.lang.reflect.Field; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -40,15 +42,6 @@ import java.util.UUID; import java.util.concurrent.ExecutionException; -import com.cloud.event.EventTypes; -import com.cloud.event.UsageEventUtils; -import com.cloud.host.HostVO; -import com.cloud.resourcelimit.CheckedReservation; -import com.cloud.service.ServiceOfferingVO; -import com.cloud.service.dao.ServiceOfferingDao; -import com.cloud.vm.snapshot.VMSnapshot; -import com.cloud.vm.snapshot.VMSnapshotDetailsVO; -import com.cloud.vm.snapshot.dao.VMSnapshotDetailsDao; import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.acl.SecurityChecker.AccessType; import org.apache.cloudstack.api.command.user.volume.CheckAndRepairVolumeCmd; @@ -107,17 +100,23 @@ import com.cloud.dc.dao.ClusterDao; import com.cloud.dc.dao.DataCenterDao; import com.cloud.dc.dao.HostPodDao; +import com.cloud.event.EventTypes; +import com.cloud.event.UsageEventUtils; import com.cloud.exception.InvalidParameterValueException; import com.cloud.exception.PermissionDeniedException; import com.cloud.exception.ResourceAllocationException; +import com.cloud.host.HostVO; import com.cloud.host.dao.HostDao; import com.cloud.hypervisor.Hypervisor.HypervisorType; import com.cloud.org.Grouping; import com.cloud.projects.Project; import com.cloud.projects.ProjectManager; +import com.cloud.resourcelimit.CheckedReservation; import com.cloud.serializer.GsonHelper; import com.cloud.server.ManagementService; import com.cloud.server.TaggedResourceService; +import com.cloud.service.ServiceOfferingVO; +import com.cloud.service.dao.ServiceOfferingDao; import com.cloud.storage.Storage.ProvisioningType; import com.cloud.storage.Volume.Type; import com.cloud.storage.dao.DiskOfferingDao; @@ -145,8 +144,11 @@ import com.cloud.vm.VirtualMachineManager; import com.cloud.vm.dao.UserVmDao; import com.cloud.vm.dao.VMInstanceDao; +import com.cloud.vm.snapshot.VMSnapshot; +import com.cloud.vm.snapshot.VMSnapshotDetailsVO; import com.cloud.vm.snapshot.VMSnapshotVO; import com.cloud.vm.snapshot.dao.VMSnapshotDao; +import com.cloud.vm.snapshot.dao.VMSnapshotDetailsDao; @RunWith(MockitoJUnitRunner.class) public class VolumeApiServiceImplTest { @@ -2320,4 +2322,216 @@ private List generateVmSnapshotVoList(VMSnapshot.Type t1, VMSnapsh Mockito.doReturn(1L).when(mock2).getId(); return List.of(mock1, mock2); } + +// ===================================================================== +// VMware ROOT-volume resize / offering-change: VM power_state-lag tests +// +// Both private guards that protect VMware ROOT-volume resize operations +// are covered here: +// • validateVolumeReadyStateAndHypervisorChecks (called by changeDiskOfferingForVolumeInternal) +// • resizeVolumeInternal (called by both resize and change-offering flows) +// +// The "power_state lag" scenario: when a VMware VM is stopped via CloudStack +// the VirtualMachine.State transitions to Stopped immediately, but the +// VMware-side VirtualMachine.PowerState (polled from ESX) may still read +// PoweredOn for some seconds. The production code must use only the +// authoritative CloudStack State field and must NOT additionally gate on +// PowerState. +// ===================================================================== + + /** + * Positive – validateVolumeReadyStateAndHypervisorChecks: + * The guard must allow a VMware ROOT-volume resize when the CloudStack VM + * state is {@code Stopped}, regardless of the VMware power_state value. + * getPowerState() is intentionally left un-stubbed so that any invocation + * of that method would cause a Mockito strict-stubbing error and surface a + * regression. + */ + @Test + public void testValidateVolumeReadyStateVMware_VMStopped_PowerStateLag_ShouldPass() + throws NoSuchMethodException, InvocationTargetException, IllegalAccessException { + + long volumeId = 200L; + long vmId = 201L; + + VolumeVO volume = Mockito.mock(VolumeVO.class); + when(volume.getId()).thenReturn(volumeId); + when(volume.getInstanceId()).thenReturn(vmId); + when(volume.getVolumeType()).thenReturn(Volume.Type.ROOT); + when(volume.getState()).thenReturn(Volume.State.Ready); + + // snapshotDaoMock returns an empty list by default (Mockito default behaviour) + when(volumeDaoMock.getHypervisorType(volumeId)).thenReturn(HypervisorType.VMware); + + UserVmVO stoppedVm = Mockito.mock(UserVmVO.class); + // Authoritative cloud state: Stopped. + // getPowerState() is NOT stubbed – power_state lag scenario. + when(stoppedVm.getState()).thenReturn(State.Stopped); + when(userVmDaoMock.findById(vmId)).thenReturn(stoppedVm); + + long currentSizeBytes = 10L << 30; // 10 GiB + Long newSizeBytes = 20L << 30; // 20 GiB (grow; VMware prohibits shrink) + + Method method = VolumeApiServiceImpl.class.getDeclaredMethod( + "validateVolumeReadyStateAndHypervisorChecks", + VolumeVO.class, long.class, Long.class); + method.setAccessible(true); + + // Must complete without throwing any exception + method.invoke(volumeApiServiceImpl, volume, currentSizeBytes, newSizeBytes); + } + + /** + * Negative – validateVolumeReadyStateAndHypervisorChecks: + * The guard must reject a VMware ROOT-volume resize when the CloudStack VM + * state is {@code Running}. + */ + @Test + public void testValidateVolumeReadyStateVMware_VMRunning_ShouldThrowInvalidParameterValueException() + throws NoSuchMethodException, IllegalAccessException { + + long volumeId = 200L; + long vmId = 201L; + + VolumeVO volume = Mockito.mock(VolumeVO.class); + when(volume.getId()).thenReturn(volumeId); + when(volume.getInstanceId()).thenReturn(vmId); + when(volume.getVolumeType()).thenReturn(Volume.Type.ROOT); + when(volume.getState()).thenReturn(Volume.State.Ready); + + when(volumeDaoMock.getHypervisorType(volumeId)).thenReturn(HypervisorType.VMware); + + UserVmVO runningVm = Mockito.mock(UserVmVO.class); + when(runningVm.getState()).thenReturn(State.Running); + when(userVmDaoMock.findById(vmId)).thenReturn(runningVm); + + long currentSizeBytes = 10L << 30; + Long newSizeBytes = 20L << 30; + + Method method = VolumeApiServiceImpl.class.getDeclaredMethod( + "validateVolumeReadyStateAndHypervisorChecks", + VolumeVO.class, long.class, Long.class); + method.setAccessible(true); + + try { + method.invoke(volumeApiServiceImpl, volume, currentSizeBytes, newSizeBytes); + Assert.fail("Expected InvalidParameterValueException for VMware ROOT-volume resize " + + "when VM state is Running"); + } catch (InvocationTargetException e) { + Assert.assertNotNull("InvocationTargetException must carry a cause", e.getCause()); + Assert.assertTrue( + "Cause must be InvalidParameterValueException, was: " + e.getCause().getClass(), + e.getCause() instanceof InvalidParameterValueException); + Assert.assertTrue( + "Exception message must reference Stopped-state requirement, was: " + e.getCause().getMessage(), + e.getCause().getMessage() != null + && e.getCause().getMessage().contains("VM should be in")); + } + } + + /** + * Positive – resizeVolumeInternal: + * The VMware stopped-state guard inside {@code resizeVolumeInternal} must NOT + * fire when the CloudStack VM state is {@code Stopped}, even when the VMware + * power_state has not yet transitioned to PowerOff. + * Any exception originating from deeper plumbing (job queue, storage layer) + * is acceptable; only the state-guard exception is a failure. + */ + @Test + public void testResizeVolumeInternal_VMware_VMStopped_PowerStateLag_ShouldNotThrowStateGuardError() + throws NoSuchMethodException, IllegalAccessException { + + long volumeId = 300L; + long vmId = 301L; + + VolumeVO volume = Mockito.mock(VolumeVO.class); + when(volume.getId()).thenReturn(volumeId); + when(volume.getInstanceId()).thenReturn(vmId); + when(volume.getVolumeType()).thenReturn(Volume.Type.ROOT); + + UserVmVO stoppedVm = Mockito.mock(UserVmVO.class); + when(stoppedVm.getState()).thenReturn(State.Stopped); // authoritative cloud state + // getPowerState() deliberately NOT stubbed – power_state lag scenario + when(userVmDaoMock.findById(vmId)).thenReturn(stoppedVm); + + when(volumeDaoMock.getHypervisorType(volumeId)).thenReturn(HypervisorType.VMware); + + // resizeVolumeInternal(VolumeVO, DiskOfferingVO, Long, Long, Long, Long, Integer, boolean) + Method method = VolumeApiServiceImpl.class.getDeclaredMethod( + "resizeVolumeInternal", + VolumeVO.class, DiskOfferingVO.class, + Long.class, Long.class, Long.class, Long.class, Integer.class, boolean.class); + method.setAccessible(true); + + try { + method.invoke(volumeApiServiceImpl, + volume, + /* newDiskOffering */ (DiskOfferingVO) null, + /* currentSize */ 0L, + /* newSize */ 1L, + /* newMinIops */ (Long) null, + /* newMaxIops */ (Long) null, + /* snapshotReserve */ (Integer) null, + /* shrinkOk */ false); + } catch (InvocationTargetException e) { + // If the state guard triggered it is a test failure; all other deeper + // exceptions (NullPointerException from job-queue plumbing, etc.) are + // acceptable because they are outside the scope of this test. + if (e.getCause() instanceof InvalidParameterValueException + && e.getCause().getMessage() != null + && e.getCause().getMessage().contains("VM should be in")) { + Assert.fail("VMware ROOT-volume resize must be allowed when CloudStack VM state is " + + "Stopped, even under a power_state lag. Unexpected exception: " + + e.getCause().getMessage()); + } + } + } + + /** + * Negative – resizeVolumeInternal: + * The VMware stopped-state guard inside {@code resizeVolumeInternal} must + * reject the operation when the CloudStack VM state is {@code Running}. + */ + @Test + public void testResizeVolumeInternal_VMware_VMRunning_ShouldThrowStateGuardError() + throws NoSuchMethodException, IllegalAccessException { + + long volumeId = 300L; + long vmId = 301L; + + VolumeVO volume = Mockito.mock(VolumeVO.class); + when(volume.getId()).thenReturn(volumeId); + when(volume.getInstanceId()).thenReturn(vmId); + when(volume.getVolumeType()).thenReturn(Volume.Type.ROOT); + + UserVmVO runningVm = Mockito.mock(UserVmVO.class); + when(runningVm.getState()).thenReturn(State.Running); + when(userVmDaoMock.findById(vmId)).thenReturn(runningVm); + + when(volumeDaoMock.getHypervisorType(volumeId)).thenReturn(HypervisorType.VMware); + + Method method = VolumeApiServiceImpl.class.getDeclaredMethod( + "resizeVolumeInternal", + VolumeVO.class, DiskOfferingVO.class, + Long.class, Long.class, Long.class, Long.class, Integer.class, boolean.class); + method.setAccessible(true); + + try { + method.invoke(volumeApiServiceImpl, + volume, + (DiskOfferingVO) null, + 0L, 1L, (Long) null, (Long) null, (Integer) null, false); + Assert.fail("Expected InvalidParameterValueException for VMware ROOT-volume resize " + + "when VM state is Running"); + } catch (InvocationTargetException e) { + Assert.assertNotNull("InvocationTargetException must carry a cause", e.getCause()); + Assert.assertTrue( + "Cause must be InvalidParameterValueException, was: " + e.getCause().getClass(), + e.getCause() instanceof InvalidParameterValueException); + Assert.assertTrue( + "Exception message must reference Stopped-state requirement, was: " + e.getCause().getMessage(), + e.getCause().getMessage() != null + && e.getCause().getMessage().contains("VM should be in")); + } + } }