GutoVeronezi commented on code in PR #8511:
URL: https://github.com/apache/cloudstack/pull/8511#discussion_r1491606011
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -2309,7 +2311,7 @@ public PowerState getVmState(final Connect conn, final
String vmName) {
final PowerState s = convertToPowerState(vms.getInfo().state);
return s;
} catch (final LibvirtException e) {
- LOGGER.warn("Can't get vm state " + vmName + e.getMessage() +
"retry:" + retry);
+ LOGGER.error(String.format("Can't get state for VM [%s]
(retry=%s).", vmName, retry), e);
Review Comment:
```suggestion
LOGGER.error("Can't get state for VM [{}] (retry={}).",
vmName, retry, e);
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtExtendedVmStatsEntry.java:
##########
@@ -0,0 +1,51 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package com.cloud.hypervisor.kvm.resource;
+
+import com.cloud.agent.api.VmStatsEntry;
+import
org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils;
+
+import java.util.Calendar;
+
+public class LibvirtExtendedVmStatsEntry extends VmStatsEntry {
+ private long cpuTime;
+ private Calendar timestamp;
+
+ public LibvirtExtendedVmStatsEntry() {
+ }
+
+ public long getCpuTime() {
+ return cpuTime;
+ }
+
+ public void setCpuTime(long cpuTime) {
+ this.cpuTime = cpuTime;
+ }
+
+ public Calendar getTimestamp() {
+ return timestamp;
+ }
+
+ public void setTimestamp(Calendar timestamp) {
+ this.timestamp = timestamp;
+ }
+
+ @Override
+ public String toString() {
+ return ReflectionToStringBuilderUtils.reflectOnlySelectedFields(this,
"cpuTime", "networkWriteKBs", "networkReadKBs", "diskReadIOs", "diskWriteIOs",
"diskReadKBs", "diskWriteKBs");
Review Comment:
We could use `ReflectionToStringBuilder.toStringExclude()` if the number of
fields omitted is smaller than the printed.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -4388,127 +4390,193 @@ protected String getDiskPathFromDiskDef(DiskDef disk)
{
return null;
}
- private class VmStats {
- long usedTime;
- long tx;
- long rx;
- long ioRead;
- long ioWrote;
- long bytesRead;
- long bytesWrote;
- Calendar timestamp;
+ private String vmToString(Domain dm) throws LibvirtException {
+ return String.format("{\"name\":\"%s\",\"uuid\":\"%s\"}",
dm.getName(), dm.getUUIDString());
}
+ /**
+ * Returns metrics for the period since this function was last called for
the specified VM.
+ * @param conn the Libvirt connection.
+ * @param vmName name of the VM.
+ * @return metrics for the period since last time this function was called
for the VM.
+ * @throws LibvirtException
+ */
public VmStatsEntry getVmStat(final Connect conn, final String vmName)
throws LibvirtException {
Domain dm = null;
try {
+ LOGGER.debug("Trying to get VM with name [{}].", vmName);
dm = getDomain(conn, vmName);
if (dm == null) {
+ LOGGER.warn("Could not get VM with name [{}].", vmName);
return null;
}
- DomainInfo info = dm.getInfo();
- final VmStatsEntry stats = new VmStatsEntry();
- stats.setNumCPUs(info.nrVirtCpu);
- stats.setEntityType("vm");
+ LibvirtExtendedVmStatsEntry newStats = getVmCurrentStats(dm);
+ LibvirtExtendedVmStatsEntry oldStats = vmStats.get(vmName);
- stats.setMemoryKBs(info.maxMem);
- stats.setTargetMemoryKBs(info.memory);
- stats.setIntFreeMemoryKBs(getMemoryFreeInKBs(dm));
+ VmStatsEntry metrics = calculateVmMetrics(dm, oldStats, newStats);
- /* get cpu utilization */
- VmStats oldStats = null;
+ String vmAsString = vmToString(dm);
+ LOGGER.info("Saving stats for VM [{}].", vmAsString);
+ vmStats.put(vmName, newStats);
+ LOGGER.debug("Saved stats for VM [{}]: [{}].", vmAsString,
newStats);
- final Calendar now = Calendar.getInstance();
+ return metrics;
+ } finally {
+ if (dm != null) {
+ dm.free();
+ }
+ }
+ }
- oldStats = vmStats.get(vmName);
+ /**
+ * Returns a VM's current statistics.
+ * @param dm domain of the VM.
+ * @return current statistics of the VM.
+ * @throws LibvirtException
+ */
+ protected LibvirtExtendedVmStatsEntry getVmCurrentStats(final Domain dm)
throws LibvirtException {
+ final LibvirtExtendedVmStatsEntry stats = new
LibvirtExtendedVmStatsEntry();
- long elapsedTime = 0;
- if (oldStats != null) {
- elapsedTime = now.getTimeInMillis() -
oldStats.timestamp.getTimeInMillis();
- double utilization = (info.cpuTime - oldStats.usedTime) /
((double)elapsedTime * 1000000);
+ getVmCurrentCpuStats(dm, stats);
+ getVmCurrentNetworkStats(dm, stats);
+ getVmCurrentDiskStats(dm, stats);
- utilization = utilization / info.nrVirtCpu;
- if (utilization > 0) {
- stats.setCPUUtilization(utilization * 100);
- }
- }
+ LOGGER.debug("Retrieved statistics for VM [{}]: [{}].",
vmToString(dm), stats);
+ stats.setTimestamp(Calendar.getInstance());
+ return stats;
+ }
- /* get network stats */
+ /**
+ * Passes a VM's current CPU statistics into the provided
LibvirtExtendedVmStatsEntry.
+ * @param dm domain of the VM.
+ * @param stats LibvirtExtendedVmStatsEntry that will receive the current
CPU statistics.
+ * @throws LibvirtException
+ */
+ protected void getVmCurrentCpuStats(final Domain dm, final
LibvirtExtendedVmStatsEntry stats) throws LibvirtException {
+ LOGGER.debug("Getting CPU stats for VM [{}].", vmToString(dm));
+ stats.setCpuTime(dm.getInfo().cpuTime);
+ }
- final List<InterfaceDef> vifs = getInterfaces(conn, vmName);
- long rx = 0;
- long tx = 0;
- for (final InterfaceDef vif : vifs) {
- final DomainInterfaceStats ifStats =
dm.interfaceStats(vif.getDevName());
- rx += ifStats.rx_bytes;
- tx += ifStats.tx_bytes;
- }
+ /**
+ * Passes a VM's current network statistics into the provided
LibvirtExtendedVmStatsEntry.
+ * @param dm domain of the VM.
+ * @param stats LibvirtExtendedVmStatsEntry that will receive the current
network statistics.
+ * @throws LibvirtException
+ */
+ protected void getVmCurrentNetworkStats(final Domain dm, final
LibvirtExtendedVmStatsEntry stats) throws LibvirtException {
+ final String vmAsString = vmToString(dm);
+ LOGGER.debug("Getting network stats for VM [{}].", vmAsString);
+ final List<InterfaceDef> vifs = getInterfaces(dm.getConnect(),
dm.getName());
+ LOGGER.debug("Found [{}] network interface(s) for VM [{}].",
vifs.size(), vmAsString);
+ double rx = 0;
+ double tx = 0;
+ for (final InterfaceDef vif : vifs) {
+ final DomainInterfaceStats ifStats =
dm.interfaceStats(vif.getDevName());
+ rx += ifStats.rx_bytes;
+ tx += ifStats.tx_bytes;
+ }
+ stats.setNetworkReadKBs(rx / 1024);
+ stats.setNetworkWriteKBs(tx / 1024);
+ }
- if (oldStats != null) {
- final double deltarx = rx - oldStats.rx;
- if (deltarx > 0) {
- stats.setNetworkReadKBs(deltarx / 1024);
- }
- final double deltatx = tx - oldStats.tx;
- if (deltatx > 0) {
- stats.setNetworkWriteKBs(deltatx / 1024);
- }
+ /**
+ * Passes a VM's current disk statistics into the provided
LibvirtExtendedVmStatsEntry.
+ * @param dm domain of the VM.
+ * @param stats LibvirtExtendedVmStatsEntry that will receive the current
disk statistics.
+ * @throws LibvirtException
+ */
+ protected void getVmCurrentDiskStats(final Domain dm, final
LibvirtExtendedVmStatsEntry stats) throws LibvirtException {
+ final String vmAsString = vmToString(dm);
+ LOGGER.debug("Getting disk stats for VM [{}].", vmAsString);
+ final List<DiskDef> disks = getDisks(dm.getConnect(), dm.getName());
+ LOGGER.debug("Found [{}] disk(s) for VM [{}].", disks.size(),
vmAsString);
+ long io_rd = 0;
+ long io_wr = 0;
+ double bytes_rd = 0;
+ double bytes_wr = 0;
+ for (final DiskDef disk : disks) {
+ if (disk.getDeviceType() == DeviceType.CDROM ||
disk.getDeviceType() == DeviceType.FLOPPY) {
+ LOGGER.debug("Ignoring disk [{}] in VM [{}]'s stats since its
deviceType is [{}].", disk.toString().replace("\n", ""), vmAsString,
disk.getDeviceType());
+ continue;
}
+ final DomainBlockStats blockStats =
dm.blockStats(disk.getDiskLabel());
+ io_rd += blockStats.rd_req;
+ io_wr += blockStats.wr_req;
+ bytes_rd += blockStats.rd_bytes;
+ bytes_wr += blockStats.wr_bytes;
+ }
+ stats.setDiskReadIOs(io_rd);
+ stats.setDiskWriteIOs(io_wr);
+ stats.setDiskReadKBs(bytes_rd / 1024);
+ stats.setDiskWriteKBs(bytes_wr / 1024);
+ }
- /* get disk stats */
- final List<DiskDef> disks = getDisks(conn, vmName);
- long io_rd = 0;
- long io_wr = 0;
- long bytes_rd = 0;
- long bytes_wr = 0;
- for (final DiskDef disk : disks) {
- if (disk.getDeviceType() == DeviceType.CDROM ||
disk.getDeviceType() == DeviceType.FLOPPY) {
- continue;
- }
- final DomainBlockStats blockStats =
dm.blockStats(disk.getDiskLabel());
- io_rd += blockStats.rd_req;
- io_wr += blockStats.wr_req;
- bytes_rd += blockStats.rd_bytes;
- bytes_wr += blockStats.wr_bytes;
+ /**
+ * Calculates a VM's metrics for the period between the two statistics
given as parameters.
+ * @param dm domain of the VM.
+ * @param oldStats old statistics. If null, the CPU, network and disk
utilization won't be calculated.
+ * @param newStats new statistics.
+ * @return metrics for the period between the two statistics.
+ * @throws LibvirtException
+ */
+ protected VmStatsEntry calculateVmMetrics(final Domain dm, final
LibvirtExtendedVmStatsEntry oldStats, final LibvirtExtendedVmStatsEntry
newStats) throws LibvirtException {
+ final VmStatsEntry metrics = new VmStatsEntry();
+ final DomainInfo info = dm.getInfo();
+ final String vmAsString = vmToString(dm);
+
+ metrics.setEntityType("vm");
+ LOGGER.debug("Writing VM [{}]'s CPU and memory information into the
metrics.", vmAsString);
+ metrics.setNumCPUs(info.nrVirtCpu);
+ metrics.setMemoryKBs(info.maxMem);
+ metrics.setTargetMemoryKBs(info.memory);
+ LOGGER.debug("Trying to get free memory for VM [{}].", vmAsString);
+ metrics.setIntFreeMemoryKBs(getMemoryFreeInKBs(dm));
+
+ if (oldStats != null) {
+ LOGGER.debug("Old stats exist for VM [{}]; therefore, the
utilization will be calculated.", vmAsString);
+
+ LOGGER.debug("Calculating CPU utilization for VM [{}].",
vmAsString);
Review Comment:
We could unify these logs.
--
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: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
