Copilot commented on code in PR #14224:
URL: https://github.com/apache/cloudstack/pull/14224#discussion_r4314521585
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -3793,10 +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.getId());
Review Comment:
This message now logs `srcHost.getId()` but the text still says `host {}`
(which can be interpreted as hostname/object). To reduce ambiguity during
troubleshooting, consider changing the wording to explicitly say `host id {}`
(or include both id and hostname if available).
##########
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.warn("Could not get VM domain state after
{}ms.", sleeptime, e);
}
if (state != null && state ==
DomainState.VIR_DOMAIN_RUNNING) {
try {
- logger.info("Pausing VM " + vmName + " due to
property vm.migrate.pauseafter setting to " + migratePauseAfter + "ms to
complete migration");
+ logger.info("Pausing VM {} due to property
vm.migrate.pauseafter setting to {}ms to complete migration.", vmName,
migratePauseAfter);
dm.suspend();
} catch (final LibvirtException e) {
// pause could be racy if it attempts to pause
right when vm is finished, simply warn
- logger.info("Failed to pause vm " + vmName + " : "
+ e.getMessage());
+ logger.warn("Failed to pause vm {}", vmName, e);
}
}
}
}
- logger.info(String.format("Migration thread of VM [%s] finished.",
vmName));
+ logger.info("Migration thread of VM [{}] finished.", vmName);
Review Comment:
Log formatting is inconsistent across the file: many messages use `VM {}`
while some use `VM [{}]`. Consider standardizing on a single convention (e.g.,
`VM {}` everywhere) to make log searching/alerting more consistent and avoid
unnecessary visual noise.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -351,10 +340,10 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
}
} catch (final LibvirtException e) {
- logger.error(String.format("Can't migrate domain [%s] due to:
[%s].", vmName, e.getMessage()), e);
+ logMigrationFailure(vmName, destinationUri, e);
result = e.getMessage();
if (result.startsWith("unable to connect to server") &&
result.endsWith("refused")) {
- logger.debug("Migration failed as connection to destination
[{}] was refused. Please check libvirt configuration compatibility and firewall
rules on the source and destination hosts.", destinationUri);
+ logger.debug("Migration failed as connection to destination
[{}] was refused. Please check Libvirt configuration compatibility and firewall
rules on the source and destination hosts.", destinationUri);
Review Comment:
The project/library name is typically styled as `libvirt` (lowercase). For
consistency and accuracy in logs, consider changing `Libvirt configuration` to
`libvirt configuration` (same applies to other newly-capitalized occurrences
like \"Ignoring Libvirt error.\").
--
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]
