Skip to content

Commit 5e066a1

Browse files
abh1sarabh1sar
committed
Fix revertVM with custom svc offering
1 parent c19630f commit 5e066a1

File tree

2 files changed

+80
-24
lines changed

2 files changed

+80
-24
lines changed

‎server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java‎

Lines changed: 39 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -754,11 +754,17 @@ public UserVm revertToSnapshot(Long vmSnapshotId) throws InsufficientCapacityExc
754754
"Instance Snapshot reverting failed because the Instance is not in Running or Stopped state.");
755755
}
756756

757-
if (userVm.getState() == VirtualMachine.State.Running && vmSnapshotVo.getType() == VMSnapshot.Type.Disk || userVm.getState() == VirtualMachine.State.Stopped
758-
&& vmSnapshotVo.getType() == VMSnapshot.Type.DiskAndMemory) {
757+
if (userVm.getState() == VirtualMachine.State.Running && vmSnapshotVo.getType() == VMSnapshot.Type.Disk) {
759758
throw new InvalidParameterValueException(
760-
"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."
761-
+ " Snapshot");
759+
"Reverting to the Instance Snapshot is not allowed for running Instances as this would result in an Instance state change. " +
760+
"For running Instances only Snapshots with memory can be reverted. " +
761+
"In order to revert to a Snapshot without memory you need to first stop the Instance.");
762+
}
763+
764+
if ( userVm.getState() == VirtualMachine.State.Stopped && vmSnapshotVo.getType() == VMSnapshot.Type.DiskAndMemory) {
765+
throw new InvalidParameterValueException(
766+
"Reverting to the Instance Snapshot is not allowed for stopped Instances if the Snapshot contains memory. " +
767+
"In order to revert to a Snapshot with memory you need to first start the Instance.");
762768
}
763769

764770
// if snapshot is not created, error out
@@ -811,20 +817,36 @@ else if (jobResult instanceof Throwable)
811817
}
812818

