Copilot commented on code in PR #14224:
URL: https://github.com/apache/cloudstack/pull/14224#discussion_r4259762110
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -3793,11 +3793,9 @@ private void orchestrateMigrateWithStorage(final String
vmUuid, final long srcHo
AttachOrDettachConfigDriveCommand dettachCommand = new
AttachOrDettachConfigDriveCommand(vm.getInstanceName(), vmData,
VmConfigDriveLabel.value(), false);
try {
_agentMgr.send(srcHost.getId(), dettachCommand);
- logger.debug("Deleted config drive ISO for vm {} in
host {}", vm.getInstanceName(), srcHost);
+ logger.debug("Deleted config drive ISO for VM {} in
host {}.", vm.getInstanceName(), srcHost);
} catch (OperationTimedoutException e) {
- logger.error("TIme out occurred while exeuting command
AttachOrDettachConfigDrive {}", e.getMessage(), e);
-
- }
+ logger.error("Timeout occurred while executing
command AttachOrDettachConfigDrive for VM {} on source host {}",
vm.getInstanceName(), srcHost.getId(), e); }
Review Comment:
This line appears to have a closing brace `}` appended on the same line with
significant extra whitespace, which is likely to violate formatting/checkstyle
and makes the catch block hard to read/maintain. Please move the closing brace
onto its own line and fix indentation so the `catch` block is properly
formatted.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -314,29 +305,27 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
DomainState state = null;
try {
state = dm.getInfo().state;
- logger.info("VM domain state when trying to pause VM
for migration: {}", state);
+ logger.info("VM domain state when trying to pause VM
for migration: {}.", state);
} catch (final LibvirtException e) {
- logger.info("Couldn't get VM domain state after " +
sleeptime + "ms: " + e.getMessage());
+ logger.info("Could not get VM domain state after {}ms:
{}.", sleeptime, e.getMessage());
Review Comment:
This log drops the stack trace by only logging `e.getMessage()`, which
reduces troubleshooting value (especially since nearby logs include the
exception object). Consider passing the exception as the last argument (and
potentially using `warn` like the earlier state-retrieval failure) to preserve
stack trace details.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -416,14 +405,18 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
commandState = Command.State.COMPLETED;
libvirtComputingResource.createOrUpdateLogFileForCommand(command,
commandState);
} else if (commandState == null) {
- logger.error("Migration of VM {} failed with result: {}", vmName,
result);
+ logger.error("Migration of VM {} failed with result: {}.", vmName,
result);
commandState = Command.State.FAILED;
libvirtComputingResource.createOrUpdateLogFileForCommand(command,
commandState);
}
return new MigrateAnswer(command, result == null, result, null);
}
+ private void logMigrationFailure(String vmName, String destinationIp,
Exception exception) {
+ logger.error("Could not migrate domain [{}] to host [{}].", vmName,
destinationIp, exception);
Review Comment:
The log message mixes SLF4J placeholders with manually embedded brackets
(`[{}]`), while most refactored logs use plain `{}` (sometimes with literal
brackets elsewhere). For consistency and easier grepping, prefer one convention
(e.g., `... domain {} to host {} ...`) and let formatting/brackets be handled
uniformly across the file.
--
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]
