Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 39 additions & 2 deletions server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
@Inject
private VMTemplateZoneDao _templateZoneDao;
@Inject
private TemplateDataStoreDao _templateStoreDao;
protected TemplateDataStoreDao _templateStoreDao;
@Inject
private DomainDao _domainDao;
@Inject
Expand Down Expand Up @@ -1227,6 +1227,39 @@ public UserVm upgradeVirtualMachine(UpgradeVMCmd cmd) throws ResourceAllocationE
return userVm;
}

/**
Updates the instance details map with the current values of the instance for the CPU speed, memory, and CPU number if they have not been specified.
@param details Map containing the instance details.
@param vmInstance The virtual machine instance.
@param newServiceOfferingId The ID of the new service offering.
*/

protected void updateInstanceDetails (Map<String, String> details, VirtualMachine vmInstance, Long newServiceOfferingId) {
ServiceOfferingVO currentServiceOffering = serviceOfferingDao.findByIdIncludingRemoved(vmInstance.getId(), vmInstance.getServiceOfferingId());
ServiceOfferingVO newServiceOffering = serviceOfferingDao.findById(newServiceOfferingId);
updateInstanceDetailsKeepCurrentValueIfNull(newServiceOffering.getSpeed(), details, VmDetailConstants.CPU_SPEED, currentServiceOffering.getSpeed());
updateInstanceDetailsKeepCurrentValueIfNull(newServiceOffering.getRamSize(), details, VmDetailConstants.MEMORY, currentServiceOffering.getRamSize());
updateInstanceDetailsKeepCurrentValueIfNull(newServiceOffering.getCpu(), details, VmDetailConstants.CPU_NUMBER, currentServiceOffering.getCpu());
}

/**
* Updates a specific instance detail with the current instance value if the new value is null.
*
* @param newValue the new value to be set
* @param details a map of instance details
* @param detailsConstant the name of the detail constant to be updated
* @param currentValue the current value of the detail constant
*/

protected void updateInstanceDetailsKeepCurrentValueIfNull(Integer newValue, Map<String, String> details, String detailsConstant, Integer currentValue) {
if (newValue == null && details.get(detailsConstant) == null) {
String currentValueString = String.valueOf(currentValue);
logger.debug("{} was not specified, keeping the current value: {}.", detailsConstant, currentValueString);
details.put(detailsConstant, currentValueString);
}
}


