From 25a053632cce7b268995ce49d17387a222ed4995 Mon Sep 17 00:00:00 2001 From: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com> Date: Fri, 30 Jan 2026 14:36:09 +0530 Subject: [PATCH 1/2] Revert settings cpu, cpu_speed and memory only during snapshot revert for custom service offerings --- .../vm/snapshot/VMSnapshotManagerImpl.java | 23 +++++++---- .../vm/snapshot/VMSnapshotManagerTest.java | 41 +++++++++++++------ 2 files changed, 44 insertions(+), 20 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 43270e6a0292..cdbb7119e9e8 100644 --- a/server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java @@ -23,6 +23,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; import javax.inject.Inject; import javax.naming.ConfigurationException; @@ -182,6 +183,11 @@ public class VMSnapshotManagerImpl extends MutualExclusiveIdsManagerBase impleme Long.class, "vm.job.check.interval", "3000", "Interval in milliseconds to check if the job is complete", false); + private static final Set VM_SNAPSHOT_CUSTOM_SERVICE_OFFERING_DETAILS = Set.of( + VmDetailConstants.CPU_NUMBER.toLowerCase(), + VmDetailConstants.CPU_SPEED.toLowerCase(), + VmDetailConstants.MEMORY.toLowerCase()); + @Override public boolean configure(String name, Map params) throws ConfigurationException { _name = name; @@ -473,7 +479,8 @@ public VMSnapshot doInTransaction(TransactionStatus status) { } /** - * Add entries on vm_snapshot_details if service offering is dynamic. This will allow setting details when revert to vm snapshot + * Add entries about cpu, cpu_speed and memory in vm_snapshot_details if service offering is dynamic. + * This will allow setting details when revert to vm snapshot. * @param vmId vm id * @param serviceOfferingId service offering id * @param vmSnapshotId vm snapshot id @@ -484,7 +491,7 @@ protected void addSupportForCustomServiceOffering(long vmId, long serviceOfferin List vmDetails = _userVmDetailsDao.listDetails(vmId); List vmSnapshotDetails = new ArrayList(); for (UserVmDetailVO detail : vmDetails) { - if(detail.getName().equalsIgnoreCase(VmDetailConstants.CPU_NUMBER) || detail.getName().equalsIgnoreCase(VmDetailConstants.CPU_SPEED) || detail.getName().equalsIgnoreCase(VmDetailConstants.MEMORY)) { + if (VM_SNAPSHOT_CUSTOM_SERVICE_OFFERING_DETAILS.contains(detail.getName().toLowerCase())) { vmSnapshotDetails.add(new VMSnapshotDetailsVO(vmSnapshotId, detail.getName(), detail.getValue(), detail.isDisplay())); } } @@ -931,7 +938,7 @@ private UserVm orchestrateRevertToVMSnapshot(Long vmSnapshotId) throws Insuffici Transaction.execute(new TransactionCallbackWithExceptionNoReturn() { @Override public void doInTransactionWithoutResult(TransactionStatus status) throws CloudRuntimeException { - revertUserVmDetailsFromVmSnapshot(userVm, vmSnapshotVo); + revertCustomServiceOfferingDetailsFromVmSnapshot(userVm, vmSnapshotVo); updateUserVmServiceOffering(userVm, vmSnapshotVo); } }); @@ -943,19 +950,19 @@ public void doInTransactionWithoutResult(TransactionStatus status) throws CloudR } /** - * Update or add user vm details from vm snapshot for vms with custom service offerings + * Update or add user vm details (cpu, cpu_speed and memory) from vm snapshot for vms with custom service offerings * @param userVm user vm * @param vmSnapshotVo vm snapshot */ - protected void revertUserVmDetailsFromVmSnapshot(UserVmVO userVm, VMSnapshotVO vmSnapshotVo) { + protected void revertCustomServiceOfferingDetailsFromVmSnapshot(UserVmVO userVm, VMSnapshotVO vmSnapshotVo) { ServiceOfferingVO serviceOfferingVO = _serviceOfferingDao.findById(vmSnapshotVo.getServiceOfferingId()); if (serviceOfferingVO.isDynamic()) { List vmSnapshotDetails = _vmSnapshotDetailsDao.listDetails(vmSnapshotVo.getId()); - List userVmDetails = new ArrayList(); for (VMSnapshotDetailsVO detail : vmSnapshotDetails) { - userVmDetails.add(new UserVmDetailVO(userVm.getId(), detail.getName(), detail.getValue(), detail.isDisplay())); + if (VM_SNAPSHOT_CUSTOM_SERVICE_OFFERING_DETAILS.contains(detail.getName().toLowerCase())) { + _userVmDetailsDao.addDetail(userVm.getId(), detail.getName(), detail.getValue(), detail.isDisplay()); + } } - _userVmDetailsDao.saveDetails(userVmDetails); } } 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 06c917a12440..84402e6e0317 100644 --- a/server/src/test/java/com/cloud/vm/snapshot/VMSnapshotManagerTest.java +++ b/server/src/test/java/com/cloud/vm/snapshot/VMSnapshotManagerTest.java @@ -51,6 +51,7 @@ import com.cloud.vm.UserVmVO; import com.cloud.vm.VirtualMachine.State; import com.cloud.vm.VirtualMachineManager; +import com.cloud.vm.VmDetailConstants; import com.cloud.vm.dao.UserVmDao; import com.cloud.vm.dao.UserVmDetailsDao; import com.cloud.vm.dao.VMInstanceDao; @@ -79,13 +80,18 @@ 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.anyBoolean; 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; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -225,13 +231,13 @@ public void setup() { when(_serviceOfferingDao.findById(SERVICE_OFFERING_ID)).thenReturn(serviceOffering); for (ResourceDetail detail : Arrays.asList(userVmDetailCpuNumber, vmSnapshotDetailCpuNumber)) { - when(detail.getName()).thenReturn("cpuNumber"); + when(detail.getName()).thenReturn(VmDetailConstants.CPU_NUMBER); when(detail.getValue()).thenReturn("2"); when(detail.isDisplay()).thenReturn(true); } for (ResourceDetail detail : Arrays.asList(userVmDetailMemory, vmSnapshotDetailMemory)) { - when(detail.getName()).thenReturn("memory"); + when(detail.getName()).thenReturn(VmDetailConstants.MEMORY); when(detail.getValue()).thenReturn("2048"); when(detail.isDisplay()).thenReturn(true); } @@ -348,12 +354,12 @@ public void testUpdateUserVmServiceOfferingSameServiceOffering() { @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); + when(_userVmManager.upgradeVirtualMachine(eq(TEST_VM_ID), eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture())).thenReturn(true); _vmSnapshotMgr.updateUserVmServiceOffering(userVm, vmSnapshotVO); verify(_vmSnapshotMgr).changeUserVmServiceOffering(userVm, vmSnapshotVO); verify(_vmSnapshotMgr).getVmMapDetails(userVm); - 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 @@ -368,18 +374,18 @@ public void testGetVmMapDetails() { @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(userVm); - 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(userVm); - 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 @@ -396,16 +402,27 @@ public void testUpgradeUserVmServiceOffering() throws ConcurrentOperationExcepti @Test public void testRevertUserVmDetailsFromVmSnapshotNotDynamicServiceOffering() { - _vmSnapshotMgr.revertUserVmDetailsFromVmSnapshot(vmMock, vmSnapshotVO); + _vmSnapshotMgr.revertCustomServiceOfferingDetailsFromVmSnapshot(vmMock, vmSnapshotVO); verify(_vmSnapshotDetailsDao, never()).listDetails(anyLong()); } @Test public void testRevertUserVmDetailsFromVmSnapshotDynamicServiceOffering() { when(serviceOffering.isDynamic()).thenReturn(true); - _vmSnapshotMgr.revertUserVmDetailsFromVmSnapshot(vmMock, vmSnapshotVO); + VMSnapshotDetailsVO uefiSnapshotDetail = new VMSnapshotDetailsVO(VM_SNAPSHOT_ID, "UEFI", "SECURE", true); + List snapshotDetailsWithUefi = Arrays.asList( + vmSnapshotDetailCpuNumber, vmSnapshotDetailMemory, uefiSnapshotDetail); + when(_vmSnapshotDetailsDao.listDetails(VM_SNAPSHOT_ID)).thenReturn(snapshotDetailsWithUefi); + + _vmSnapshotMgr.revertCustomServiceOfferingDetailsFromVmSnapshot(vmMock, vmSnapshotVO); + verify(_vmSnapshotDetailsDao).listDetails(VM_SNAPSHOT_ID); - verify(_userVmDetailsDao).saveDetails(listUserVmDetailsCaptor.capture()); + verify(_userVmDetailsDao, never()).saveDetails(any()); + ArgumentCaptor detailNameCaptor = ArgumentCaptor.forClass(String.class); + verify(_userVmDetailsDao, times(2)).addDetail(eq(TEST_VM_ID), detailNameCaptor.capture(), anyString(), anyBoolean()); + List appliedNames = detailNameCaptor.getAllValues(); + assertTrue(appliedNames.contains(VmDetailConstants.CPU_NUMBER)); + assertTrue(appliedNames.contains(VmDetailConstants.MEMORY)); + assertFalse("UEFI must not be applied from snapshot so that existing UEFI setting is preserved", appliedNames.contains("UEFI")); } - } From 5ce66336b9ca3b649b00c08645a4dab4a7e75499 Mon Sep 17 00:00:00 2001 From: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com> Date: Fri, 30 Jan 2026 15:01:51 +0530 Subject: [PATCH 2/2] fix checkstyle --- .../test/java/com/cloud/vm/snapshot/VMSnapshotManagerTest.java | 1 - 1 file changed, 1 deletion(-) 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 84402e6e0317..8d48fc4dac5b 100644 --- a/server/src/test/java/com/cloud/vm/snapshot/VMSnapshotManagerTest.java +++ b/server/src/test/java/com/cloud/vm/snapshot/VMSnapshotManagerTest.java @@ -68,7 +68,6 @@ 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;