Skip to content

Commit 9ae696d

Browse files
abh1sarabh1sar
authored
Preserve VM settings on Instance Snapshot revert for Custom Service Offering (apache#12555)
1 parent ce42ce5 commit 9ae696d

File tree

2 files changed

+44
-21
lines changed

2 files changed

+44
-21
lines changed

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

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import java.util.HashMap;
2424
import java.util.List;
2525
import java.util.Map;
26+
import java.util.Set;
2627

2728
import javax.inject.Inject;
2829
import javax.naming.ConfigurationException;
@@ -182,6 +183,11 @@ public class VMSnapshotManagerImpl extends MutualExclusiveIdsManagerBase impleme
182183
Long.class, "vm.job.check.interval", "3000",
183184
"Interval in milliseconds to check if the job is complete", false);
184185

186+
private static final Set<String> VM_SNAPSHOT_CUSTOM_SERVICE_OFFERING_DETAILS = Set.of(
187+
VmDetailConstants.CPU_NUMBER.toLowerCase(),
188+
VmDetailConstants.CPU_SPEED.toLowerCase(),
189+
VmDetailConstants.MEMORY.toLowerCase());
190+
185191
@Override
186192
public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
187193
_name = name;
@@ -473,7 +479,8 @@ public VMSnapshot doInTransaction(TransactionStatus status) {
473479
}
474480