813819
/**
814-
* If snapshot was taken with a different service offering than actual used in vm, should change it back to it.
815-
* We also call <code>changeUserVmServiceOffering</code> in case the service offering is dynamic in order to
816-
* perform resource limit validation, as the amount of CPUs or memory may have been changed.
817-
* @param vmSnapshotVo vm snapshot
820+
* Check if service offering change is needed for user vm when reverting to vm snapshot.
821+
* Service offering change is needed when snapshot was taken with a different service offering than actual used in vm.
822+
* Service offering change is also needed when service offering is dynamic and the amount of cpu, memory or cpu speed
823+
* has been changed since snapshot was taken.
824+
* @param userVm
825+
* @param vmSnapshotVo
826+
* @return true if service offering change is needed; false otherwise
818827
*/
819-
protected void updateUserVmServiceOffering(UserVm userVm, VMSnapshotVO vmSnapshotVo) {
828+
protected boolean userVmServiceOfferingNeedsChange(UserVm userVm, VMSnapshotVO vmSnapshotVo) {
820829
if (vmSnapshotVo.getServiceOfferingId() != userVm.getServiceOfferingId()) {
821-
changeUserVmServiceOffering(userVm, vmSnapshotVo);
822-
return;
830+
return true;
823831
}
824-
ServiceOfferingVO serviceOffering = _serviceOfferingDao.findById(userVm.getServiceOfferingId());
825-
if (serviceOffering.isDynamic()) {
826-
changeUserVmServiceOffering(userVm, vmSnapshotVo);
832+
833+
ServiceOfferingVO currentServiceOffering = _serviceOfferingDao.findByIdIncludingRemoved(userVm.getId(), userVm.getServiceOfferingId());
834+
if (currentServiceOffering.isDynamic()) {
835+
Map<String, String> vmDetails = getVmMapDetails(vmSnapshotVo);
836+
ServiceOfferingVO newServiceOffering = _serviceOfferingDao.getComputeOffering(currentServiceOffering, vmDetails);
837+
838+
int newCpu = newServiceOffering.getCpu();
839+
int newMemory = newServiceOffering.getRamSize();
840+
int newSpeed = newServiceOffering.getSpeed();
841+
int currentCpu = currentServiceOffering.getCpu();
842+
int currentMemory = currentServiceOffering.getRamSize();
843+
int currentSpeed = currentServiceOffering.getSpeed();
844+
845+
if (newCpu != currentCpu || newMemory != currentMemory || newSpeed != currentSpeed) {
846+
return true;
847+
}
827848
}
849+
return false;
828850
}
829851

830852
/**
@@ -944,7 +966,9 @@ private UserVm orchestrateRevertToVMSnapshot(Long vmSnapshotId) throws Insuffici
944966
Transaction.execute(new TransactionCallbackWithExceptionNoReturn<CloudRuntimeException>() {
945967
@Override
946968
public void doInTransactionWithoutResult(TransactionStatus status) throws CloudRuntimeException {
947-
updateUserVmServiceOffering(userVm, vmSnapshotVo);
969+
if (userVmServiceOfferingNeedsChange(userVm, vmSnapshotVo)) {
970+
changeUserVmServiceOffering(userVm, vmSnapshotVo);
971+
}
948972
revertCustomServiceOfferingDetailsFromVmSnapshot(userVm, vmSnapshotVo);
949973
}
950974
});

‎server/src/test/java/com/cloud/vm/snapshot/VMSnapshotManagerTest.java‎

Lines changed: 41 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,7 @@ public void setup() {
228228
when(vmSnapshotVO.getId()).thenReturn(VM_SNAPSHOT_ID);
229229
when(serviceOffering.isDynamic()).thenReturn(false);
230230
when(_serviceOfferingDao.findById(SERVICE_OFFERING_ID)).thenReturn(serviceOffering);
231+
when(_serviceOfferingDao.findByIdIncludingRemoved(TEST_VM_ID, SERVICE_OFFERING_ID)).thenReturn(serviceOffering);
231232

232233
for (ResourceDetail detail : Arrays.asList(userVmDetailCpuNumber, vmSnapshotDetailCpuNumber)) {
233234
when(detail.getName()).thenReturn(VmDetailConstants.CPU_NUMBER);
@@ -345,20 +346,51 @@ public void testAddSupportForCustomServiceOfferingDynamicServiceOffering() {
345346
}
346347

347348
@Test
348-
public void testUpdateUserVmServiceOfferingSameServiceOffering() {
349-
_vmSnapshotMgr.updateUserVmServiceOffering(userVm, vmSnapshotVO);
350-
verify(_vmSnapshotMgr, never()).changeUserVmServiceOffering(userVm, vmSnapshotVO);
349+
public void testUserVmServiceOfferingNeedsChangeWhenSnapshotOfferingDiffers() {
350+
when(userVm.getServiceOfferingId()).thenReturn(SERVICE_OFFERING_DIFFERENT_ID);
351+
when(vmSnapshotVO.getServiceOfferingId()).thenReturn(SERVICE_OFFERING_ID);
352+
353+
assertTrue(_vmSnapshotMgr.userVmServiceOfferingNeedsChange(userVm, vmSnapshotVO));
354+
355+
verify(_serviceOfferingDao, never()).findByIdIncludingRemoved(anyLong(), anyLong());
356+
verify(_serviceOfferingDao, never()).getComputeOffering(any(ServiceOfferingVO.class), any());
351357
}
352358

353359
@Test
354-
public void testUpdateUserVmServiceOfferingDifferentServiceOffering() throws ConcurrentOperationException, ResourceUnavailableException, ManagementServerException, VirtualMachineMigrationException {
355-
when(userVm.getServiceOfferingId()).thenReturn(SERVICE_OFFERING_DIFFERENT_ID);
356-
when(_userVmManager.upgradeVirtualMachine(eq(TEST_VM_ID), eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture())).thenReturn(true);
357-
_vmSnapshotMgr.updateUserVmServiceOffering(userVm, vmSnapshotVO);
360+
public void testUserVmServiceOfferingNeedsChangeWhenSameNonDynamicOffering() {
361+
assertFalse(_vmSnapshotMgr.userVmServiceOfferingNeedsChange(userVm, vmSnapshotVO));
362+
363+
verify(_serviceOfferingDao).findByIdIncludingRemoved(TEST_VM_ID, SERVICE_OFFERING_ID);
364+
verify(_serviceOfferingDao, never()).getComputeOffering(any(ServiceOfferingVO.class), any());
365+
}
358366

359-
verify(_vmSnapshotMgr).changeUserVmServiceOffering(userVm, vmSnapshotVO);
367+
@Test
368+
public void testUserVmServiceOfferingNeedsChangeWhenDynamicOfferingMatchesSnapshot() {
369+
when(serviceOffering.isDynamic()).thenReturn(true);
370+
when(serviceOffering.getCpu()).thenReturn(2);
371+
when(serviceOffering.getRamSize()).thenReturn(2048);
372+
when(serviceOffering.getSpeed()).thenReturn(1000);
373+
when(_serviceOfferingDao.getComputeOffering(eq(serviceOffering), any())).thenReturn(serviceOffering);
374+
375+
assertFalse(_vmSnapshotMgr.userVmServiceOfferingNeedsChange(userVm, vmSnapshotVO));
376+
377+
verify(_serviceOfferingDao).getComputeOffering(eq(serviceOffering), any());
360378
verify(_vmSnapshotMgr).getVmMapDetails(vmSnapshotVO);
361-
verify(_vmSnapshotMgr).upgradeUserVmServiceOffering(eq(userVm), eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture());
379+
}
380+
381+
@Test
382+
public void testUserVmServiceOfferingNeedsChangeWhenDynamicCpuDiffersFromSnapshot() {
383+
when(serviceOffering.isDynamic()).thenReturn(true);
384+
when(serviceOffering.getCpu()).thenReturn(2);
385+
when(serviceOffering.getRamSize()).thenReturn(2048);
386+
when(serviceOffering.getSpeed()).thenReturn(1000);
387+
ServiceOfferingVO fromSnapshot = mock(ServiceOfferingVO.class);
388+
when(fromSnapshot.getCpu()).thenReturn(4);
389+
when(fromSnapshot.getRamSize()).thenReturn(2048);
390+
when(fromSnapshot.getSpeed()).thenReturn(1000);
391+
when(_serviceOfferingDao.getComputeOffering(eq(serviceOffering), any())).thenReturn(fromSnapshot);
392+
393+
assertTrue(_vmSnapshotMgr.userVmServiceOfferingNeedsChange(userVm, vmSnapshotVO));
362394
}
363395

364396
@Test

0 commit comments

Comments
 (0)