Copilot commented on code in PR #13263:
URL: https://github.com/apache/cloudstack/pull/13263#discussion_r3316082636
##########
server/src/main/java/org/apache/cloudstack/vm/ImportVmTasksManagerImpl.java:
##########
@@ -99,14 +100,16 @@ private ImportVmTask.TaskState getStateFromFilter(String
tasksFilter) {
try {
return ImportVmTask.TaskState.getValue(tasksFilter);
} catch (IllegalArgumentException e) {
- throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
String.format("Invalid value for task state: %s", tasksFilter));
+ throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
+ String.format("Invalid value for task state: %s",
tasksFilter));
}
}
@Override
- public ImportVmTask createImportVMTaskRecord(DataCenter zone, Account
owner, long userId, String displayName, String vcenter, String datacenterName,
String sourceVMName, Host convertHost, Host importHost) {
+ public ImportVmTask createImportVMTaskRecord(DataCenter zone, Account
owner, long userId, String displayName,
+ String vcenter, String datacenterName, String sourceVMName, Host
convertHost, Host importHost) {
logger.debug("Creating import VM task entry for VM: {} for account {}
on zone {} " +
- "from the vCenter: {} / datacenter: {} / source VM:
{}",
+ "from the vCenter: {} / datacenter: {} / source VM: {}",
sourceVMName, owner.getAccountName(), zone.getName(),
displayName, vcenter, datacenterName);
Review Comment:
The placeholder arguments in this debug log are mis-ordered: the message
expects (vCenter, datacenter, source VM) but the code passes (displayName,
vcenter, datacenterName). This will log incorrect values and make
troubleshooting harder; please reorder the arguments (and/or adjust the
message) so each placeholder matches the intended field.
##########
server/src/main/java/org/apache/cloudstack/vm/ImportVmTasksManagerImpl.java:
##########
@@ -139,11 +147,12 @@ private String getStepDescription(ImportVMTaskVO
importVMTaskVO, Host convertHos
@Override
public void updateImportVMTaskStep(ImportVmTask importVMTask, DataCenter
zone, Account owner, Host convertHost,
- Host importHost, Long vmId,
ImportVmTask.Step step) {
+ Host importHost, Long vmId, ImportVmTask.Step step) {
ImportVMTaskVO importVMTaskVO = (ImportVMTaskVO) importVMTask;
logger.debug("Updating import VM task entry for VM: {} for account {}
on zone {} " +
- "from the vCenter: {} / datacenter: {} / source VM: {}
to step: {}",
- importVMTaskVO.getSourceVMName(), owner.getAccountName(),
zone.getName(), importVMTaskVO.getDisplayName(),
+ "from the vCenter: {} / datacenter: {} / source VM: {} to
step: {}",
+ importVMTaskVO.getSourceVMName(), owner.getAccountName(),
zone.getName(),
+ importVMTaskVO.getDisplayName(),
importVMTaskVO.getVcenter(), importVMTaskVO.getDatacenter(),
step);
Review Comment:
This debug log also appears to pass arguments in the wrong order: the
message includes placeholders for vCenter, datacenter, and source VM, but
`importVMTaskVO.getDisplayName()` is provided where the vCenter is expected,
shifting the remaining values. Please realign the arguments with the
placeholders to avoid misleading log output.
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -1755,48 +2014,62 @@ protected UserVm
importUnmanagedInstanceFromVmwareToKvm(DataCenter zone, Cluster
serviceOffering, dataDiskOfferingMap,
temporaryConvertLocation,
ovfTemplateOnConvertLocation, forceConvertToPool,
extraParams);
} else {
- // Uses KVM Host for direct conversion using VDDK, or for OVF
export to temporary conversion location through ovftool
- importVmTasksManager.updateImportVMTaskStep(importVMTask,
zone, owner, convertHost, importHost, null, ConvertingInstance);
+ // Uses KVM Host for direct conversion using VDDK, or for OVF
export to
+ // temporary conversion location through ovftool
+ importVmTasksManager.updateImportVMTaskStep(importVMTask,
zone, owner, convertHost, importHost, null,
+ ConvertingInstance);
convertedInstance =
convertVmwareInstanceToKVMUsingVDDKOrAfterExportingOVFToConvertLocation(
sourceVMName, sourceVMwareInstance, convertHost,
importHost,
convertStoragePools, serviceOffering,
dataDiskOfferingMap,
- temporaryConvertLocation, vcenter, username, password,
datacenterName, forceConvertToPool, extraParams, useVddk, details);
+ temporaryConvertLocation, vcenter, username, password,
datacenterName, forceConvertToPool,
+ extraParams, useVddk, details);
}
sanitizeConvertedInstance(convertedInstance, sourceVMwareInstance);
- importVmTasksManager.updateImportVMTaskStep(importVMTask, zone,
owner, convertHost, importHost, null, Importing);
+ importVmTasksManager.updateImportVMTaskStep(importVMTask, zone,
owner, convertHost, importHost, null,
+ Importing);
UserVm userVm = importVirtualMachineInternal(convertedInstance,
null, zone, destinationCluster, null,
template, displayName, hostName, caller, owner, userId,
serviceOffering, dataDiskOfferingMap,
nicNetworkMap, nicIpAddressMap, guestOsId,
details, false, forced, false);
long timeElapsedInSecs = (System.currentTimeMillis() -
importStartTime) / 1000;
- logger.debug(String.format("VMware VM %s imported successfully to
CloudStack instance %s (%s), Time taken: %d secs, OVF files imported from %s,
Source VMware VM details - OS: %s, PowerState: %s, Disks: %s, NICs: %s",
- sourceVMName, displayName, displayName, timeElapsedInSecs,
(ovfTemplateOnConvertLocation != null)? "MS" : "KVM Host",
sourceVMwareInstance.getOperatingSystem(),
sourceVMwareInstance.getPowerState(), sourceVMwareInstance.getDisks(),
sourceVMwareInstance.getNics()));
- importVmTasksManager.updateImportVMTaskStep(importVMTask, zone,
owner, convertHost, importHost, userVm.getId(), Completed);
+ logger.debug(String.format(
+ "VMware VM %s imported successfully to CloudStack instance
%s (%s), Time taken: %d secs, OVF files imported from %s, Source VMware VM
details - OS: %s, PowerState: %s, Disks: %s, NICs: %s",
+ sourceVMName, displayName, displayName, timeElapsedInSecs,
+ (ovfTemplateOnConvertLocation != null) ? "MS" : "KVM Host",
+ sourceVMwareInstance.getOperatingSystem(),
sourceVMwareInstance.getPowerState(),
+ sourceVMwareInstance.getDisks(),
sourceVMwareInstance.getNics()));
+ importVmTasksManager.updateImportVMTaskStep(importVMTask, zone,
owner, convertHost, importHost,
+ userVm.getId(), Completed);
return userVm;
} catch (CloudRuntimeException e) {
logger.error(String.format("Error importing VM: %s",
e.getMessage()), e);
- importVmTasksManager.updateImportVMTaskErrorState(importVMTask,
ImportVmTask.TaskState.Failed, e.getMessage());
- ActionEventUtils.onCompletedActionEvent(userId, owner.getId(),
EventVO.LEVEL_ERROR, EventTypes.EVENT_VM_IMPORT,
- cmd.getEventDescription(), null, null, 0);
+ importVmTasksManager.updateImportVMTaskErrorState(importVMTask,
ImportVmTask.TaskState.Failed,
+ e.getMessage());
+ ActionEventUtils.onCompletedActionEvent(userId, owner.getId(),
EventVO.LEVEL_ERROR,
+ EventTypes.EVENT_VM_IMPORT, cmd.getEventDescription(),
null, null, 0);
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR,
e.getMessage());
Review Comment:
The PR description mentions adding a null guard before calling
`updateImportVMTaskErrorState(...)` here, but the call site still invokes it
unconditionally. Since the callee now early-returns on null, this is
functionally safe, but it would be clearer to either add the caller-side guard
(avoiding the extra warn log) or update the PR description to reflect the
actual change.
--
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]
