This is an automated email from the ASF dual-hosted git repository. dahn pushed a commit to tag 4.22.0.1 in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit 4265eac832c18e9080b902a7d7ca6764e2c6d501 Author: Abhisar Sinha <[email protected]> AuthorDate: Fri Mar 27 10:19:52 2026 +0530 Fix Revert Instance to Snapshot with custom service offering (#12885) * Fix revertVM with custom svc offering --- .../cloud/vm/snapshot/VMSnapshotManagerImpl.java | 61 +++++++++++++++------- .../cloud/vm/snapshot/VMSnapshotManagerTest.java | 52 +++++++++++++++--- 2 files changed, 85 insertions(+), 28 deletions(-) diff --git a/server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java b/server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java index b2e820bce94..8c77eecc1aa 100644 --- a/server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java @@ -442,7 +442,6 @@ public class VMSnapshotManagerImpl extends MutualExclusiveIdsManagerBase impleme * Create, persist and return vm snapshot for userVmVo with given parameters. * Persistence and support for custom service offerings are done on the same transaction * @param userVmVo user vm - * @param vmId vm id * @param vsDescription vm description * @param vmSnapshotName vm snapshot name * @param vsDisplayName vm snapshot display name @@ -751,11 +750,17 @@ public class VMSnapshotManagerImpl extends MutualExclusiveIdsManagerBase impleme "VM Snapshot reverting failed due to vm is not in the state of Running or Stopped."); } - if (userVm.getState() == VirtualMachine.State.Running && vmSnapshotVo.getType() == VMSnapshot.Type.Disk || userVm.getState() == VirtualMachine.State.Stopped - && vmSnapshotVo.getType() == VMSnapshot.Type.DiskAndMemory) { + if (userVm.getState() == VirtualMachine.State.Running && vmSnapshotVo.getType() == VMSnapshot.Type.Disk) { throw new InvalidParameterValueException( - "VM Snapshot revert not allowed. This will result in VM state change. You can revert running VM to disk and memory type snapshot and stopped VM to disk type" - + " snapshot"); + "Reverting to the Instance Snapshot is not allowed for running Instances as this would result in an Instance state change. " + + "For running Instances only Snapshots with memory can be reverted. " + + "In order to revert to a Snapshot without memory you need to first stop the Instance."); + } + + if (userVm.getState() == VirtualMachine.State.Stopped && vmSnapshotVo.getType() == VMSnapshot.Type.DiskAndMemory) { + throw new InvalidParameterValueException( + "Reverting to the Instance Snapshot is not allowed for stopped Instances when the Snapshot contains memory as this would result in an Instance state change. " + + "In order to revert to a Snapshot with memory you need to first start the Instance."); } // if snapshot is not created, error out @@ -808,20 +813,36 @@ public class VMSnapshotManagerImpl extends MutualExclusiveIdsManagerBase impleme } /** - * If snapshot was taken with a different service offering than actual used in vm, should change it back to it. - * We also call <code>changeUserVmServiceOffering</code> in case the service offering is dynamic in order to - * perform resource limit validation, as the amount of CPUs or memory may have been changed. - * @param vmSnapshotVo vm snapshot + * Check if service offering change is needed for user vm when reverting to vm snapshot. + * Service offering change is needed when snapshot was taken with a different service offering than actual used in vm. + * Service offering change is also needed when service offering is dynamic and the amount of cpu, memory or cpu speed + * has been changed since snapshot was taken. + * @param userVm + * @param vmSnapshotVo + * @return true if service offering change is needed; false otherwise */ - protected void updateUserVmServiceOffering(UserVm userVm, VMSnapshotVO vmSnapshotVo) { + protected boolean userVmServiceOfferingNeedsChange(UserVm userVm, VMSnapshotVO vmSnapshotVo) { if (vmSnapshotVo.getServiceOfferingId() != userVm.getServiceOfferingId()) { - changeUserVmServiceOffering(userVm, vmSnapshotVo); - return; + return true; } - ServiceOfferingVO serviceOffering = _serviceOfferingDao.findById(userVm.getServiceOfferingId()); - if (serviceOffering.isDynamic()) { - changeUserVmServiceOffering(userVm, vmSnapshotVo); + + ServiceOfferingVO currentServiceOffering = _serviceOfferingDao.findByIdIncludingRemoved(userVm.getId(), userVm.getServiceOfferingId()); + if (currentServiceOffering.isDynamic()) { + Map<String, String> vmDetails = getVmMapDetails(vmSnapshotVo); + ServiceOfferingVO newServiceOffering = _serviceOfferingDao.getComputeOffering(currentServiceOffering, vmDetails); + + int newCpu = newServiceOffering.getCpu(); + int newMemory = newServiceOffering.getRamSize(); + int newSpeed = newServiceOffering.getSpeed(); + int currentCpu = currentServiceOffering.getCpu(); + int currentMemory = currentServiceOffering.getRamSize(); + int currentSpeed = currentServiceOffering.getSpeed(); + + if (newCpu != currentCpu || newMemory != currentMemory || newSpeed != currentSpeed) { + return true; + } } + return false; } /** @@ -839,7 +860,7 @@ public class VMSnapshotManagerImpl extends MutualExclusiveIdsManagerBase impleme } /** - * Update service offering on {@link userVm} to the one specified in {@link vmSnapshotVo} + * Update service offering on {code}userVm{code} to the one specified in {code}vmSnapshotVo{code} * @param userVm user vm to be updated * @param vmSnapshotVo vm snapshot */ @@ -853,8 +874,8 @@ public class VMSnapshotManagerImpl extends MutualExclusiveIdsManagerBase impleme } /** - * Upgrade virtual machine {@linkplain vmId} to new service offering {@linkplain serviceOfferingId} - * @param vmId vm id + * Upgrade virtual machine {code}vm{code} to new service offering {code}serviceOfferingId{code} + * @param vm vm to be upgraded * @param serviceOfferingId service offering id * @param details vm details * @return if operation was successful @@ -941,7 +962,7 @@ public class VMSnapshotManagerImpl extends MutualExclusiveIdsManagerBase impleme Transaction.execute(new TransactionCallbackWithExceptionNoReturn<CloudRuntimeException>() { @Override public void doInTransactionWithoutResult(TransactionStatus status) throws CloudRuntimeException { - updateUserVmServiceOffering(userVm, vmSnapshotVo); + userVmServiceOfferingNeedsChange(userVm, vmSnapshotVo); revertUserVmDetailsFromVmSnapshot(userVm, vmSnapshotVo); } }); @@ -1292,7 +1313,7 @@ public class VMSnapshotManagerImpl extends MutualExclusiveIdsManagerBase impleme public Pair<JobInfo.Status, String> orchestrateCreateVMSnapshot(VmWorkCreateVMSnapshot work) throws Exception { VMSnapshot snapshot = orchestrateCreateVMSnapshot(work.getVmId(), work.getVmSnapshotId(), work.isQuiesceVm()); return new Pair<JobInfo.Status, String>(JobInfo.Status.SUCCEEDED, - _jobMgr.marshallResultObject(new Long(snapshot.getId()))); + _jobMgr.marshallResultObject(Long.valueOf(snapshot.getId()))); } @ReflectionUse diff --git a/server/src/test/java/com/cloud/vm/snapshot/VMSnapshotManagerTest.java b/server/src/test/java/com/cloud/vm/snapshot/VMSnapshotManagerTest.java index 7b12fc4a99c..566d97d7b40 100644 --- a/server/src/test/java/com/cloud/vm/snapshot/VMSnapshotManagerTest.java +++ b/server/src/test/java/com/cloud/vm/snapshot/VMSnapshotManagerTest.java @@ -79,9 +79,12 @@ import java.util.List; import java.util.Map; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; @@ -223,6 +226,7 @@ public class VMSnapshotManagerTest { when(vmSnapshotVO.getId()).thenReturn(VM_SNAPSHOT_ID); when(serviceOffering.isDynamic()).thenReturn(false); when(_serviceOfferingDao.findById(SERVICE_OFFERING_ID)).thenReturn(serviceOffering); + when(_serviceOfferingDao.findByIdIncludingRemoved(TEST_VM_ID, SERVICE_OFFERING_ID)).thenReturn(serviceOffering); for (ResourceDetail detail : Arrays.asList(userVmDetailCpuNumber, vmSnapshotDetailCpuNumber)) { when(detail.getName()).thenReturn("cpuNumber"); @@ -340,22 +344,54 @@ public class VMSnapshotManagerTest { } @Test - public void testUpdateUserVmServiceOfferingSameServiceOffering() { - _vmSnapshotMgr.updateUserVmServiceOffering(userVm, vmSnapshotVO); - verify(_vmSnapshotMgr, never()).changeUserVmServiceOffering(userVm, vmSnapshotVO); + public void testUserVmServiceOfferingNeedsChangeWhenSnapshotOfferingDiffers() { + when(userVm.getServiceOfferingId()).thenReturn(SERVICE_OFFERING_DIFFERENT_ID); + when(vmSnapshotVO.getServiceOfferingId()).thenReturn(SERVICE_OFFERING_ID); + + assertTrue(_vmSnapshotMgr.userVmServiceOfferingNeedsChange(userVm, vmSnapshotVO)); + + verify(_serviceOfferingDao, never()).findByIdIncludingRemoved(anyLong(), anyLong()); + verify(_serviceOfferingDao, never()).getComputeOffering(any(ServiceOfferingVO.class), any()); } @Test - public void testUpdateUserVmServiceOfferingDifferentServiceOffering() throws ConcurrentOperationException, ResourceUnavailableException, ManagementServerException, VirtualMachineMigrationException { - when(userVm.getServiceOfferingId()).thenReturn(SERVICE_OFFERING_DIFFERENT_ID); - when(_userVmManager.upgradeVirtualMachine(ArgumentMatchers.eq(TEST_VM_ID), ArgumentMatchers.eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture())).thenReturn(true); - _vmSnapshotMgr.updateUserVmServiceOffering(userVm, vmSnapshotVO); + public void testUserVmServiceOfferingNeedsChangeWhenSameNonDynamicOffering() { + assertFalse(_vmSnapshotMgr.userVmServiceOfferingNeedsChange(userVm, vmSnapshotVO)); + + verify(_serviceOfferingDao).findByIdIncludingRemoved(TEST_VM_ID, SERVICE_OFFERING_ID); + verify(_serviceOfferingDao, never()).getComputeOffering(any(ServiceOfferingVO.class), any()); + } - verify(_vmSnapshotMgr).changeUserVmServiceOffering(userVm, vmSnapshotVO); + @Test + public void testUserVmServiceOfferingNeedsChangeWhenDynamicOfferingMatchesSnapshot() { + when(serviceOffering.isDynamic()).thenReturn(true); + when(serviceOffering.getCpu()).thenReturn(2); + when(serviceOffering.getRamSize()).thenReturn(2048); + when(serviceOffering.getSpeed()).thenReturn(1000); + when(_serviceOfferingDao.getComputeOffering(eq(serviceOffering), any())).thenReturn(serviceOffering); + + assertFalse(_vmSnapshotMgr.userVmServiceOfferingNeedsChange(userVm, vmSnapshotVO)); + + verify(_serviceOfferingDao).getComputeOffering(eq(serviceOffering), any()); verify(_vmSnapshotMgr).getVmMapDetails(vmSnapshotVO); verify(_vmSnapshotMgr).upgradeUserVmServiceOffering(ArgumentMatchers.eq(userVm), ArgumentMatchers.eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture()); } + @Test + public void testUserVmServiceOfferingNeedsChangeWhenDynamicCpuDiffersFromSnapshot() { + when(serviceOffering.isDynamic()).thenReturn(true); + when(serviceOffering.getCpu()).thenReturn(2); + when(serviceOffering.getRamSize()).thenReturn(2048); + when(serviceOffering.getSpeed()).thenReturn(1000); + ServiceOfferingVO fromSnapshot = mock(ServiceOfferingVO.class); + when(fromSnapshot.getCpu()).thenReturn(4); + when(fromSnapshot.getRamSize()).thenReturn(2048); + when(fromSnapshot.getSpeed()).thenReturn(1000); + when(_serviceOfferingDao.getComputeOffering(eq(serviceOffering), any())).thenReturn(fromSnapshot); + + assertTrue(_vmSnapshotMgr.userVmServiceOfferingNeedsChange(userVm, vmSnapshotVO)); + } + @Test public void testGetVmMapDetails() { Map<String, String> result = _vmSnapshotMgr.getVmMapDetails(vmSnapshotVO);
