Copilot commented on code in PR #12793:
URL: https://github.com/apache/cloudstack/pull/12793#discussion_r3217896899
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -9508,6 +9508,16 @@ public UserVm importVM(final DataCenter zone, final Host
host, final VirtualMach
if (host == null && hypervisorType == HypervisorType.VMware) {
throw new InvalidParameterValueException("Unable to import
virtual machine with invalid host");
}
+ if (template == null) {
+ throw new InvalidParameterValueException("Unable to import
virtual machine without a template");
+ }
+
+ // Ensure template details are loaded so that commitUserVm can
copy them into the VM's details map
+ VMTemplateVO vmTemplateVO =
_templateDao.findById(template.getId());
+ if (vmTemplateVO == null) {
+ throw new InvalidParameterValueException("Unable to find
template with id " + template.getId() + " for virtual machine import");
+ }
+ _templateDao.loadDetails(vmTemplateVO);
Review Comment:
This introduces an unconditional DB lookup (`findById`) even if the provided
`template` is already a `VMTemplateVO` instance (or already has details
loaded). Consider reusing/casting the existing `template` when possible and
only querying when necessary, or moving the `loadDetails` responsibility into
`commitUserVm` to centralize the invariant and avoid duplicate lookups across
call sites.
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -9521,8 +9531,8 @@ public UserVm importVM(final DataCenter zone, final Host
host, final VirtualMach
final String uuidName = _uuidMgr.generateUuid(UserVm.class, null);
final Host lastHost = powerState !=
VirtualMachine.PowerState.PowerOn ? host : null;
- final Boolean dynamicScalingEnabled =
checkIfDynamicScalingCanBeEnabled(null, serviceOffering, template,
zone.getId());
- return commitUserVm(true, zone, host, lastHost, template,
hostName, displayName, owner,
+ final Boolean dynamicScalingEnabled =
checkIfDynamicScalingCanBeEnabled(null, serviceOffering, vmTemplateVO,
zone.getId());
+ return commitUserVm(true, zone, host, lastHost, vmTemplateVO,
hostName, displayName, owner,
null, null, userData, null, null, isDisplayVm, keyboard,
accountId, userId, serviceOffering,
template.getFormat().equals(ImageFormat.ISO), sshPublicKeys, networkNicMap,
Review Comment:
The ISO flag is computed from `template.getFormat()` while the rest of the
flow now relies on `vmTemplateVO`. If `template` is not the same DB-backed
object (or has incomplete fields), this can lead to inconsistent behavior.
Compute the ISO flag from `vmTemplateVO` instead, and prefer
`ImageFormat.ISO.equals(...)` to avoid potential NPEs.
--
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]
