Copilot commented on code in PR #14226:
URL: https://github.com/apache/cloudstack/pull/14226#discussion_r4075516713
##########
plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/utils/linux/KVMHostInfo.java:
##########
@@ -252,12 +253,21 @@ private void getHostInfoFromLibvirt() {
*/
this.capabilities.add("snapshot");
} catch (final LibvirtException e) {
- LOGGER.error("Caught libvirt exception while fetching host
information", e);
+ LOGGER.error("Caught Libvirt exception while fetching host
information", e);
}
}
private String getCPUArchFromCommand() {
LOGGER.info("Fetching host CPU arch");
return
Script.runSimpleBashScript(Script.getExecutableAbsolutePath(cpuArchRetrieveExecutable));
}
+
+ private static void logCpuSpeedCommandAndSpeed(String command, long speed)
{
+ LOGGER.info("Command [{}] resulted in the value [{}] for CPU speed.",
command, speed);
+ }
+
+ private static void logFailureToGetCpuSpeedAndException(String command,
Exception e) {
+ LOGGER.debug("Unable to retrieve the CPU speed from command [{}].
Trying another way to retrieve the CPU speed.", command);
+ LOGGER.trace(e);
Review Comment:
The failure is currently split into two separate log events (a `debug`
message and a separate `trace` call). This can make troubleshooting harder in
aggregated logging systems (separation by time/thread, missing shared metadata,
harder searching). Consider emitting a single log statement that includes both
the context message and the throwable.
##########
plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/utils/linux/KVMHostInfo.java:
##########
@@ -252,12 +253,21 @@ private void getHostInfoFromLibvirt() {
*/
this.capabilities.add("snapshot");
} catch (final LibvirtException e) {
- LOGGER.error("Caught libvirt exception while fetching host
information", e);
+ LOGGER.error("Caught Libvirt exception while fetching host
information", e);
}
}
private String getCPUArchFromCommand() {
LOGGER.info("Fetching host CPU arch");
return
Script.runSimpleBashScript(Script.getExecutableAbsolutePath(cpuArchRetrieveExecutable));
}
+
+ private static void logCpuSpeedCommandAndSpeed(String command, long speed)
{
+ LOGGER.info("Command [{}] resulted in the value [{}] for CPU speed.",
command, speed);
+ }
+
+ private static void logFailureToGetCpuSpeedAndException(String command,
Exception e) {
+ LOGGER.debug("Unable to retrieve the CPU speed from command [{}].
Trying another way to retrieve the CPU speed.", command);
+ LOGGER.trace(e);
Review Comment:
`LOGGER.trace(e)` is likely calling the `trace(Object message)` overload
(Log4j2 supports `trace(Object)`), which will usually log only `e.toString()`
and not the stack trace. Prefer logging the exception as a throwable parameter
(e.g., include `e` as the throwable on the same log call) so the stack trace is
recorded correctly and in a single event.
##########
plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/utils/linux/KVMHostInfo.java:
##########
@@ -126,55 +125,57 @@ protected static long getCpuSpeed(final String
cpabilities, final NodeInfo nodeI
return speed;
}
- speed = getCpuSpeedFromHostCapabilities(cpabilities);
+ speed = getCpuSpeedFromHostCapabilities(capabilities);
if(speed > 0L) {
return speed;
}
- LOGGER.info(String.format("Using the value [%s] provided by Libvirt.",
nodeInfo.mhz));
+ LOGGER.info("Using the value [{}] provided by Libvirt.", nodeInfo.mhz);
speed = nodeInfo.mhz;
return speed;
}
private static long getCpuSpeedFromCommandLscpu() {
long speed = 0L;
LOGGER.info("Fetching CPU speed from command \"lscpu\".");
+ String command = null;
try {
- String command = "lscpu | grep -i 'CPU max MHz' | head -n 1 | sed
's/^.*: //' | xargs";
+ command = "lscpu | grep -i 'CPU max MHz' | head -n 1 | sed 's/^.*:
//' | xargs";
if(isHostS390x()) {
command = "lscpu | grep 'CPU dynamic MHz' | cut -d ':' -f 2 |
tr -d ' ' | awk '{printf \"%.1f\\n\", $1 / 1000}'";
}
String result = Script.runSimpleBashScript(command);
speed = (long) (Float.parseFloat(result));
- LOGGER.info(String.format("Command [%s] resulted in the value [%s]
for CPU speed.", command, speed));
+ logCpuSpeedCommandAndSpeed(command, speed);
return speed;
} catch (NullPointerException | NumberFormatException e) {
- LOGGER.error(String.format("Unable to retrieve the CPU speed from
lscpu."), e);
+ logFailureToGetCpuSpeedAndException(command, e);
}
+
try {
- String command = "lscpu | grep -i 'Model name' | head -n 1 | egrep
-o '[[:digit:]].[[:digit:]]+GHz' | sed 's/GHz//g'";
+ command = "lscpu | grep -i 'Model name' | head -n 1 | egrep -o
'[[:digit:]].[[:digit:]]+GHz' | sed 's/GHz//g'";
Review Comment:
This line appears to have an extra leading space compared to surrounding
indentation, which can cause noisy diffs and inconsistent formatting. Align the
indentation with the rest of the method body.
--
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]