475481
/**
476-
* Add entries on vm_snapshot_details if service offering is dynamic. This will allow setting details when revert to vm snapshot
482+
* Add entries about cpu, cpu_speed and memory in vm_snapshot_details if service offering is dynamic.
483+
* This will allow setting details when revert to vm snapshot.
477484
* @param vmId vm id
478485
* @param serviceOfferingId service offering id
479486
* @param vmSnapshotId vm snapshot id
@@ -484,7 +491,7 @@ protected void addSupportForCustomServiceOffering(long vmId, long serviceOfferin
484491
List<UserVmDetailVO> vmDetails = _userVmDetailsDao.listDetails(vmId);
485492
List<VMSnapshotDetailsVO> vmSnapshotDetails = new ArrayList<VMSnapshotDetailsVO>();
486493
for (UserVmDetailVO detail : vmDetails) {
487-
if(detail.getName().equalsIgnoreCase(VmDetailConstants.CPU_NUMBER) || detail.getName().equalsIgnoreCase(VmDetailConstants.CPU_SPEED) || detail.getName().equalsIgnoreCase(VmDetailConstants.MEMORY)) {
494+
if (VM_SNAPSHOT_CUSTOM_SERVICE_OFFERING_DETAILS.contains(detail.getName().toLowerCase())) {
488495
vmSnapshotDetails.add(new VMSnapshotDetailsVO(vmSnapshotId, detail.getName(), detail.getValue(), detail.isDisplay()));
489496
}
490497
}
@@ -931,7 +938,7 @@ private UserVm orchestrateRevertToVMSnapshot(Long vmSnapshotId) throws Insuffici
931938
Transaction.execute(new TransactionCallbackWithExceptionNoReturn<CloudRuntimeException>() {
932939
@Override
933940
public void doInTransactionWithoutResult(TransactionStatus status) throws CloudRuntimeException {
934-
revertUserVmDetailsFromVmSnapshot(userVm, vmSnapshotVo);
941+
revertCustomServiceOfferingDetailsFromVmSnapshot(userVm, vmSnapshotVo);
935942
updateUserVmServiceOffering(userVm, vmSnapshotVo);
936943
}
937944
});
@@ -943,19 +950,19 @@ public void doInTransactionWithoutResult(TransactionStatus status) throws CloudR
943950
}
944951

945952
/**
946-
* Update or add user vm details from vm snapshot for vms with custom service offerings
953+
* Update or add user vm details (cpu, cpu_speed and memory) from vm snapshot for vms with custom service offerings
947954
* @param userVm user vm
948955
* @param vmSnapshotVo vm snapshot
949956
*/
950-
protected void revertUserVmDetailsFromVmSnapshot(UserVmVO userVm, VMSnapshotVO vmSnapshotVo) {
957+
protected void revertCustomServiceOfferingDetailsFromVmSnapshot(UserVmVO userVm, VMSnapshotVO vmSnapshotVo) {
951958
ServiceOfferingVO serviceOfferingVO = _serviceOfferingDao.findById(vmSnapshotVo.getServiceOfferingId());
952959
if (serviceOfferingVO.isDynamic()) {
953960
List<VMSnapshotDetailsVO> vmSnapshotDetails = _vmSnapshotDetailsDao.listDetails(vmSnapshotVo.getId());
954-
List<UserVmDetailVO> userVmDetails = new ArrayList<UserVmDetailVO>();
955961
for (VMSnapshotDetailsVO detail : vmSnapshotDetails) {
956-
userVmDetails.add(new UserVmDetailVO(userVm.getId(), detail.getName(), detail.getValue(), detail.isDisplay()));
962+
if (VM_SNAPSHOT_CUSTOM_SERVICE_OFFERING_DETAILS.contains(detail.getName().toLowerCase())) {
963+
_userVmDetailsDao.addDetail(userVm.getId(), detail.getName(), detail.getValue(), detail.isDisplay());
964+
}
957965
}
958-
_userVmDetailsDao.saveDetails(userVmDetails);
959966
}
960967
}
961968

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

Lines changed: 29 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@
5151
import com.cloud.vm.UserVmVO;
5252
import com.cloud.vm.VirtualMachine.State;
5353
import com.cloud.vm.VirtualMachineManager;
54+
import com.cloud.vm.VmDetailConstants;
5455
import com.cloud.vm.dao.UserVmDao;
5556
import com.cloud.vm.dao.UserVmDetailsDao;
5657
import com.cloud.vm.dao.VMInstanceDao;
@@ -67,7 +68,6 @@
6768
import org.junit.Test;
6869
import org.mockito.ArgumentCaptor;
6970
import org.mockito.Captor;
70-
import org.mockito.ArgumentMatchers;
7171
import org.mockito.Mock;
7272
import org.mockito.MockitoAnnotations;
7373
import org.mockito.Spy;
@@ -79,13 +79,18 @@
7979
import java.util.Map;
8080

8181
import static org.junit.Assert.assertEquals;
82+
import static org.junit.Assert.assertFalse;
83+
import static org.junit.Assert.assertTrue;
8284
import static org.mockito.ArgumentMatchers.any;
85+
import static org.mockito.ArgumentMatchers.anyBoolean;
8386
import static org.mockito.ArgumentMatchers.anyLong;
8487
import static org.mockito.ArgumentMatchers.anyString;
88+
import static org.mockito.ArgumentMatchers.eq;
8589
import static org.mockito.Mockito.doNothing;
8690
import static org.mockito.Mockito.doReturn;
8791
import static org.mockito.Mockito.mock;
8892
import static org.mockito.Mockito.never;
93+
import static org.mockito.Mockito.times;
8994
import static org.mockito.Mockito.verify;
9095
import static org.mockito.Mockito.when;
9196

@@ -225,13 +230,13 @@ public void setup() {
225230
when(_serviceOfferingDao.findById(SERVICE_OFFERING_ID)).thenReturn(serviceOffering);
226231

227232
for (ResourceDetail detail : Arrays.asList(userVmDetailCpuNumber, vmSnapshotDetailCpuNumber)) {
228-
when(detail.getName()).thenReturn("cpuNumber");
233+
when(detail.getName()).thenReturn(VmDetailConstants.CPU_NUMBER);
229234
when(detail.getValue()).thenReturn("2");
230235
when(detail.isDisplay()).thenReturn(true);
231236
}
232237

233238
for (ResourceDetail detail : Arrays.asList(userVmDetailMemory, vmSnapshotDetailMemory)) {
234-
when(detail.getName()).thenReturn("memory");
239+
when(detail.getName()).thenReturn(VmDetailConstants.MEMORY);
235240
when(detail.getValue()).thenReturn("2048");
236241
when(detail.isDisplay()).thenReturn(true);
237242
}
@@ -348,12 +353,12 @@ public void testUpdateUserVmServiceOfferingSameServiceOffering() {
348353
@Test
349354
public void testUpdateUserVmServiceOfferingDifferentServiceOffering() throws ConcurrentOperationException, ResourceUnavailableException, ManagementServerException, VirtualMachineMigrationException {
350355
when(userVm.getServiceOfferingId()).thenReturn(SERVICE_OFFERING_DIFFERENT_ID);
351-
when(_userVmManager.upgradeVirtualMachine(ArgumentMatchers.eq(TEST_VM_ID), ArgumentMatchers.eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture())).thenReturn(true);
356+
when(_userVmManager.upgradeVirtualMachine(eq(TEST_VM_ID), eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture())).thenReturn(true);
352357
_vmSnapshotMgr.updateUserVmServiceOffering(userVm, vmSnapshotVO);
353358

354359
verify(_vmSnapshotMgr).changeUserVmServiceOffering(userVm, vmSnapshotVO);
355360
verify(_vmSnapshotMgr).getVmMapDetails(userVm);
356-
verify(_vmSnapshotMgr).upgradeUserVmServiceOffering(ArgumentMatchers.eq(userVm), ArgumentMatchers.eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture());
361+
verify(_vmSnapshotMgr).upgradeUserVmServiceOffering(eq(userVm), eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture());
357362
}
358363

359364
@Test
@@ -368,18 +373,18 @@ public void testGetVmMapDetails() {
368373

369374
@Test
370375
public void testChangeUserVmServiceOffering() throws ConcurrentOperationException, ResourceUnavailableException, ManagementServerException, VirtualMachineMigrationException {
371-
when(_userVmManager.upgradeVirtualMachine(ArgumentMatchers.eq(TEST_VM_ID), ArgumentMatchers.eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture())).thenReturn(true);
376+
when(_userVmManager.upgradeVirtualMachine(eq(TEST_VM_ID), eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture())).thenReturn(true);
372377
_vmSnapshotMgr.changeUserVmServiceOffering(userVm, vmSnapshotVO);
373378
verify(_vmSnapshotMgr).getVmMapDetails(userVm);
374-
verify(_vmSnapshotMgr).upgradeUserVmServiceOffering(ArgumentMatchers.eq(userVm), ArgumentMatchers.eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture());
379+
verify(_vmSnapshotMgr).upgradeUserVmServiceOffering(eq(userVm), eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture());
375380
}
376381

377382
@Test(expected=CloudRuntimeException.class)
378383
public void testChangeUserVmServiceOfferingFailOnUpgradeVMServiceOffering() throws ConcurrentOperationException, ResourceUnavailableException, ManagementServerException, VirtualMachineMigrationException {
379-
when(_userVmManager.upgradeVirtualMachine(ArgumentMatchers.eq(TEST_VM_ID), ArgumentMatchers.eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture())).thenReturn(false);
384+
when(_userVmManager.upgradeVirtualMachine(eq(TEST_VM_ID), eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture())).thenReturn(false);
380385
_vmSnapshotMgr.changeUserVmServiceOffering(userVm, vmSnapshotVO);
381386
verify(_vmSnapshotMgr).getVmMapDetails(userVm);
382-
verify(_vmSnapshotMgr).upgradeUserVmServiceOffering(ArgumentMatchers.eq(userVm), ArgumentMatchers.eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture());
387+
verify(_vmSnapshotMgr).upgradeUserVmServiceOffering(eq(userVm), eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture());
383388
}
384389

385390
@Test
@@ -396,16 +401,27 @@ public void testUpgradeUserVmServiceOffering() throws ConcurrentOperationExcepti
396401

397402
@Test
398403
public void testRevertUserVmDetailsFromVmSnapshotNotDynamicServiceOffering() {
399-
_vmSnapshotMgr.revertUserVmDetailsFromVmSnapshot(vmMock, vmSnapshotVO);
404+
_vmSnapshotMgr.revertCustomServiceOfferingDetailsFromVmSnapshot(vmMock, vmSnapshotVO);
400405
verify(_vmSnapshotDetailsDao, never()).listDetails(anyLong());
401406
}
402407

403408
@Test
404409
public void testRevertUserVmDetailsFromVmSnapshotDynamicServiceOffering() {
405410
when(serviceOffering.isDynamic()).thenReturn(true);
406-
_vmSnapshotMgr.revertUserVmDetailsFromVmSnapshot(vmMock, vmSnapshotVO);
411+
VMSnapshotDetailsVO uefiSnapshotDetail = new VMSnapshotDetailsVO(VM_SNAPSHOT_ID, "UEFI", "SECURE", true);
412+
List<VMSnapshotDetailsVO> snapshotDetailsWithUefi = Arrays.asList(
413+
vmSnapshotDetailCpuNumber, vmSnapshotDetailMemory, uefiSnapshotDetail);
414+
when(_vmSnapshotDetailsDao.listDetails(VM_SNAPSHOT_ID)).thenReturn(snapshotDetailsWithUefi);
415+
416+
_vmSnapshotMgr.revertCustomServiceOfferingDetailsFromVmSnapshot(vmMock, vmSnapshotVO);
417+
407418
verify(_vmSnapshotDetailsDao).listDetails(VM_SNAPSHOT_ID);
408-
verify(_userVmDetailsDao).saveDetails(listUserVmDetailsCaptor.capture());
419+
verify(_userVmDetailsDao, never()).saveDetails(any());
420+
ArgumentCaptor<String> detailNameCaptor = ArgumentCaptor.forClass(String.class);
421+
verify(_userVmDetailsDao, times(2)).addDetail(eq(TEST_VM_ID), detailNameCaptor.capture(), anyString(), anyBoolean());
422+
List<String> appliedNames = detailNameCaptor.getAllValues();
423+
assertTrue(appliedNames.contains(VmDetailConstants.CPU_NUMBER));
424+
assertTrue(appliedNames.contains(VmDetailConstants.MEMORY));
425+
assertFalse("UEFI must not be applied from snapshot so that existing UEFI setting is preserved", appliedNames.contains("UEFI"));
409426
}
410-
411427
}

0 commit comments

Comments
 (0)