Copilot commented on code in PR #14224:
URL: https://github.com/apache/cloudstack/pull/14224#discussion_r4259475084
##########
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);
} catch (OperationTimedoutException e) {
- logger.error("TIme out occurred while exeuting command
AttachOrDettachConfigDrive {}", e.getMessage(), e);
-
+ logger.error("Timeout occurred while executing command
AttachOrDettachConfigDrive {}", e.getMessage(), e);
Review Comment:
This error log lacks key troubleshooting context available in-scope (at
least the VM instance name and source host identifier). Also, since the
throwable is logged, interpolating `e.getMessage()` into `{}` is usually
redundant. Consider including `vm.getInstanceName()` and `srcHost.getId()` (or
another stable host identifier) in the message, and log the exception as the
throwable.
##########
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);
+ logger.error("Could not migrate domain [{}] to host [{}] due to:
[{}].", vmName, command.getDestinationIp(), e.getMessage(), e);
Review Comment:
The same error logging line is duplicated across two catch blocks. To reduce
drift and keep logging consistent over time, consider extracting this into a
small helper (e.g., `logMigrationFailure(vmName, destinationIp, e)`) or
otherwise centralizing the shared log formatting (while preserving the extra
refused-connection handling in the LibvirtException branch).
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -273,38 +265,37 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
final int setDowntime =
dm.migrateSetMaxDowntime(migrateDowntime);
if (setDowntime == 0 ) {
isMigrateDowntimeSet = true;
- logger.debug("Set max downtime for migration of "
+ vmName + " to " + String.valueOf(migrateDowntime) + "ms");
+ logger.debug("Set max downtime for migration of {}
to {}ms.", vmName, migrateDowntime);
}
} catch (final LibvirtException e) {
- logger.debug("Failed to set max downtime for
migration, perhaps migration completed? Error: " + e.getMessage());
+ logger.debug("Failed to set max downtime for
migration. It might happen if the migration has already completed.", e);
}
}
if (sleeptime % 1000 == 0) {
- logger.info("Waiting for migration of " + vmName + " to
complete, waited " + sleeptime + "ms");
+ logger.info("Waiting for migration of {} to complete,
waited {}ms.", vmName, sleeptime);
}
// abort the vm migration if the job is executed more than
vm.migrate.wait
if (migrateWait > 0 && sleeptime > migrateWait * 1000) {
DomainState state = null;
try {
state = dm.getInfo().state;
- logger.info("VM domain state when trying to abort
migration : {}", state);
+ logger.info("VM domain state when trying to abort
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 || state == DomainState.VIR_DOMAIN_PAUSED)) {
try {
DomainJobInfo job = dm.getJobInfo();
- logger.warn("Aborting migration of VM {} with
domain job [{}] due to timeout after {} seconds. " +
- "Job stats: data processed={} bytes, data
remaining={} bytes", vmName, job, migrateWait, job.getDataProcessed(),
job.getDataRemaining());
+ logger.warn("Aborting migration of VM [{}] with
domain job [{}] due to timeout after {} seconds. Job stats: data processed={}
bytes, data remaining={} bytes.", vmName, job, migrateWait,
job.getDataProcessed(), job.getDataRemaining());
dm.abortJob();
result = String.format("Migration of VM [%s] was
cancelled by CloudStack due to time out after %d seconds.", vmName,
migrateWait);
commandState = Command.State.FAILED;
libvirtComputingResource.createOrUpdateLogFileForCommand(command, commandState);
logger.debug(result);
break;
} catch (final LibvirtException e) {
- logger.error(String.format("Failed to abort the VM
migration job of VM [%s] due to: [%s].", vmName, e.getMessage()), e);
+ logger.error("Failed to abort the VM migration job
of VM {} due to: {}.", vmName, e.getMessage(), e);
Review Comment:
This logs both `e.getMessage()` and the exception `e`. Since the throwable
is already passed, including `e.getMessage()` typically duplicates information
and can make logs noisier/inconsistent. Prefer logging the throwable only (and
adjust the message/placeholders accordingly), e.g., keep context like vmName
and attach `e` for the stack trace.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -365,7 +354,7 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
| SAXException
| TransformerException
| URISyntaxException e) {
- logger.error(String.format("Can't migrate domain [%s] due to:
[%s].", vmName, e.getMessage()), e);
+ logger.error("Could not migrate domain [{}] to host [{}] due to:
[{}].", vmName, command.getDestinationIp(), e.getMessage(), e);
Review Comment:
The same error logging line is duplicated across two catch blocks. To reduce
drift and keep logging consistent over time, consider extracting this into a
small helper (e.g., `logMigrationFailure(vmName, destinationIp, e)`) or
otherwise centralizing the shared log formatting (while preserving the extra
refused-connection handling in the LibvirtException branch).
--
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]
