This is an automated email from the ASF dual-hosted git repository.
abhisar pushed a commit to branch 4.20
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/4.20 by this push:
new b22dbbe2d7a Fix Revert Instance to Snapshot with custom service
offering (#12885)
b22dbbe2d7a is described below
commit b22dbbe2d7ad128f5d4b73f9b24586ce095cc1d6
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 | 50 ++++++++++++++++----
2 files changed, 80 insertions(+), 24 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 8e7ce351246..b2ac831a633 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 class VMSnapshotManagerImpl extends
MutualExclusiveIdsManagerBase impleme
"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 @@ 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;
}
/**
@@ -944,7 +966,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);
+ }
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 01512b448b2..26f452cb9f4 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 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(VmDetailConstants.CPU_NUMBER);
@@ -345,20 +346,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(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
