From 89d915493fb83c5e0abe8db0fc1800bedc5be42b Mon Sep 17 00:00:00 2001 From: Fabricio Duarte Date: Fri, 27 Mar 2026 01:55:16 -0300 Subject: [PATCH 1/3] Fix NPE on external/unmanaged instance import using custom offerings (#12884) * Fix NPE on external/unmanaged instance import using custom offerings --- .../vm/UnmanagedVMsManagerImpl.java | 268 +++++++++++------- .../vm/UnmanagedVMsManagerImplTest.java | 118 +++++++- 2 files changed, 287 insertions(+), 99 deletions(-) diff --git a/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java b/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java index 75f87931591..18ba2d17179 100644 --- a/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java @@ -1387,10 +1387,7 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager { List managedVms = new ArrayList<>(additionalNameFilters); managedVms.addAll(getHostsManagedVms(hosts)); - List resourceLimitHostTags = resourceLimitService.getResourceLimitHostTags(serviceOffering, template); - try (CheckedReservation vmReservation = new CheckedReservation(owner, Resource.ResourceType.user_vm, resourceLimitHostTags, 1L, reservationDao, resourceLimitService); - CheckedReservation cpuReservation = new CheckedReservation(owner, Resource.ResourceType.cpu, resourceLimitHostTags, Long.valueOf(serviceOffering.getCpu()), reservationDao, resourceLimitService); - CheckedReservation memReservation = new CheckedReservation(owner, Resource.ResourceType.memory, resourceLimitHostTags, Long.valueOf(serviceOffering.getRamSize()), reservationDao, resourceLimitService)) { + try { ActionEventUtils.onStartedActionEvent(userId, owner.getId(), EventTypes.EVENT_VM_IMPORT, cmd.getEventDescription(), null, null, true, 0); @@ -1560,7 +1557,7 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager { String hostName, Account caller, Account owner, long userId, ServiceOfferingVO serviceOffering, Map dataDiskOfferingMap, Map nicNetworkMap, Map nicIpAddressMap, - Map details, Boolean migrateAllowed, List managedVms, boolean forced) { + Map details, Boolean migrateAllowed, List managedVms, boolean forced) throws ResourceAllocationException { UserVm userVm = null; for (HostVO host : hosts) { HashMap unmanagedInstances = getUnmanagedInstancesForHost(host, instanceName, managedVms); @@ -1604,11 +1601,18 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager { template.setGuestOSId(guestOSHypervisor.getGuestOsId()); } - userVm = importVirtualMachineInternal(unmanagedInstance, instanceName, zone, cluster, host, - template, displayName, hostName, CallContext.current().getCallingAccount(), owner, userId, - serviceOffering, dataDiskOfferingMap, - nicNetworkMap, nicIpAddressMap, - details, migrateAllowed, forced, true); + + List reservations = new ArrayList<>(); + try { + checkVmResourceLimitsForUnmanagedInstanceImport(owner, unmanagedInstance, serviceOffering, template, reservations); + userVm = importVirtualMachineInternal(unmanagedInstance, instanceName, zone, cluster, host, + template, displayName, hostName, CallContext.current().getCallingAccount(), owner, userId, + serviceOffering, dataDiskOfferingMap, + nicNetworkMap, nicIpAddressMap, + details, migrateAllowed, forced, true); + } finally { + ReservationHelper.closeAll(reservations); + } break; } if (userVm != null) { @@ -1618,6 +1622,36 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager { return userVm; } + protected void checkVmResourceLimitsForUnmanagedInstanceImport(Account owner, UnmanagedInstanceTO unmanagedInstance, ServiceOfferingVO serviceOffering, VMTemplateVO template, List reservations) throws ResourceAllocationException { + // When importing an unmanaged instance, the amount of CPUs and memory is obtained from the hypervisor unless powered off + // and not using a dynamic offering, unlike the external VM import that always obtains it from the compute offering + Integer cpu = serviceOffering.getCpu(); + Integer memory = serviceOffering.getRamSize(); + + if (serviceOffering.isDynamic() || !UnmanagedInstanceTO.PowerState.PowerOff.equals(unmanagedInstance.getPowerState())) { + cpu = unmanagedInstance.getCpuCores(); + memory = unmanagedInstance.getMemory(); + } + + if (cpu == null || cpu == 0) { + throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("CPU cores [%s] is not valid for importing VM [%s].", cpu, unmanagedInstance.getName())); + } + if (memory == null || memory == 0) { + throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("Memory [%s] is not valid for importing VM [%s].", memory, unmanagedInstance.getName())); + } + + List resourceLimitHostTags = resourceLimitService.getResourceLimitHostTags(serviceOffering, template); + + CheckedReservation vmReservation = new CheckedReservation(owner, Resource.ResourceType.user_vm, resourceLimitHostTags, 1L, reservationDao, resourceLimitService); + reservations.add(vmReservation); + + CheckedReservation cpuReservation = new CheckedReservation(owner, Resource.ResourceType.cpu, resourceLimitHostTags, cpu.longValue(), reservationDao, resourceLimitService); + reservations.add(cpuReservation); + + CheckedReservation memReservation = new CheckedReservation(owner, Resource.ResourceType.memory, resourceLimitHostTags, memory.longValue(), reservationDao, resourceLimitService); + reservations.add(memReservation); + } + private Pair getSourceVmwareUnmanagedInstance(String vcenter, String datacenterName, String username, String password, String clusterName, String sourceHostName, String sourceVM, ServiceOfferingVO serviceOffering) { @@ -1674,7 +1708,7 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager { Account caller, Account owner, long userId, ServiceOfferingVO serviceOffering, Map dataDiskOfferingMap, Map nicNetworkMap, Map nicIpAddressMap, - Map details, ImportVmCmd cmd, boolean forced) { + Map details, ImportVmCmd cmd, boolean forced) throws ResourceAllocationException { Long existingVcenterId = cmd.getExistingVcenterId(); String vcenter = cmd.getVcenter(); String datacenterName = cmd.getDatacenterName(); @@ -1719,6 +1753,8 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager { DataStoreTO temporaryConvertLocation = null; String ovfTemplateOnConvertLocation = null; ImportVmTask importVMTask = null; + List reservations = new ArrayList<>(); + try { HostVO convertHost = selectKVMHostForConversionInCluster(destinationCluster, convertInstanceHostId); HostVO importHost = selectKVMHostForImportingInCluster(destinationCluster, importInstanceHostId); @@ -1741,6 +1777,10 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager { Pair sourceInstanceDetails = getSourceVmwareUnmanagedInstance(vcenter, datacenterName, username, password, clusterName, sourceHostName, sourceVMName, serviceOffering); sourceVMwareInstance = sourceInstanceDetails.first(); isClonedInstance = sourceInstanceDetails.second(); + + // Ensure that the configured resource limits will not be exceeded before beginning the conversion process + checkVmResourceLimitsForUnmanagedInstanceImport(owner, sourceVMwareInstance, serviceOffering, template, reservations); + boolean isWindowsVm = sourceVMwareInstance.getOperatingSystem().toLowerCase().contains("windows"); if (isWindowsVm) { checkConversionSupportOnHost(convertHost, sourceVMName, true); @@ -1793,6 +1833,7 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager { if (temporaryConvertLocation != null && StringUtils.isNotBlank(ovfTemplateOnConvertLocation)) { removeTemplate(temporaryConvertLocation, ovfTemplateOnConvertLocation); } + ReservationHelper.closeAll(reservations); } } @@ -2581,10 +2622,7 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager { UserVm userVm = null; - List resourceLimitHostTags = resourceLimitService.getResourceLimitHostTags(serviceOffering, template); - try (CheckedReservation vmReservation = new CheckedReservation(owner, Resource.ResourceType.user_vm, resourceLimitHostTags, 1L, reservationDao, resourceLimitService); - CheckedReservation cpuReservation = new CheckedReservation(owner, Resource.ResourceType.cpu, resourceLimitHostTags, Long.valueOf(serviceOffering.getCpu()), reservationDao, resourceLimitService); - CheckedReservation memReservation = new CheckedReservation(owner, Resource.ResourceType.memory, resourceLimitHostTags, Long.valueOf(serviceOffering.getRamSize()), reservationDao, resourceLimitService)) { + try { if (ImportSource.EXTERNAL == importSource) { String username = cmd.getUsername(); @@ -2647,6 +2685,7 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager { List reservations = new ArrayList<>(); try { + checkVmResourceLimitsForExternalKvmVmImport(owner, serviceOffering, (VMTemplateVO) template, details, reservations); checkVolumeResourceLimitsForExternalKvmVmImport(owner, rootDisk, dataDisks, diskOffering, dataDiskOfferingMap, reservations); // Check NICs and supplied networks @@ -2811,101 +2850,138 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager { profiles.add(nicProfile); networkNicMap.put(network.getUuid(), profiles); + List reservations = new ArrayList<>(); try { + checkVmResourceLimitsForExternalKvmVmImport(owner, serviceOffering, (VMTemplateVO) template, details, reservations); userVm = userVmManager.importVM(zone, null, template, null, displayName, owner, null, caller, true, null, owner.getAccountId(), userId, serviceOffering, null, hostName, Hypervisor.HypervisorType.KVM, allDetails, powerState, networkNicMap); - } catch (InsufficientCapacityException ice) { + + if (userVm == null) { + throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("Failed to import vm name: %s", instanceName)); + } + + DiskOfferingVO diskOffering = diskOfferingDao.findById(serviceOffering.getDiskOfferingId()); + List resourceLimitStorageTags = resourceLimitService.getResourceLimitStorageTagsForResourceCountOperation(true, diskOffering); + CheckedReservation volumeReservation = new CheckedReservation(owner, Resource.ResourceType.volume, resourceLimitStorageTags, + CollectionUtils.isNotEmpty(resourceLimitStorageTags) ? 1L : 0L, reservationDao, resourceLimitService); + reservations.add(volumeReservation); + + String rootVolumeName = String.format("ROOT-%s", userVm.getId()); + DiskProfile diskProfile = volumeManager.allocateRawVolume(Volume.Type.ROOT, rootVolumeName, diskOffering, null, null, null, userVm, template, owner, null, false); + + final VirtualMachineProfile profile = new VirtualMachineProfileImpl(userVm, template, serviceOffering, owner, null); + ServiceOfferingVO dummyOffering = serviceOfferingDao.findById(userVm.getId(), serviceOffering.getId()); + profile.setServiceOffering(dummyOffering); + DeploymentPlanner.ExcludeList excludeList = new DeploymentPlanner.ExcludeList(); + final DataCenterDeployment plan = new DataCenterDeployment(zone.getId(), null, null, hostId, poolId, null); + DeployDestination dest = null; + try { + dest = deploymentPlanningManager.planDeployment(profile, plan, excludeList, null); + } catch (Exception e) { + logger.warn("Import failed for Vm: {} while finding deployment destination", userVm, e); + cleanupFailedImportVM(userVm); + throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("Import failed for Vm: %s while finding deployment destination", userVm.getInstanceName())); + } + if(dest == null) { + throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("Import failed for Vm: %s. Suitable deployment destination not found", userVm.getInstanceName())); + } + + Map storage = dest.getStorageForDisks(); + Volume volume = volumeDao.findById(diskProfile.getVolumeId()); + StoragePool storagePool = storage.get(volume); + CheckVolumeCommand checkVolumeCommand = new CheckVolumeCommand(); + checkVolumeCommand.setSrcFile(diskPath); + StorageFilerTO storageTO = new StorageFilerTO(storagePool); + checkVolumeCommand.setStorageFilerTO(storageTO); + Answer answer = agentManager.easySend(dest.getHost().getId(), checkVolumeCommand); + if (!(answer instanceof CheckVolumeAnswer)) { + cleanupFailedImportVM(userVm); + throw new CloudRuntimeException("Disk not found or is invalid"); + } + CheckVolumeAnswer checkVolumeAnswer = (CheckVolumeAnswer) answer; + try { + checkVolume(checkVolumeAnswer.getVolumeDetails()); + } catch (CloudRuntimeException e) { + cleanupFailedImportVM(userVm); + throw e; + } + if (!checkVolumeAnswer.getResult()) { + cleanupFailedImportVM(userVm); + throw new CloudRuntimeException("Disk not found or is invalid"); + } + diskProfile.setSize(checkVolumeAnswer.getSize()); + + CheckedReservation primaryStorageReservation = new CheckedReservation(owner, Resource.ResourceType.primary_storage, resourceLimitStorageTags, + CollectionUtils.isNotEmpty(resourceLimitStorageTags) ? diskProfile.getSize() : 0L, reservationDao, resourceLimitService); + reservations.add(primaryStorageReservation); + + List> diskProfileStoragePoolList = new ArrayList<>(); + try { + long deviceId = 1L; + if(ImportSource.SHARED == importSource) { + diskProfileStoragePoolList.add(importKVMSharedDisk(userVm, diskOffering, Volume.Type.ROOT, + template, deviceId, poolId, diskPath, diskProfile)); + } else if(ImportSource.LOCAL == importSource) { + diskProfileStoragePoolList.add(importKVMLocalDisk(userVm, diskOffering, Volume.Type.ROOT, + template, deviceId, hostId, diskPath, diskProfile)); + } + } catch (Exception e) { + logger.error(String.format("Failed to import volumes while importing vm: %s", instanceName), e); + cleanupFailedImportVM(userVm); + throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("Failed to import volumes while importing vm: %s. %s", instanceName, StringUtils.defaultString(e.getMessage()))); + } + networkOrchestrationService.importNic(macAddress, 0, network, true, userVm, requestedIpPair, zone, true); + publishVMUsageUpdateResourceCount(userVm, dummyOffering, template); + return userVm; + + } catch (InsufficientCapacityException ice) { // This will be thrown by com.cloud.vm.UserVmService.importVM logger.error(String.format("Failed to import vm name: %s", instanceName), ice); throw new ServerApiException(ApiErrorCode.INSUFFICIENT_CAPACITY_ERROR, ice.getMessage()); - } - if (userVm == null) { - throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("Failed to import vm name: %s", instanceName)); - } - - DiskOfferingVO diskOffering = diskOfferingDao.findById(serviceOffering.getDiskOfferingId()); - - List reservations = new ArrayList<>(); - List resourceLimitStorageTags = resourceLimitService.getResourceLimitStorageTagsForResourceCountOperation(true, diskOffering); - try { - CheckedReservation volumeReservation = new CheckedReservation(owner, Resource.ResourceType.volume, resourceLimitStorageTags, - CollectionUtils.isNotEmpty(resourceLimitStorageTags) ? 1L : 0L, reservationDao, resourceLimitService); - reservations.add(volumeReservation); - - String rootVolumeName = String.format("ROOT-%s", userVm.getId()); - DiskProfile diskProfile = volumeManager.allocateRawVolume(Volume.Type.ROOT, rootVolumeName, diskOffering, null, null, null, userVm, template, owner, null, false); - - final VirtualMachineProfile profile = new VirtualMachineProfileImpl(userVm, template, serviceOffering, owner, null); - ServiceOfferingVO dummyOffering = serviceOfferingDao.findById(userVm.getId(), serviceOffering.getId()); - profile.setServiceOffering(dummyOffering); - DeploymentPlanner.ExcludeList excludeList = new DeploymentPlanner.ExcludeList(); - final DataCenterDeployment plan = new DataCenterDeployment(zone.getId(), null, null, hostId, poolId, null); - DeployDestination dest = null; - try { - dest = deploymentPlanningManager.planDeployment(profile, plan, excludeList, null); - } catch (Exception e) { - logger.warn("Import failed for Vm: {} while finding deployment destination", userVm, e); - cleanupFailedImportVM(userVm); - throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("Import failed for Vm: %s while finding deployment destination", userVm.getInstanceName())); - } - if(dest == null) { - throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("Import failed for Vm: %s. Suitable deployment destination not found", userVm.getInstanceName())); - } - - Map storage = dest.getStorageForDisks(); - Volume volume = volumeDao.findById(diskProfile.getVolumeId()); - StoragePool storagePool = storage.get(volume); - CheckVolumeCommand checkVolumeCommand = new CheckVolumeCommand(); - checkVolumeCommand.setSrcFile(diskPath); - StorageFilerTO storageTO = new StorageFilerTO(storagePool); - checkVolumeCommand.setStorageFilerTO(storageTO); - Answer answer = agentManager.easySend(dest.getHost().getId(), checkVolumeCommand); - if (!(answer instanceof CheckVolumeAnswer)) { - cleanupFailedImportVM(userVm); - throw new CloudRuntimeException("Disk not found or is invalid"); - } - CheckVolumeAnswer checkVolumeAnswer = (CheckVolumeAnswer) answer; - try { - checkVolume(checkVolumeAnswer.getVolumeDetails()); - } catch (CloudRuntimeException e) { + } catch (ResourceAllocationException e) { cleanupFailedImportVM(userVm); throw e; - } - if (!checkVolumeAnswer.getResult()) { - cleanupFailedImportVM(userVm); - throw new CloudRuntimeException("Disk not found or is invalid"); - } - diskProfile.setSize(checkVolumeAnswer.getSize()); - - CheckedReservation primaryStorageReservation = new CheckedReservation(owner, Resource.ResourceType.primary_storage, resourceLimitStorageTags, - CollectionUtils.isNotEmpty(resourceLimitStorageTags) ? diskProfile.getSize() : 0L, reservationDao, resourceLimitService); - reservations.add(primaryStorageReservation); - - List> diskProfileStoragePoolList = new ArrayList<>(); - try { - long deviceId = 1L; - if(ImportSource.SHARED == importSource) { - diskProfileStoragePoolList.add(importKVMSharedDisk(userVm, diskOffering, Volume.Type.ROOT, - template, deviceId, poolId, diskPath, diskProfile)); - } else if(ImportSource.LOCAL == importSource) { - diskProfileStoragePoolList.add(importKVMLocalDisk(userVm, diskOffering, Volume.Type.ROOT, - template, deviceId, hostId, diskPath, diskProfile)); - } - } catch (Exception e) { - logger.error(String.format("Failed to import volumes while importing vm: %s", instanceName), e); - cleanupFailedImportVM(userVm); - throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("Failed to import volumes while importing vm: %s. %s", instanceName, StringUtils.defaultString(e.getMessage()))); - } - networkOrchestrationService.importNic(macAddress, 0, network, true, userVm, requestedIpPair, zone, true); - publishVMUsageUpdateResourceCount(userVm, dummyOffering, template); - return userVm; - } finally { ReservationHelper.closeAll(reservations); } } + protected void checkVmResourceLimitsForExternalKvmVmImport(Account owner, ServiceOfferingVO serviceOffering, VMTemplateVO template, Map details, List reservations) throws ResourceAllocationException { + // When importing an external VM, the amount of CPUs and memory is always obtained from the compute offering, + // unlike the unmanaged instance import that obtains it from the hypervisor unless the VM is powered off and the offering is fixed + Integer cpu = serviceOffering.getCpu(); + Integer memory = serviceOffering.getRamSize(); + + if (serviceOffering.isDynamic()) { + cpu = getDetailAsInteger(VmDetailConstants.CPU_NUMBER, details); + memory = getDetailAsInteger(VmDetailConstants.MEMORY, details); + } + + List resourceLimitHostTags = resourceLimitService.getResourceLimitHostTags(serviceOffering, template); + + CheckedReservation vmReservation = new CheckedReservation(owner, Resource.ResourceType.user_vm, resourceLimitHostTags, 1L, reservationDao, resourceLimitService); + reservations.add(vmReservation); + + CheckedReservation cpuReservation = new CheckedReservation(owner, Resource.ResourceType.cpu, resourceLimitHostTags, cpu.longValue(), reservationDao, resourceLimitService); + reservations.add(cpuReservation); + + CheckedReservation memReservation = new CheckedReservation(owner, Resource.ResourceType.memory, resourceLimitHostTags, memory.longValue(), reservationDao, resourceLimitService); + reservations.add(memReservation); + } + + protected Integer getDetailAsInteger(String key, Map details) { + String detail = details.get(key); + if (detail == null) { + throw new InvalidParameterValueException(String.format("Detail '%s' must be provided.", key)); + } + try { + return Integer.valueOf(detail); + } catch (NumberFormatException e) { + throw new InvalidParameterValueException(String.format("Please provide a valid integer value for detail '%s'.", key)); + } + } + private void checkVolume(Map volumeDetails) { if (MapUtils.isEmpty(volumeDetails)) { return; diff --git a/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java b/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java index 9935a228c5b..544c37d57e9 100644 --- a/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java @@ -38,9 +38,11 @@ import java.util.List; import java.util.Map; import java.util.UUID; +import com.cloud.exception.ResourceAllocationException; import com.cloud.offering.DiskOffering; import com.cloud.resourcelimit.CheckedReservation; import com.cloud.vm.ImportVMTaskVO; +import com.cloud.vm.VmDetailConstants; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.ResponseGenerator; import org.apache.cloudstack.api.ResponseObject; @@ -60,6 +62,7 @@ import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager; import org.apache.cloudstack.framework.config.ConfigKey; import org.apache.cloudstack.framework.config.dao.ConfigurationDao; import org.apache.cloudstack.reservation.dao.ReservationDao; +import org.apache.cloudstack.resourcelimit.Reserver; import org.apache.cloudstack.storage.datastore.db.ImageStoreDao; import org.apache.cloudstack.storage.datastore.db.ImageStoreVO; import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; @@ -167,7 +170,6 @@ import com.cloud.vm.UserVmVO; import com.cloud.vm.VMInstanceDetailVO; import com.cloud.vm.VMInstanceVO; import com.cloud.vm.VirtualMachine; -import com.cloud.vm.VmDetailConstants; import com.cloud.vm.dao.NicDao; import com.cloud.vm.dao.UserVmDao; import com.cloud.vm.dao.VMInstanceDao; @@ -178,7 +180,7 @@ public class UnmanagedVMsManagerImplTest { @Spy @InjectMocks - private UnmanagedVMsManagerImpl unmanagedVMsManager = new UnmanagedVMsManagerImpl(); + private UnmanagedVMsManagerImpl unmanagedVMsManager; @Mock private UserVmManager userVmManager; @@ -255,11 +257,18 @@ public class UnmanagedVMsManagerImplTest { ImportVMTaskVO importVMTaskVO; @Mock private VMInstanceDetailsDao vmInstanceDetailsDao; - @Mock private ConfigKey configKeyMockParamsAllowed; @Mock private ConfigKey configKeyMockParamsAllowedList; + @Mock + private Account accountMock; + @Mock + private ServiceOfferingVO serviceOfferingMock; + @Mock + private VMTemplateVO templateMock; + @Mock + private UnmanagedInstanceTO unmanagedInstanceMock; private static final long virtualMachineId = 1L; @@ -386,6 +395,11 @@ public class UnmanagedVMsManagerImplTest { when(vmDao.findById(virtualMachineId)).thenReturn(virtualMachine); when(virtualMachine.getState()).thenReturn(VirtualMachine.State.Running); + + when(unmanagedInstanceMock.getCpuCores()).thenReturn(8); + when(unmanagedInstanceMock.getMemory()).thenReturn(4096); + when(serviceOfferingMock.getCpu()).thenReturn(4); + when(serviceOfferingMock.getRamSize()).thenReturn(2048); } @NotNull @@ -1321,4 +1335,102 @@ public class UnmanagedVMsManagerImplTest { Assert.assertFalse(params.containsKey(VmDetailConstants.CPU_SPEED)); Assert.assertFalse(params.containsKey(VmDetailConstants.MEMORY)); } + + @Test + public void checkVmResourceLimitsForUnmanagedInstanceImportTestUsesInformationFromHypervisorWhenOfferingIsDynamic() throws Exception { + when(serviceOfferingMock.isDynamic()).thenReturn(true); + List reservations = new ArrayList<>(); + + try (MockedConstruction mockedConstruction = Mockito.mockConstruction(CheckedReservation.class)) { + unmanagedVMsManager.checkVmResourceLimitsForUnmanagedInstanceImport(accountMock, unmanagedInstanceMock, serviceOfferingMock, templateMock, reservations); + + Assert.assertEquals(3, mockedConstruction.constructed().size()); + Assert.assertEquals(3, reservations.size()); + verify(unmanagedInstanceMock).getCpuCores(); + verify(unmanagedInstanceMock).getMemory(); + } + } + + @Test + public void checkVmResourceLimitsForUnmanagedInstanceImportTestUsesInformationFromHypervisorWhenVmIsPoweredOn() throws Exception { + when(unmanagedInstanceMock.getPowerState()).thenReturn(UnmanagedInstanceTO.PowerState.PowerOn); + when(serviceOfferingMock.isDynamic()).thenReturn(false); + List reservations = new ArrayList<>(); + + try (MockedConstruction mockedConstruction = Mockito.mockConstruction(CheckedReservation.class)) { + unmanagedVMsManager.checkVmResourceLimitsForUnmanagedInstanceImport(accountMock, unmanagedInstanceMock, serviceOfferingMock, templateMock, reservations); + + Assert.assertEquals(3, mockedConstruction.constructed().size()); + Assert.assertEquals(3, reservations.size()); + verify(unmanagedInstanceMock).getCpuCores(); + verify(unmanagedInstanceMock).getMemory(); + } + } + + @Test + public void checkVmResourceLimitsForUnmanagedInstanceImportTestUsesInformationFromOfferingWhenOfferingIsNotDynamicAndVmIsPoweredOff() throws Exception { + when(unmanagedInstanceMock.getPowerState()).thenReturn(UnmanagedInstanceTO.PowerState.PowerOff); + when(serviceOfferingMock.isDynamic()).thenReturn(false); + List reservations = new ArrayList<>(); + + try (MockedConstruction mockedConstruction = Mockito.mockConstruction(CheckedReservation.class)) { + unmanagedVMsManager.checkVmResourceLimitsForUnmanagedInstanceImport(accountMock, unmanagedInstanceMock, serviceOfferingMock, templateMock, reservations); + + Assert.assertEquals(3, mockedConstruction.constructed().size()); + Assert.assertEquals(3, reservations.size()); + verify(serviceOfferingMock).getCpu(); + verify(serviceOfferingMock).getRamSize(); + verify(unmanagedInstanceMock, Mockito.never()).getCpuCores(); + verify(unmanagedInstanceMock, Mockito.never()).getMemory(); + } + } + + @Test + public void checkVmResourceLimitsForExternalKvmVmImportTestUsesInformationFromOfferingWhenOfferingIsNotDynamic() throws ResourceAllocationException { + when(serviceOfferingMock.isDynamic()).thenReturn(false); + Map details = new HashMap<>(); + List reservations = new ArrayList<>(); + + try (MockedConstruction mockedConstruction = Mockito.mockConstruction(CheckedReservation.class)) { + unmanagedVMsManager.checkVmResourceLimitsForExternalKvmVmImport(accountMock, serviceOfferingMock, templateMock, details, reservations); + + Assert.assertEquals(3, mockedConstruction.constructed().size()); + Assert.assertEquals(3, reservations.size()); + verify(serviceOfferingMock).getCpu(); + verify(serviceOfferingMock).getRamSize(); + verify(unmanagedVMsManager, Mockito.never()).getDetailAsInteger(VmDetailConstants.CPU_NUMBER, details); + verify(unmanagedVMsManager, Mockito.never()).getDetailAsInteger(VmDetailConstants.MEMORY, details); + } + } + + @Test + public void checkVmResourceLimitsForExternalKvmVmImportTestUsesInformationFromDetailsWhenOfferingIsDynamic() throws ResourceAllocationException { + when(serviceOfferingMock.isDynamic()).thenReturn(true); + Map details = new HashMap<>(); + details.put(VmDetailConstants.CPU_NUMBER, "8"); + details.put(VmDetailConstants.MEMORY, "4096"); + List reservations = new ArrayList<>(); + + try (MockedConstruction mockedConstruction = Mockito.mockConstruction(CheckedReservation.class)) { + unmanagedVMsManager.checkVmResourceLimitsForExternalKvmVmImport(accountMock, serviceOfferingMock, templateMock, details, reservations); + + Assert.assertEquals(3, mockedConstruction.constructed().size()); + Assert.assertEquals(3, reservations.size()); + verify(unmanagedVMsManager).getDetailAsInteger(VmDetailConstants.CPU_NUMBER, details); + verify(unmanagedVMsManager).getDetailAsInteger(VmDetailConstants.MEMORY, details); + } + } + + @Test(expected = InvalidParameterValueException.class) + public void getDetailAsIntegerTestThrowsInvalidParameterValueExceptionWhenDetailIsNull() { + Map details = new HashMap<>(); + unmanagedVMsManager.getDetailAsInteger("non-existent", details); + } + + @Test(expected = InvalidParameterValueException.class) + public void getDetailAsIntegerTestThrowsInvalidParameterValueExceptionWhenValueIsInvalid() { + Map details = new HashMap<>(); + details.put("key", "not-a-number"); + unmanagedVMsManager.getDetailAsInteger("key", details); + } } From a127a26ebd3cb2b3cdfed3e8a6b5f1a2c855a5cd Mon Sep 17 00:00:00 2001 From: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com> Date: Fri, 27 Mar 2026 10:19:52 +0530 Subject: [PATCH 2/3] Fix Revert Instance to Snapshot with custom service offering (#12885) * Fix revertVM with custom svc offering --- .../vm/snapshot/VMSnapshotManagerImpl.java | 54 +++++++++++----- .../vm/snapshot/VMSnapshotManagerTest.java | 62 ++++++++++++++----- 2 files changed, 87 insertions(+), 29 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 b2e820bce94..fe91e1d9caa 100644 --- a/server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java @@ -751,11 +751,17 @@ public class VMSnapshotManagerImpl extends MutualExclusiveIdsManagerBase impleme "VM Snapshot reverting failed due to vm is not in the state of Running or Stopped."); } - 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( - "VM Snapshot revert not allowed. This will result in VM state change. You can revert running VM to disk and memory type snapshot and stopped VM to disk type" - + " 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 @@ -808,20 +814,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 changeUserVmServiceOffering 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 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; } /** @@ -941,7 +963,9 @@ public class VMSnapshotManagerImpl extends MutualExclusiveIdsManagerBase impleme Transaction.execute(new TransactionCallbackWithExceptionNoReturn() { @Override public void doInTransactionWithoutResult(TransactionStatus status) throws CloudRuntimeException { - updateUserVmServiceOffering(userVm, vmSnapshotVo); + if (userVmServiceOfferingNeedsChange(userVm, vmSnapshotVo)) { + changeUserVmServiceOffering(userVm, vmSnapshotVo); + } revertUserVmDetailsFromVmSnapshot(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 7b12fc4a99c..88af1a6325d 100644 --- a/server/src/test/java/com/cloud/vm/snapshot/VMSnapshotManagerTest.java +++ b/server/src/test/java/com/cloud/vm/snapshot/VMSnapshotManagerTest.java @@ -67,7 +67,6 @@ import org.junit.Before; 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; @@ -79,9 +78,12 @@ import java.util.List; 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.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; @@ -223,6 +225,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("cpuNumber"); @@ -340,20 +343,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(ArgumentMatchers.eq(TEST_VM_ID), ArgumentMatchers.eq(SERVICE_OFFERING_ID), mapDetailsCaptor.capture())).thenReturn(true); - _vmSnapshotMgr.updateUserVmServiceOffering(userVm, vmSnapshotVO); + public void testUserVmServiceOfferingNeedsChangeWhenSameNonDynamicOffering() { + assertFalse(_vmSnapshotMgr.userVmServiceOfferingNeedsChange(userVm, vmSnapshotVO)); - verify(_vmSnapshotMgr).changeUserVmServiceOffering(userVm, vmSnapshotVO); + verify(_serviceOfferingDao).findByIdIncludingRemoved(TEST_VM_ID, SERVICE_OFFERING_ID); + verify(_serviceOfferingDao, never()).getComputeOffering(any(ServiceOfferingVO.class), any()); + } + + @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(ArgumentMatchers.eq(userVm), ArgumentMatchers.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 @@ -368,18 +402,18 @@ public class VMSnapshotManagerTest { @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(vmSnapshotVO); - 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(vmSnapshotVO); - 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 From 2eef7aa9a272a1346e5caea3c3a84f3f5264afed Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Thu, 30 Apr 2026 13:52:39 +0200 Subject: [PATCH 3/3] adding default deny keys also when there are no other keys --- .../extensions/manager/ExtensionsManagerImpl.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/framework/extensions/src/main/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImpl.java b/framework/extensions/src/main/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImpl.java index 1422338ddc9..f6fd08b6da2 100644 --- a/framework/extensions/src/main/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImpl.java +++ b/framework/extensions/src/main/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImpl.java @@ -1663,14 +1663,14 @@ public class ExtensionsManagerImpl extends ManagerBase implements ExtensionsMana public List getExtensionReservedResourceDetails(long extensionId) { ExtensionDetailsVO detailsVO = extensionDetailsDao.findDetail(extensionId, ApiConstants.RESERVED_RESOURCE_DETAILS); - if (detailsVO == null || !StringUtils.isNotBlank(detailsVO.getValue())) { - return Collections.emptyList(); - } List reservedDetails = new ArrayList<>(); - String[] parts = detailsVO.getValue().split(","); - for (String part : parts) { - if (StringUtils.isNotBlank(part)) { - reservedDetails.add(part.trim()); + if (detailsVO != null && StringUtils.isNotBlank(detailsVO.getValue())) { + String[] parts = detailsVO.getValue().split(","); + for (String part : parts) { + String trimmedPart = part.trim(); + if (StringUtils.isNotBlank(trimmedPart)) { + reservedDetails.add(trimmedPart); + } } } addInbuiltExtensionReservedResourceDetails(extensionId, reservedDetails);