Copilot commented on code in PR #14224:
URL: https://github.com/apache/cloudstack/pull/14224#discussion_r4072240268
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -208,25 +200,17 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
Set<String> migrateDiskLabels = null;
if (migrateStorage) {
- if (logger.isDebugEnabled()) {
- logger.debug("Changing VM {} volumes during migration to
host: {}.", vmName, target);
- }
+ logger.debug("Changing VM {} volumes during migration to host:
{}.", vmName, target);
xmlDesc = replaceStorage(xmlDesc, mapMigrateStorage,
migrateStorageManaged);
- if (logger.isDebugEnabled()) {
- logger.debug("Changed VM {} XML configuration of used
storage. New XML configuration is {}.", vmName,
maskSensitiveInfoInXML(xmlDesc));
- }
+ logger.debug("Changed VM {} XML configuration of used storage.
New XML configuration is {}.", vmName, maskSensitiveInfoInXML(xmlDesc));
Review Comment:
`maskSensitiveInfoInXML(xmlDesc)` is evaluated eagerly even when DEBUG is
disabled, which can be expensive (XML parsing/masking). Please wrap these
statements with `if (logger.isDebugEnabled()) { ... }` (or use a lazy logging
mechanism available in your logging facade) so the masking work only happens
when the log will be emitted.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -175,11 +169,9 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
String oldIsoVolumePath = getOldVolumePath(disks, vmName);
String newIsoVolumePath =
getNewVolumePathIfDatastoreHasChanged(libvirtComputingResource, conn, to);
if (newIsoVolumePath != null &&
!newIsoVolumePath.equals(oldIsoVolumePath)) {
- logger.debug(String.format("Editing mount path of ISO from %s
to %s", oldIsoVolumePath, newIsoVolumePath));
+ logger.debug("Editing mount path of ISO from {} to {}.",
oldIsoVolumePath, newIsoVolumePath);
xmlDesc = replaceDiskSourceFile(xmlDesc, newIsoVolumePath,
vmName);
- if (logger.isDebugEnabled()) {
- logger.debug("Replaced disk mount point {} with {} in
Instance {} XML configuration. New XML configuration is {}.", oldIsoVolumePath,
newIsoVolumePath, vmName, maskSensitiveInfoInXML(xmlDesc));
- }
+ logger.debug("Replaced disk mount point {} with {} in Instance
{} XML configuration. New XML configuration is {}.", oldIsoVolumePath,
newIsoVolumePath, vmName, maskSensitiveInfoInXML(xmlDesc));
Review Comment:
`maskSensitiveInfoInXML(xmlDesc)` is evaluated eagerly even when DEBUG is
disabled, which can be expensive (XML parsing/masking). Please wrap these
statements with `if (logger.isDebugEnabled()) { ... }` (or use a lazy logging
mechanism available in your logging facade) so the masking work only happens
when the log will be emitted.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -273,14 +257,14 @@ 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, String.valueOf(migrateDowntime));
}
} catch (final LibvirtException e) {
- logger.debug("Failed to set max downtime for
migration, perhaps migration completed? Error: " + e.getMessage());
+ logger.warn("Failed to set max downtime for migration.
It might happen if the migration has already completed. Error: {}",
e.getMessage());
Review Comment:
These exception logs only include `e.getMessage()` and drop the stack trace,
which makes troubleshooting much harder in production. Please pass the
exception as the last argument (e.g., `..., e`) and consider removing
`e.getMessage()` duplication when you include the throwable.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -162,9 +158,7 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
final String target = command.getDestinationIp();
xmlDesc = dm.getXMLDesc(xmlFlag);
- if (logger.isDebugEnabled()) {
- logger.debug("VM {} with XML configuration {} will be migrated
to host {}.", vmName, maskSensitiveInfoInXML(xmlDesc), target);
- }
+ logger.debug("VM {} will be migrated to host {} with the following
XML configuration retrieved with flag [{}]: {} .", vmName, target, xmlFlag,
maskSensitiveInfoInXML(xmlDesc));
Review Comment:
`maskSensitiveInfoInXML(xmlDesc)` is evaluated eagerly even when DEBUG is
disabled, which can be expensive (XML parsing/masking). Please wrap these
statements with `if (logger.isDebugEnabled()) { ... }` (or use a lazy logging
mechanism available in your logging facade) so the masking work only happens
when the log will be emitted.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -290,21 +274,20 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
state = dm.getInfo().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.getMessage());
Review Comment:
These exception logs only include `e.getMessage()` and drop the stack trace,
which makes troubleshooting much harder in production. Please pass the
exception as the last argument (e.g., `..., e`) and consider removing
`e.getMessage()` duplication when you include the throwable.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -273,14 +257,14 @@ 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, String.valueOf(migrateDowntime));
Review Comment:
`String.valueOf(migrateDowntime)` is unnecessary for parameterized logging
(it will be converted automatically). Consider also adding terminal punctuation
to keep log messages consistent with the surrounding updates.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -162,9 +158,7 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
final String target = command.getDestinationIp();
xmlDesc = dm.getXMLDesc(xmlFlag);
- if (logger.isDebugEnabled()) {
- logger.debug("VM {} with XML configuration {} will be migrated
to host {}.", vmName, maskSensitiveInfoInXML(xmlDesc), target);
- }
+ logger.debug("VM {} will be migrated to host {} with the following
XML configuration retrieved with flag [{}]: {} .", vmName, target, xmlFlag,
maskSensitiveInfoInXML(xmlDesc));
Review Comment:
There are a couple of formatting issues that reduce readability/consistency
in logs: (1) line 161 has an extra space before the final period (`{} .`), and
(2) line 302 is missing a space after the comma in the argument list
(`{}",sleeptime`). Please fix these so log output and code style stay
consistent.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -316,27 +299,25 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
state = dm.getInfo().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());
}
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.getMessage());
Review Comment:
These exception logs only include `e.getMessage()` and drop the stack trace,
which makes troubleshooting much harder in production. Please pass the
exception as the last argument (e.g., `..., e`) and consider removing
`e.getMessage()` duplication when you include the throwable.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -208,25 +200,17 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
Set<String> migrateDiskLabels = null;
if (migrateStorage) {
- if (logger.isDebugEnabled()) {
- logger.debug("Changing VM {} volumes during migration to
host: {}.", vmName, target);
- }
+ logger.debug("Changing VM {} volumes during migration to host:
{}.", vmName, target);
xmlDesc = replaceStorage(xmlDesc, mapMigrateStorage,
migrateStorageManaged);
- if (logger.isDebugEnabled()) {
- logger.debug("Changed VM {} XML configuration of used
storage. New XML configuration is {}.", vmName,
maskSensitiveInfoInXML(xmlDesc));
- }
+ logger.debug("Changed VM {} XML configuration of used storage.
New XML configuration is {}.", vmName, maskSensitiveInfoInXML(xmlDesc));
migrateDiskLabels = getMigrateStorageDeviceLabels(disks,
mapMigrateStorage);
}
Map<String, DpdkTO> dpdkPortsMapping =
command.getDpdkInterfaceMapping();
if (MapUtils.isNotEmpty(dpdkPortsMapping)) {
- if (logger.isTraceEnabled()) {
- logger.trace("Changing VM {} DPDK interfaces during
migration to host: {}.", vmName, target);
- }
+ logger.trace("Changing VM {} DPDK interfaces during migration
to host: {}.", vmName, target);
xmlDesc = replaceDpdkInterfaces(xmlDesc, dpdkPortsMapping);
- if (logger.isDebugEnabled()) {
- logger.debug("Changed VM {} XML configuration of DPDK
interfaces. New XML configuration is {}.", vmName,
maskSensitiveInfoInXML(xmlDesc));
- }
+ logger.debug("Changed VM {} XML configuration of DPDK
interfaces. New XML configuration is {}.", vmName,
maskSensitiveInfoInXML(xmlDesc));
Review Comment:
`maskSensitiveInfoInXML(xmlDesc)` is evaluated eagerly even when DEBUG is
disabled, which can be expensive (XML parsing/masking). Please wrap these
statements with `if (logger.isDebugEnabled()) { ... }` (or use a lazy logging
mechanism available in your logging facade) so the masking work only happens
when the log will be emitted.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java:
##########
@@ -316,27 +299,25 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
state = dm.getInfo().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:
There are a couple of formatting issues that reduce readability/consistency
in logs: (1) line 161 has an extra space before the final period (`{} .`), and
(2) line 302 is missing a space after the comma in the argument list
(`{}",sleeptime`). Please fix these so log output and code style stay
consistent.
--
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]