private void validateOfferingMaxResource(ServiceOfferingVO offering) {
Integer maxCPUCores = ConfigurationManagerImpl.VM_SERVICE_OFFERING_MAX_CPU_CORES.value() == 0 ? Integer.MAX_VALUE: ConfigurationManagerImpl.VM_SERVICE_OFFERING_MAX_CPU_CORES.value();
if (offering.getCpu() > maxCPUCores) {
Expand Down Expand Up @@ -1892,7 +1925,11 @@ public UserVm upgradeVirtualMachine(ScaleVMCmd cmd) throws ResourceUnavailableEx
}
CallContext.current().setEventDetails("Vm Id: " + vm.getUuid());

boolean result = upgradeVirtualMachine(vmId, newServiceOfferingId, cmd.getDetails());
Map<String, String> cmdDetails = cmd.getDetails();

updateInstanceDetails(cmdDetails, vm, newServiceOfferingId);

boolean result = upgradeVirtualMachine(vmId, newServiceOfferingId, cmdDetails);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for late review.

@GaOrtiga
if the upgrade fail, should the details be restored to the original values ?

ping @DaanHoogland @GutoVeronezi , anyone has tested it ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @DaanHoogland @GutoVeronezi , anyone has tested it ?

Yes, it was tested.

if the upgrade fail, should the details be restored to the original values ?

The PR does not address changes to this behavior. it remains the same; we can discuss it in an issue or another PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @DaanHoogland @GutoVeronezi , anyone has tested it ?

Yes, it was tested.

Ok, I asked it because I did not see a comment mentioning it has been manually tested ok. If not, it would be good to be manually tested.
Why has tested it, except the author?

if the upgrade fail, should the details be restored to the original values ?

The PR does not address changes to this behavior. it remains the same; we can discuss it in an issue or another PR.

Do you think it is a regression of this PR we should address?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why has tested it, except the author?

I don't think I understood your question.

Do you think it is a regression of this PR we should address?

The PR does not address changes to method upgradeVirtualMachine; therefore, the topic you are questioning would not be a regression of this PR.

Copy link
Member

@weizhouapache weizhouapache Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why has tested it, except the author?

I don't think I understood your question.

Sorry typo.
Who has tested it, except the author?

Do you think it is a regression of this PR we should address?

The PR does not address changes to method upgradeVirtualMachine; therefore, the topic you are questioning would not be a regression of this PR.

please ignore my question regarding the vm details, They are not saved in database before upgrade, so no need to restore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who has tested it, except the author?

I tested it; I do not know if anybody else has tested it too.

please ignore my question regarding the vm details, They are not saved in database before upgrade, so no need to restore.

Yes. The only thing this PR does is facilitate calling the API. For instance, without this PR, if one has a VM with 1 vCPU and 1 GB RAM and wants to scale it to 2 GB RAM, one has to call the API with both the current vCPU count and the new RAM size. With this PR, one will only have to pass the new RAM size in the parameters; ACS will retrieve the current VM's vCPU count and will add it to the details map to be updated; thus, passing the current vCPU count and the new RAM size will have the same behavior as just passing the new RAM size. It just updates the details map (if necessary) before scaling the VM. This applies to RAM, frequency and vCPU.

I created PR #8673 to improve the methods names and docs.

if (result) {
UserVmVO vmInstance = _vmDao.findById(vmId);
if (vmInstance.getState().equals(State.Stopped)) {
Expand Down
76 changes: 76 additions & 0 deletions server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,9 @@ public class UserVmManagerImplTest {
@Mock
ServiceOfferingJoinDao serviceOfferingJoinDao;

@Mock
private VMInstanceVO vmInstanceMock;

private static final long vmId = 1l;
private static final long zoneId = 2L;
private static final long accountId = 3L;
Expand All @@ -273,6 +276,8 @@ public class UserVmManagerImplTest {

private Map<String, String> customParameters = new HashMap<>();

String[] detailsConstants = {VmDetailConstants.MEMORY, VmDetailConstants.CPU_NUMBER, VmDetailConstants.CPU_SPEED};

private DiskOfferingVO smallerDisdkOffering = prepareDiskOffering(5l * GiB_TO_BYTES, 1l, 1L, 2L);
private DiskOfferingVO largerDisdkOffering = prepareDiskOffering(10l * GiB_TO_BYTES, 2l, 10L, 20L);

Expand All @@ -289,6 +294,10 @@ public void beforeTest() {
CallContext.register(callerUser, callerAccount);

customParameters.put(VmDetailConstants.ROOT_DISK_SIZE, "123");
customParameters.put(VmDetailConstants.MEMORY, "2048");
customParameters.put(VmDetailConstants.CPU_NUMBER, "4");
customParameters.put(VmDetailConstants.CPU_SPEED, "1000");

lenient().doNothing().when(resourceLimitMgr).incrementResourceCount(anyLong(), any(Resource.ResourceType.class));
lenient().doNothing().when(resourceLimitMgr).decrementResourceCount(anyLong(), any(Resource.ResourceType.class), anyLong());

Expand Down Expand Up @@ -1395,4 +1404,71 @@ public void testRestoreVirtualMachineWithVMSnapshots() throws ResourceUnavailabl

userVmManagerImpl.restoreVirtualMachine(accountMock, vmId, newTemplateId);
}

@Test
public void updateInstanceDetailsKeepCurrentValueIfNullTestDetailsConstantIsNotNullDoNothing() {
int currentValue = 123;

for (String detailsConstant : detailsConstants) {
userVmManagerImpl.updateInstanceDetailsKeepCurrentValueIfNull(null, customParameters, detailsConstant, currentValue);
}

Assert.assertEquals(customParameters.get(VmDetailConstants.MEMORY), "2048");
Assert.assertEquals(customParameters.get(VmDetailConstants.CPU_NUMBER), "4");
Assert.assertEquals(customParameters.get(VmDetailConstants.CPU_SPEED), "1000");
}

@Test
public void updateInstanceDetailsKeepCurrentValueIfNullTestNewValueIsNotNullDoNothing() {
Map<String, String> details = new HashMap<>();
int currentValue = 123;

for (String detailsConstant : detailsConstants) {
userVmManagerImpl.updateInstanceDetailsKeepCurrentValueIfNull(321, details, detailsConstant, currentValue);
}

Assert.assertNull(details.get(VmDetailConstants.MEMORY));
Assert.assertNull(details.get(VmDetailConstants.CPU_NUMBER));
Assert.assertNull(details.get(VmDetailConstants.CPU_SPEED));
}

@Test
public void updateInstanceDetailsKeepCurrentValueIfNullTestBothValuesAreNullKeepCurrentValue() {
Map<String, String> details = new HashMap<>();
int currentValue = 123;

for (String detailsConstant : detailsConstants) {
userVmManagerImpl.updateInstanceDetailsKeepCurrentValueIfNull(null, details, detailsConstant, currentValue);
}

Assert.assertEquals(details.get(VmDetailConstants.MEMORY), String.valueOf(currentValue));
Assert.assertEquals(details.get(VmDetailConstants.CPU_NUMBER), String.valueOf(currentValue));
Assert.assertEquals(details.get(VmDetailConstants.CPU_SPEED),String.valueOf(currentValue));
}

@Test
public void updateInstanceDetailsKeepCurrentValueIfNullTestNeitherValueIsNullDoNothing() {
int currentValue = 123;

for (String detailsConstant : detailsConstants) {
userVmManagerImpl.updateInstanceDetailsKeepCurrentValueIfNull(321, customParameters, detailsConstant, currentValue);
}

Assert.assertEquals(customParameters.get(VmDetailConstants.MEMORY), "2048");
Assert.assertEquals(customParameters.get(VmDetailConstants.CPU_NUMBER), "4");
Assert.assertEquals(customParameters.get(VmDetailConstants.CPU_SPEED),"1000");
}

@Test
public void updateInstanceDetailsTestAllConstantsAreUpdated() {
Mockito.doReturn(serviceOffering).when(_serviceOfferingDao).findById(Mockito.anyLong());
Mockito.doReturn(1L).when(vmInstanceMock).getId();
Mockito.doReturn(1L).when(vmInstanceMock).getServiceOfferingId();
Mockito.doReturn(serviceOffering).when(_serviceOfferingDao).findByIdIncludingRemoved(Mockito.anyLong(), Mockito.anyLong());
userVmManagerImpl.updateInstanceDetails(null, vmInstanceMock, 0l);

Mockito.verify(userVmManagerImpl).updateInstanceDetailsKeepCurrentValueIfNull(Mockito.any(), Mockito.any(), Mockito.eq(VmDetailConstants.CPU_SPEED), Mockito.any());
Mockito.verify(userVmManagerImpl).updateInstanceDetailsKeepCurrentValueIfNull(Mockito.any(), Mockito.any(), Mockito.eq(VmDetailConstants.MEMORY), Mockito.any());
Mockito.verify(userVmManagerImpl).updateInstanceDetailsKeepCurrentValueIfNull(Mockito.any(), Mockito.any(), Mockito.eq(VmDetailConstants.CPU_NUMBER), Mockito.any());
}
}