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 1b588b042105..d1265562deba 100644 --- a/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java @@ -1352,10 +1352,7 @@ private UserVmResponse baseImportInstance(ImportUnmanagedInstanceCmd cmd) { 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); @@ -1497,7 +1494,7 @@ private UserVm importUnmanagedInstanceFromHypervisor(DataCenter zone, Cluster cl 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); @@ -1541,11 +1538,18 @@ private UserVm importUnmanagedInstanceFromHypervisor(DataCenter zone, Cluster cl 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) { @@ -1555,6 +1559,36 @@ private UserVm importUnmanagedInstanceFromHypervisor(DataCenter zone, Cluster cl 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) { @@ -1582,7 +1616,7 @@ protected UserVm importUnmanagedInstanceFromVmwareToKvm(DataCenter zone, Cluster 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(); @@ -1620,6 +1654,8 @@ protected UserVm importUnmanagedInstanceFromVmwareToKvm(DataCenter zone, Cluster UnmanagedInstanceTO sourceVMwareInstance = null; DataStoreTO temporaryConvertLocation = null; String ovfTemplateOnConvertLocation = null; + List reservations = new ArrayList<>(); + try { HostVO convertHost = selectKVMHostForConversionInCluster(destinationCluster, convertInstanceHostId); HostVO importHost = selectKVMHostForImportingInCluster(destinationCluster, importInstanceHostId); @@ -1634,6 +1670,10 @@ protected UserVm importUnmanagedInstanceFromVmwareToKvm(DataCenter zone, Cluster Pair sourceInstanceDetails = getSourceVmwareUnmanagedInstance(vcenter, datacenterName, username, password, clusterName, sourceHostName, sourceVMName); 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); @@ -1681,6 +1721,7 @@ protected UserVm importUnmanagedInstanceFromVmwareToKvm(DataCenter zone, Cluster if (temporaryConvertLocation != null && StringUtils.isNotBlank(ovfTemplateOnConvertLocation)) { removeTemplate(temporaryConvertLocation, ovfTemplateOnConvertLocation); } + ReservationHelper.closeAll(reservations); } } @@ -2400,10 +2441,7 @@ private UserVmResponse importKvmInstance(ImportVmCmd cmd) { 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(); @@ -2466,6 +2504,7 @@ private UserVm importExternalKvmVirtualMachine(final UnmanagedInstanceTO unmanag List reservations = new ArrayList<>(); try { + checkVmResourceLimitsForExternalKvmVmImport(owner, serviceOffering, (VMTemplateVO) template, details, reservations); checkVolumeResourceLimitsForExternalKvmVmImport(owner, rootDisk, dataDisks, diskOffering, dataDiskOfferingMap, reservations); // Check NICs and supplied networks @@ -2630,24 +2669,20 @@ private UserVm importKvmVirtualMachineFromDisk(final ImportSource importSource, 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) { - 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()); + if (userVm == null) { + throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, String.format("Failed to import vm name: %s", instanceName)); + } - List reservations = new ArrayList<>(); - List resourceLimitStorageTags = resourceLimitService.getResourceLimitStorageTagsForResourceCountOperation(true, diskOffering); - try { + 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); @@ -2720,6 +2755,9 @@ private UserVm importKvmVirtualMachineFromDisk(final ImportSource importSource, 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()); } catch (ResourceAllocationException e) { cleanupFailedImportVM(userVm); throw e; @@ -2728,6 +2766,41 @@ private UserVm importKvmVirtualMachineFromDisk(final ImportSource importSource, } } + 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 d56299126a52..be513e8b7177 100644 --- a/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java @@ -37,8 +37,10 @@ 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.VmDetailConstants; import org.apache.cloudstack.api.ResponseGenerator; import org.apache.cloudstack.api.ResponseObject; import org.apache.cloudstack.api.ServerApiException; @@ -55,6 +57,7 @@ import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager; import org.apache.cloudstack.framework.config.dao.ConfigurationDao; +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; @@ -72,6 +75,7 @@ import org.mockito.MockedStatic; import org.mockito.Mockito; import org.mockito.MockitoAnnotations; +import org.mockito.Spy; import org.mockito.junit.MockitoJUnitRunner; import com.cloud.agent.AgentManager; @@ -168,7 +172,8 @@ public class UnmanagedVMsManagerImplTest { @InjectMocks - private UnmanagedVMsManagerImpl unmanagedVMsManager = new UnmanagedVMsManagerImpl(); + @Spy + private UnmanagedVMsManagerImpl unmanagedVMsManager; @Mock private UserVmManager userVmManager; @@ -237,6 +242,14 @@ public class UnmanagedVMsManagerImplTest { EntityManager entityMgr; @Mock DeploymentPlanningManager deploymentPlanningManager; + @Mock + private Account accountMock; + @Mock + private ServiceOfferingVO serviceOfferingMock; + @Mock + private VMTemplateVO templateMock; + @Mock + private UnmanagedInstanceTO unmanagedInstanceMock; private static final long virtualMachineId = 1L; @@ -363,6 +376,11 @@ public void setUp() throws Exception { 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 @@ -1110,4 +1128,102 @@ public void testSelectKVMHostForConversionInClusterWithImportInstanceIdInvalidHo unmanagedVMsManager.selectKVMHostForConversionInCluster(cluster, hostId); } + + @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); + } }