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 8e7ce351246a..b2ac831a633e 100644 --- a/server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java @@ -754,11 +754,17 @@ public UserVm revertToSnapshot(Long vmSnapshotId) throws InsufficientCapacityExc "Instance Snapshot reverting failed because the Instance is not in Running or Stopped state."); } - 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( - "Reverting to the Instance Snapshot is not allowed for running Instances as this would result in a 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." - + " 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 @@ -811,20 +817,36 @@ else if (jobResult instanceof Throwable) } /** - * If snapshot was taken with a different service offering than actual used in vm, should change it back to it. - * We also call changeUserVmServiceOffering 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 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; } /** @@ -944,7 +966,9 @@ private UserVm orchestrateRevertToVMSnapshot(Long vmSnapshotId) throws Insuffici Transaction.execute(new TransactionCallbackWithExceptionNoReturn() { @Override public void doInTransactionWithoutResult(TransactionStatus status) throws CloudRuntimeException { - updateUserVmServiceOffering(userVm, vmSnapshotVo); + if (userVmServiceOfferingNeedsChange(userVm, vmSnapshotVo)) { + changeUserVmServiceOffering(userVm, vmSnapshotVo); + } revertCustomServiceOfferingDetailsFromVmSnapshot(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 01512b448b25..26f452cb9f42 100644 --- a/server/src/test/java/com/cloud/vm/snapshot/VMSnapshotManagerTest.java +++ b/server/src/test/java/com/cloud/vm/snapshot/VMSnapshotManagerTest.java @@ -228,6 +228,7 @@ public void setup() { 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(VmDetailConstants.CPU_NUMBER); @@ -345,20 +346,51 @@ public void testAddSupportForCustomServiceOfferingDynamicServiceOffering() { } @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(eq(TEST_VM_ID), 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(eq(userVm), 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