Skip to content

check state instead of power state on change offering for ROOT volume #13271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
DaanHoogland wants to merge 5 commits into
base: 4.22
Choose a base branch
from
+206 −18
Open

check state instead of power state on change offering for ROOT volume #13271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
63010c9
check state instead of power state on change offering for ROOT volume
May 28, 2026
69def68
tests
May 28, 2026
902a8c9
Apply suggestions from code review
DaanHoogland Jul 2, 2026
8eff83a
Apply test by package private
DaanHoogland Jul 2, 2026
4f36ed6
fixing tests
Jul 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2311,14 +2311,16 @@ private boolean compareEqualsIncludingNullOrZero(Long a, Long b) {
return a.equals(b);
}

private VolumeVO resizeVolumeInternal(VolumeVO volume, DiskOfferingVO newDiskOffering, Long currentSize, Long newSize, Long newMinIops, Long newMaxIops, Integer newHypervisorSnapshotReserve, boolean shrinkOk) throws ResourceAllocationException {
VolumeVO resizeVolumeInternal(VolumeVO volume, DiskOfferingVO newDiskOffering, Long currentSize, Long newSize, Long newMinIops, Long newMaxIops, Integer newHypervisorSnapshotReserve, boolean shrinkOk) throws ResourceAllocationException {
UserVmVO userVm = _userVmDao.findById(volume.getInstanceId());
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("The current VM state is '" + userVm.getState() + "'. But the VM should be in " + State.Stopped + " state.");
}
Comment thread
DaanHoogland marked this conversation as resolved.
// serialize VM operation
AsyncJobExecutionContext jobContext = AsyncJobExecutionContext.getCurrentExecutionContext();
Expand Down Expand Up @@ -2372,7 +2374,7 @@ private VolumeVO resizeVolumeInternal(VolumeVO volume, DiskOfferingVO newDiskOff
shrinkOk);
}

private void validateVolumeReadyStateAndHypervisorChecks(VolumeVO volume, long currentSize, Long newSize) {
void validateVolumeReadyStateAndHypervisorChecks(VolumeVO volume, long currentSize, Long newSize) {
// checking if there are any ongoing snapshots on the volume which is to be resized
List<SnapshotVO> ongoingSnapshots = _snapshotDao.listByStatus(volume.getId(), Snapshot.State.Creating, Snapshot.State.CreatedOnPrimary, Snapshot.State.BackingUp);
if (ongoingSnapshots.size() > 0) {
Expand All @@ -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.");
}
}
}
Expand All @@ -2415,7 +2419,7 @@ private void setNewIopsLimits(VolumeVO volume, DiskOfferingVO newDiskOffering, L
}
}

private void validateVolumeResizeWithNewDiskOfferingAndLoad(VolumeVO volume, DiskOfferingVO existingDiskOffering, DiskOfferingVO newDiskOffering, Long[] newSize, Long[] newMinIops, Long[] newMaxIops, Integer[] newHypervisorSnapshotReserve) {
void validateVolumeResizeWithNewDiskOfferingAndLoad(VolumeVO volume, DiskOfferingVO existingDiskOffering, DiskOfferingVO newDiskOffering, Long[] newSize, Long[] newMinIops, Long[] newMaxIops, Integer[] newHypervisorSnapshotReserve) {
if (newDiskOffering.getRemoved() != null) {
throw new InvalidParameterValueException("Requested disk offering has been removed.");
}
Expand Down
202 changes: 193 additions & 9 deletions server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,22 +33,14 @@
import static org.mockito.Mockito.when;

import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
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;
Expand All @@ -70,6 +62,7 @@
import org.apache.cloudstack.framework.async.AsyncCallFuture;
import org.apache.cloudstack.framework.jobs.AsyncJobExecutionContext;
import org.apache.cloudstack.framework.jobs.AsyncJobManager;
import org.apache.cloudstack.framework.jobs.Outcome;
import org.apache.cloudstack.framework.jobs.dao.AsyncJobJoinMapDao;
import org.apache.cloudstack.framework.jobs.impl.AsyncJobVO;
import org.apache.cloudstack.resourcedetail.dao.SnapshotPolicyDetailsDao;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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);

Comment on lines +2363 to +2365
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);
}
Comment on lines +2375 to +2377

/**
* 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);

Comment on lines +2397 to +2398
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;

volumeApiServiceImpl.validateVolumeReadyStateAndHypervisorChecks(volume, currentSizeBytes, newSizeBytes);
Assert.fail("Expected InvalidParameterValueException for VMware ROOT-volume resize "
+ "when VM state is Running");
}

/**
* 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);

// Stub the job-queue call so OutcomeImpl.s_jobMgr (static, uninitialized in tests)
// is never reached. The Outcome mock returns null from get() and a bare AsyncJobVO
// from getJob(), which is enough for the unmarshallResultObject path to return null.
@SuppressWarnings("unchecked")
Outcome<Volume> outcomemock = Mockito.mock(Outcome.class);
AsyncJobVO stubJob = new AsyncJobVO();
when(outcomemock.getJob()).thenReturn(stubJob);
doReturn(outcomemock).when(volumeApiServiceImpl).resizeVolumeThroughJobQueue(
anyLong(), anyLong(), anyLong(), anyLong(),
nullable(Long.class), nullable(Long.class),
nullable(Integer.class), nullable(Long.class), anyBoolean());

// resizeVolumeInternal(VolumeVO, DiskOfferingVO, Long, Long, Long, Long, Integer, boolean)
try {
volumeApiServiceImpl.resizeVolumeInternal(
volume,
/* newDiskOffering */ (DiskOfferingVO) null,
/* currentSize */ 0L,
/* newSize */ 1L,
/* newMinIops */ (Long) null,
/* newMaxIops */ (Long) null,
/* snapshotReserve */ (Integer) null,
/* shrinkOk */ false);
} catch (ResourceAllocationException e) {
Assert.fail("Unexpected ResourceAllocationException");
} catch (InvalidParameterValueException e) {
Assert.fail("VMware ROOT-volume resize must be allowed when CloudStack VM state is "
+ "Stopped, even under a power_state lag. Unexpected exception: "
+ e.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);

try {
volumeApiServiceImpl.resizeVolumeInternal(
volume,
(DiskOfferingVO) null,
0L, 1L, (Long) null, (Long) null, (Integer) null, false);
Assert.fail("Expected an InvalidParameterValueException for VMware ROOT-volume resize when VM state is Running");
} catch (ResourceAllocationException e) {
Assert.fail("Cause must be InvalidParameterValueException, was: " + e.getClass());
} catch (InvalidParameterValueException e) {
Assert.assertTrue(
"Exception message must reference Stopped-state requirement, was: " + e.getMessage(),
e.getMessage() != null && e.getMessage().contains("VM should be in"));
}
}
}
Loading