winterhazel commented on code in PR #13271:
URL: https://github.com/apache/cloudstack/pull/13271#discussion_r3508735768
##########
server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java:
##########
@@ -2320,4 +2322,216 @@ private List<VMSnapshotVO>
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",
Review Comment:
@DaanHoogland also, can you make the method protected instead of doing this?
##########
server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java:
##########
@@ -2320,4 +2322,216 @@ private List<VMSnapshotVO>
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(
Review Comment:
Same thing here
##########
server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java:
##########
@@ -2320,4 +2322,216 @@ private List<VMSnapshotVO>
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(
Review Comment:
And here
##########
server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java:
##########
@@ -2320,4 +2322,216 @@ private List<VMSnapshotVO>
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(
Review Comment:
Here as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
