This is an automated email from the ASF dual-hosted git repository. DaanHoogland pushed a commit to tag 4.22.0.1 in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit a127a26ebd3cb2b3cdfed3e8a6b5f1a2c855a5cd 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 | 54 +++++++++++++------ .../cloud/vm/snapshot/VMSnapshotManagerTest.java | 62 +++++++++++++++++----- 2 files changed, 87 insertions(+), 29 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..fe91e1d9caa 100644 --- a/server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java @@ -751,11 +751,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 +814,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; } /** @@ -941,7 +963,9 @@ public class VMSnapshotManagerImpl extends MutualExclusiveIdsManagerBase impleme Transaction.execute(new TransactionCallbackWithExceptionNoReturn<CloudRuntimeException>() { @Override public void doInTransactionWithoutResult(TransactionStatus status) throws CloudRuntimeException { - updateUserVmServiceOffering(userVm, vmSnapshotVo); + if (userVmServiceOfferingNeedsChange(userVm, vmSnapshotVo)) { + changeUserVmServiceOffering(userVm, vmSnapshotVo); + } revertUserVmDetailsFromVmSnapshot(userVm, vmSnapshotVo); } }); 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..88af1a6325d 100644 --- a/server/src/test/java/com/cloud/vm/snapshot/VMSnapshotManagerTest.java +++ b/server/src/test/java/com/cloud/vm/snapshot/VMSnapshotManagerTest.java @@ -67,7 +67,6 @@ import org.junit.Before; import org.junit.Test; import org.mockito.ArgumentCaptor; import org.mockito.Captor; -import org.mockito.ArgumentMatchers; import org.mockito.Mock; import org.mockito.MockitoAnnotations; import org.mockito.Spy; @@ -79,9 +78,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 +225,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,20 +343,51 @@ 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 @@ -368,18 +402,18 @@ public class VMSnapshotManagerTest { @Test public void testChangeUserVmServiceOffering() throws ConcurrentOperationException, ResourceUnavailableException, ManagementServerException, VirtualMachineMigrationException { - when(_userVmManager.upgradeVirtualMachine(ArgumentMatchers.eq(TEST_VM_ID), ArgumentMatchers.eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture())).thenReturn(true); + when(_userVmManager.upgradeVirtualMachine(eq(TEST_VM_ID), eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture())).thenReturn(true); _vmSnapshotMgr.changeUserVmServiceOffering(userVm, vmSnapshotVO); verify(_vmSnapshotMgr).getVmMapDetails(vmSnapshotVO); - verify(_vmSnapshotMgr).upgradeUserVmServiceOffering(ArgumentMatchers.eq(userVm), ArgumentMatchers.eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture()); + verify(_vmSnapshotMgr).upgradeUserVmServiceOffering(eq(userVm), eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture()); } @Test(expected=CloudRuntimeException.class) public void testChangeUserVmServiceOfferingFailOnUpgradeVMServiceOffering() throws ConcurrentOperationException, ResourceUnavailableException, ManagementServerException, VirtualMachineMigrationException { - when(_userVmManager.upgradeVirtualMachine(ArgumentMatchers.eq(TEST_VM_ID), ArgumentMatchers.eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture())).thenReturn(false); + when(_userVmManager.upgradeVirtualMachine(eq(TEST_VM_ID), eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture())).thenReturn(false); _vmSnapshotMgr.changeUserVmServiceOffering(userVm, vmSnapshotVO); verify(_vmSnapshotMgr).getVmMapDetails(vmSnapshotVO); - verify(_vmSnapshotMgr).upgradeUserVmServiceOffering(ArgumentMatchers.eq(userVm), ArgumentMatchers.eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture()); + verify(_vmSnapshotMgr).upgradeUserVmServiceOffering(eq(userVm), eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture()); } @Test
