Copilot commented on code in PR #12884:
URL: https://github.com/apache/cloudstack/pull/12884#discussion_r2985297747
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -2728,6 +2766,41 @@ private UserVm importKvmVirtualMachineFromDisk(final
ImportSource importSource,
}
}
+ private void checkVmResourceLimitsForExternalKvmVmImport(Account owner,
ServiceOfferingVO serviceOffering, VMTemplateVO template, Map<String, String>
details, List<Reserver> 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<String> 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);
+ }
Review Comment:
New VM resource-limit selection logic (dynamic vs fixed offering,
powered-off vs running unmanaged VM, and parsing CPU/memory from `details`) is
not covered by unit tests in `UnmanagedVMsManagerImplTest`. Add focused tests
for: (1) unmanaged import with null/unknown powerState, (2) external import
with dynamic offering reading `cpuNumber`/`memory`, and (3) validation failures
for missing/invalid values, to prevent regressions back to NPEs.
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -1634,6 +1670,10 @@ protected UserVm
importUnmanagedInstanceFromVmwareToKvm(DataCenter zone, Cluster
Pair<UnmanagedInstanceTO, Boolean> 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 beggining the conversion process
Review Comment:
Typo in comment: “beggining” → “beginning”.
```suggestion
// Ensure that the configured resource limits will not be
exceeded before beginning the conversion process
```
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -2728,6 +2766,41 @@ private UserVm importKvmVirtualMachineFromDisk(final
ImportSource importSource,
}
}
+ private void checkVmResourceLimitsForExternalKvmVmImport(Account owner,
ServiceOfferingVO serviceOffering, VMTemplateVO template, Map<String, String>
details, List<Reserver> 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<String> 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);
Review Comment:
`checkVmResourceLimitsForExternalKvmVmImport` can still throw an NPE: if
`serviceOffering.getCpu()` / `getRamSize()` are null (e.g., custom offering
metadata) and the offering is not treated as dynamic here, `cpu.longValue()` /
`memory.longValue()` will crash. Add explicit validation (null/<=0) for cpu and
memory (both for dynamic and non-dynamic) and throw a parameter/internal error
before constructing reservations.
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -1555,6 +1559,36 @@ private UserVm
importUnmanagedInstanceFromHypervisor(DataCenter zone, Cluster cl
return userVm;
}
+ private void checkVmResourceLimitsForUnmanagedInstanceImport(Account
owner, UnmanagedInstanceTO unmanagedInstance, ServiceOfferingVO
serviceOffering, VMTemplateVO template, List<Reserver> reservations) throws
ResourceAllocationException {
+ // When importing a 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() ||
!unmanagedInstance.getPowerState().equals(UnmanagedInstanceTO.PowerState.PowerOff))
{
Review Comment:
`unmanagedInstance.getPowerState()` can be null (see UnmanagedInstanceTO),
so calling `.equals(PowerOff)` here can throw an NPE during import. Make this
null-safe (e.g., compare with `PowerOff.equals(...)` or treat null/unknown as
not-PowerOff) so the resource-limit check can’t crash the import path.
```suggestion
if (serviceOffering.isDynamic() ||
!UnmanagedInstanceTO.PowerState.PowerOff.equals(unmanagedInstance.getPowerState()))
{
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
