Copilot commented on code in PR #14224:
URL: https://github.com/apache/cloudstack/pull/14224#discussion_r4273485007
##########
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 migration failure helper only logs `destinationIp`, but elsewhere this
method uses `destinationUri` (which can carry additional connection context
beyond just the IP). To improve consistency and troubleshooting value, consider
changing this helper to accept (and log) the same destination identifier used
in other migration logs (e.g., `destinationUri`), or log both IP and URI when
available.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -108,9 +108,7 @@ public Answer execute(final MigrateCommand command, final
LibvirtComputingResour
final Map<String, Boolean> vlanToPersistenceMap =
command.getVlanToPersistenceMap();
final String destinationUri =
createMigrationURI(command.getDestinationIp(), libvirtComputingResource);
final List<MigrateDiskInfo> migrateDiskInfoList =
command.getMigrateDiskInfoList();
- if (logger.isDebugEnabled()) {
- logger.debug(String.format("Trying to migrate VM [%s] to
destination host: [%s].", vmName, destinationUri));
- }
+ logger.debug("Trying to migrate VM {} to destination host: {}.",
vmName, destinationUri);
Review Comment:
This (and similar changes in this PR) removes the prior `isDebugEnabled()` /
`isTraceEnabled()` guards. While parameterized logging avoids string formatting
work, SLF4J still typically allocates a varargs `Object[]` for these calls even
when the level is disabled. If this path is performance-sensitive (or executed
frequently), consider restoring log-level guards for these statements
(especially inside loops) to avoid the allocation overhead.
##########
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);
Review Comment:
This log uses `srcHost` (object) as the placeholder value, while the updated
error log in the same block uses `srcHost.getId()`. Using the host ID (or
another stable identifier) here as well would keep logs consistent and avoid
potentially verbose/unstable `toString()` output for the host object.
--
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]
