mirror of https://github.com/apache/cloudstack.git
check state instead of power state on change offering for ROOT volume (#13271)
* check state instead of power state on change offering for ROOT volume * tests * Apply suggestions from code review Co-authored-by: Fabricio Duarte <fabricio.duarte.jr@gmail.com> Co-authored-by: dahn <daan.hoogland@gmail.com> * Apply test by package private * fixing tests --------- Co-authored-by: Daan Hoogland <dahn@apache.org> Co-authored-by: Fabricio Duarte <fabricio.duarte.jr@gmail.com>
This commit is contained in:
dahn
GitHub
parent
daa2e61eef
commit
7c2a8768e6
|
|
@ -2311,14 +2311,16 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
|
|||
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.");
|
||||
}
|
||||
// serialize VM operation
|
||||
AsyncJobExecutionContext jobContext = AsyncJobExecutionContext.getCurrentExecutionContext();
|
||||
|
|
@ -2372,7 +2374,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
|
|||
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) {
|
||||
|
|
@ -2396,9 +2398,11 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
|
|||
|
||||
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.");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -2415,7 +2419,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic
|
|||
}
|
||||
}
|
||||
|
||||
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.");
|
||||
}
|
||||
|
|
|
|||
|
|
@ -33,6 +33,7 @@ import static org.mockito.Mockito.verify;
|
|||
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;
|
||||
|
|
@ -40,15 +41,6 @@ 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;
|
||||
|
|
@ -70,6 +62,7 @@ import org.apache.cloudstack.engine.subsystem.api.storage.VolumeService.VolumeAp
|
|||
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;
|
||||
|
|
@ -107,17 +100,23 @@ import com.cloud.dc.HostPodVO;
|
|||
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.VirtualMachine.State;
|
|||
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,186 @@ public class VolumeApiServiceImplTest {
|
|||
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);
|
||||
|
||||
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…
Reference in New Issue