Copilot commented on code in PR #13271:
URL: https://github.com/apache/cloudstack/pull/13271#discussion_r3520492729
##########
server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java:
##########
@@ -2320,4 +2322,186 @@ 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)
+
+ // Must complete without throwing any exception
+
volumeApiServiceImpl.validateVolumeReadyStateAndHypervisorChecks(volume,
currentSizeBytes, newSizeBytes);
+ }
+
+ /**
+ * Negative – validateVolumeReadyStateAndHypervisorChecks:
+ * The guard must reject a VMware ROOT-volume resize when the CloudStack VM
+ * state is {@code Running}.
+ */
+ @Test(expected = InvalidParameterValueException.class)
+ 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);
+
Review Comment:
This test also calls `validateVolumeReadyStateAndHypervisorChecks`, which
queries `snapshotDaoMock.listByStatus(...)`. Without stubbing, the mock returns
null and the test will NPE before reaching the expected VMware/Running-state
validation.
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -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.VMware.equals(hypervisorType)) {
+ logger.error("For ROOT volume resize VM should be in Stopped
state.");
+ throw new InvalidParameterValueException("The current VM state
is '" + userVm.getState() + "'. But VM should be in " + State.Stopped + "
state.");
Review Comment:
The updated exception text is slightly inconsistent with the one in
`resizeVolumeInternal` ("But the VM should be…" vs "But VM should be…").
Keeping them consistent makes log triage and tests easier.
##########
server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java:
##########
@@ -2320,4 +2322,186 @@ 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);
+
Review Comment:
`validateVolumeReadyStateAndHypervisorChecks` calls
`snapshotDaoMock.listByStatus(...)`; with a Mockito `@Mock` this returns null
by default, so the test will NPE on `ongoingSnapshots.size()`. Stub
`listByStatus` to return an empty list so the test actually exercises the
VMware state guard.
##########
server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java:
##########
@@ -2320,4 +2322,186 @@ 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)
+
+ // Must complete without throwing any exception
+
volumeApiServiceImpl.validateVolumeReadyStateAndHypervisorChecks(volume,
currentSizeBytes, newSizeBytes);
+ }
Review Comment:
To make the “power_state lag” regression check explicit, add a `verify(...,
never())` for `getPowerState()` after the call. This will fail if a future
refactor reintroduces a `getPowerState()` dependency that doesn’t necessarily
throw.
--
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]
