Fix memory stats for KVM (apache#6358)

joseflauzino · joseflauzino · web-flow · commit 1843632c24da · 2022-11-09T18:00:12.000+01:00
Co-authored-by: joseflauzino &lt;jose@scclouds.com.br&gt;
diff --git a/agent/conf/agent.properties b/agent/conf/agent.properties
@@ -202,6 +202,10 @@ hypervisor.type=kvm
 # Disable memory ballooning on vm guests for overcommit, by default overcommit
 # feature enables balloon and sets currentMemory to a minimum value.
 #
+# The time interval (in seconds) at which the balloon driver will get memory stats updates.
+# This is equivalent to Libvirt's --period parameter when using the dommemstat command.
+# vm.memballoon.stats.period=0
+#
 # vm.diskactivity.checkenabled=false
 # Set to true to check disk activity on VM's disks before starting a VM. This only applies
 # to QCOW2 files, and ensures that there is no other running instance accessing
diff --git a/agent/src/main/java/com/cloud/agent/Agent.java b/agent/src/main/java/com/cloud/agent/Agent.java
@@ -100,7 +100,7 @@
  *
  **/
 public class Agent implements HandlerFactory, IAgentControl {
-    private static final Logger s_logger = Logger.getLogger(Agent.class.getName());
+    protected static Logger s_logger = Logger.getLogger(Agent.class);
 
     public enum ExitStatus {
         Normal(0), // Normal status = 0.
@@ -303,6 +303,7 @@ public void start() {
         }
         _shell.updateConnectedHost();
         scavengeOldAgentObjects();
+
     }
 
     public void stop(final String reason, final String detail) {
diff --git a/agent/src/main/java/com/cloud/agent/properties/AgentProperties.java b/agent/src/main/java/com/cloud/agent/properties/AgentProperties.java
@@ -54,6 +54,21 @@ public class AgentProperties{
      */
     public static final Property<Boolean> ENABLE_MANUALLY_SETTING_CPU_TOPOLOGY_ON_KVM_VM = new Property<Boolean>("enable.manually.setting.cpu.topology.on.kvm.vm", true);
 
+    /**
+     * Disables memory ballooning on VM guests for overcommit.<br>
+     * By default overcommit feature enables balloon and sets currentMemory to a minimum value.<br>
+     * Data type: Boolean.<br>
+     * Default value: <code>false</code>
+     */
+    public static final Property<Boolean> VM_MEMBALLOON_DISABLE = new Property<>("vm.memballoon.disable", false);
+
+    /**
+     * The time interval (in seconds) at which the balloon driver will get memory stats updates. This is equivalent to Libvirt's <code>--period</code> parameter when using the dommemstat command.
+     * Data type: Integer.<br>
+     * Default value: <code>0</code>
+     */
+    public static final Property<Integer> VM_MEMBALLOON_STATS_PERIOD = new Property<>("vm.memballoon.stats.period", 0);
+
     public static class Property <T>{
         private final String name;
         private final T defaultValue;
diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
@@ -147,6 +147,7 @@
 import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.GuestResourceDef;
 import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.InputDef;
 import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.InterfaceDef;
+import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.MemBalloonDef;
 import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.RngDef;
 import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.RngDef.RngBackendModel;
 import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.SCSIDef;
@@ -221,7 +222,7 @@
  *         pool | the parent of the storage pool hierarchy * }
  **/
 public class LibvirtComputingResource extends ServerResourceBase implements ServerResource, VirtualRouterDeployer {
-    private static final Logger s_logger = Logger.getLogger(LibvirtComputingResource.class);
+    protected static Logger s_logger = Logger.getLogger(LibvirtComputingResource.class);
 
     private static final String CONFIG_VALUES_SEPARATOR = ",";
 
@@ -448,6 +449,15 @@ public class LibvirtComputingResource extends ServerResourceBase implements Serv
 
     protected Boolean enableManuallySettingCpuTopologyOnKvmVm = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.ENABLE_MANUALLY_SETTING_CPU_TOPOLOGY_ON_KVM_VM);
 
+    protected LibvirtDomainXMLParser parser = new LibvirtDomainXMLParser();
+
+    /**
+     * Virsh command to set the memory balloon stats period.<br><br>
+     * 1st parameter: the VM ID or name;<br>
+     * 2nd parameter: the period (in seconds).
+     */
+    private static final String COMMAND_SET_MEM_BALLOON_STATS_PERIOD = "virsh dommemstat %s --period %s --live";
+
     protected long getHypervisorLibvirtVersion() {
         return _hypervisorLibvirtVersion;
     }
@@ -1298,9 +1308,84 @@ public boolean configure(final String name, final Map<String, Object> params) th
             s_logger.info("iscsi session clean up is disabled");
         }
 
+        setupMemoryBalloonStatsPeriod(conn);
+
         return true;
     }
 
+    /**
+     * Gets the ID list of the VMs to set memory balloon stats period.
+     * @param conn the Libvirt connection.
+     * @return the list of VM IDs.
+     */
+    protected List<Integer> getVmsToSetMemoryBalloonStatsPeriod(Connect conn) {
+        List<Integer> vmIdList = new ArrayList<>();
+        Integer[] vmIds = null;
+        try {
+            vmIds = ArrayUtils.toObject(conn.listDomains());
+        } catch (final LibvirtException e) {
+            s_logger.error("Unable to get the list of Libvirt domains on this host.", e);
+            return vmIdList;
+        }
+        vmIdList.addAll(Arrays.asList(vmIds));
+        s_logger.debug(String.format("We have found a total of [%s] VMs (Libvirt domains) on this host: [%s].", vmIdList.size(), vmIdList.toString()));
+
+        if (vmIdList.isEmpty()) {
+            s_logger.info("Skipping the memory balloon stats period setting, since there are no VMs (active Libvirt domains) on this host.");
+        }
+        return vmIdList;
+    }
+
+    /**
+     * Gets the current VM balloon stats period from the agent.properties file.
+     * @return the current VM balloon stats period.
+     */
+    protected Integer getCurrentVmBalloonStatsPeriod() {
+        if (Boolean.TRUE.equals(AgentPropertiesFileHandler.getPropertyValue(AgentProperties.VM_MEMBALLOON_DISABLE))) {
+            s_logger.info(String.format("The [%s] property is set to 'true', so the memory balloon stats period will be set to 0 for all VMs.",
+                    AgentProperties.VM_MEMBALLOON_DISABLE.getName()));
+            return 0;
+        }
+        Integer vmBalloonStatsPeriod = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.VM_MEMBALLOON_STATS_PERIOD);
+        if (vmBalloonStatsPeriod == 0) {
+            s_logger.info(String.format("The [%s] property is set to '0', this prevents memory statistics from being displayed correctly. "
+                    + "Adjust (increase) the value of this parameter to correct this.", AgentProperties.VM_MEMBALLOON_STATS_PERIOD.getName()));
+        }
+        return vmBalloonStatsPeriod;
+    }
+
+    /**
+     * Sets the balloon driver of each VM to get the memory stats at the time interval defined in the agent.properties file.
+     * @param conn the Libvirt connection.
+     */
+    protected void setupMemoryBalloonStatsPeriod(Connect conn) {
+        List<Integer> vmIdList = getVmsToSetMemoryBalloonStatsPeriod(conn);
+        Integer currentVmBalloonStatsPeriod = getCurrentVmBalloonStatsPeriod();
+        for (Integer vmId : vmIdList) {
+            Domain dm = null;
+            try {
+                dm = conn.domainLookupByID(vmId);
+                parser.parseDomainXML(dm.getXMLDesc(0));
+                MemBalloonDef memBalloon = parser.getMemBalloon();
+                if (!MemBalloonDef.MemBalloonModel.VIRTIO.equals(memBalloon.getMemBalloonModel())) {
+                    s_logger.debug(String.format("Skipping the memory balloon stats period setting for the VM (Libvirt Domain) with ID [%s] and name [%s] because this VM has no memory"
+                            + " balloon.", vmId, dm.getName()));
+                }
+                String setMemBalloonStatsPeriodCommand = String.format(COMMAND_SET_MEM_BALLOON_STATS_PERIOD, vmId, currentVmBalloonStatsPeriod);
+                String setMemBalloonStatsPeriodResult = Script.runSimpleBashScript(setMemBalloonStatsPeriodCommand);
+                if (StringUtils.isNotBlank(setMemBalloonStatsPeriodResult)) {
+                    s_logger.error(String.format("Unable to set up memory balloon stats period for VM (Libvirt Domain) with ID [%s] due to an error when running the [%s] "
+                            + "command. Output: [%s].", vmId, setMemBalloonStatsPeriodCommand, setMemBalloonStatsPeriodResult));
+                    continue;
+                }
+                s_logger.debug(String.format("The memory balloon stats period [%s] has been set successfully for the VM (Libvirt Domain) with ID [%s] and name [%s].",
+                        currentVmBalloonStatsPeriod, vmId, dm.getName()));
+            } catch (final LibvirtException e) {
+                s_logger.warn("Failed to set up memory balloon stats period." + e.getMessage());
+            }
+        }
+    }
+
     private void enableSSLForKvmAgent(final Map<String, Object> params) {
         final File keyStoreFile = PropertiesUtil.findConfigFile(KeyStoreUtils.KS_FILENAME);
         if (keyStoreFile == null) {
diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java
@@ -41,6 +41,7 @@
 import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.DiskDef;
 import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.InterfaceDef;
 import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.InterfaceDef.NicModel;
+import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.MemBalloonDef;
 import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.RngDef;
 import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.RngDef.RngBackendModel;
 import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.WatchDogDef;
@@ -50,6 +51,7 @@
 public class LibvirtDomainXMLParser {
     private static final Logger s_logger = Logger.getLogger(LibvirtDomainXMLParser.class);
     private final List<InterfaceDef> interfaces = new ArrayList<InterfaceDef>();
+    private MemBalloonDef memBalloonDef = new MemBalloonDef();
     private final List<DiskDef> diskDefs = new ArrayList<DiskDef>();
     private final List<RngDef> rngDefs = new ArrayList<RngDef>();
     private final List<ChannelDef> channels = new ArrayList<ChannelDef>();
@@ -206,6 +208,8 @@ public boolean parseDomainXML(String domXML) {
                 diskDefs.add(def);
             }
 
+            memBalloonDef = parseMemBalloonTag(devices);
+
             NodeList nics = devices.getElementsByTagName("interface");
             for (int i = 0; i < nics.getLength(); i++) {
                 Element nic = (Element)nics.item(i);
@@ -342,6 +346,25 @@ public boolean parseDomainXML(String domXML) {
         return false;
     }
 
+    /**
+     * Parse the memballoon tag.
+     * @param devices the devices tag.
+     * @return the MemBalloonDef.
+     */
+    private MemBalloonDef parseMemBalloonTag(Element devices) {
+        MemBalloonDef def = new MemBalloonDef();
+        NodeList memBalloons = devices.getElementsByTagName("memballoon");
+        if (memBalloons != null && memBalloons.getLength() != 0) {
+            Element memBalloon = (Element)memBalloons.item(0);
+            String model = memBalloon.getAttribute("model");
+            if (model.equalsIgnoreCase("virtio")) {
+                String statsPeriod = getAttrValue("stats", "period", memBalloon);
+                def.defVirtioMemBalloon(statsPeriod);
+            }
+        }
+        return def;
+    }
+
     private static String getTagValue(String tag, Element eElement) {
         NodeList tagNodeList = eElement.getElementsByTagName(tag);
         if (tagNodeList == null || tagNodeList.getLength() == 0) {
@@ -372,6 +395,10 @@ public List<InterfaceDef> getInterfaces() {
         return interfaces;
     }
 
+    public MemBalloonDef getMemBalloon() {
+        return memBalloonDef;
+    }
+
     public List<DiskDef> getDisks() {
         return diskDefs;
     }
diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java
@@ -27,6 +27,9 @@
 import org.apache.commons.lang3.StringUtils;
 import org.apache.log4j.Logger;
 
+import com.cloud.agent.properties.AgentProperties;
+import com.cloud.agent.properties.AgentPropertiesFileHandler;
+
 public class LibvirtVMDef {
     private static final Logger s_logger = Logger.getLogger(LibvirtVMDef.class);
 
@@ -236,6 +239,7 @@ public static class GuestResourceDef {
         private int vcpu = -1;
         private int maxVcpu = -1;
         private boolean memoryBalloning = false;
+        private int memoryBalloonStatsPeriod = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.VM_MEMBALLOON_STATS_PERIOD);
 
         public void setMemorySize(long mem) {
             this.memory = mem;
@@ -276,7 +280,14 @@ public String toString() {
                 response.append(String.format("<cpu> <numa> <cell id='0' cpus='0-%s' memory='%s' unit='KiB'/> </numa> </cpu>\n", this.maxVcpu - 1, this.currentMemory));
             }
 
-            response.append(String.format("<devices>\n<memballoon model='%s'/>\n</devices>\n", this.memoryBalloning ? "virtio" : "none"));
+            MemBalloonDef memBalloonDef = new MemBalloonDef();
+            if (this.memoryBalloning) {
+                memBalloonDef.defVirtioMemBalloon(String.valueOf(memoryBalloonStatsPeriod));
+            } else {
+                memBalloonDef.defNoneMemBalloon();
+            }
+            response.append(String.format("<devices>%n%s%n</devices>%n", memBalloonDef.toString()));
+
             response.append(String.format("<vcpu current=\"%s\">%s</vcpu>\n", this.vcpu, this.maxVcpu));
             return response.toString();
         }
@@ -1172,6 +1183,53 @@ public String toString() {
         }
     }
 
+    public static class MemBalloonDef {
+        private MemBalloonModel memBalloonModel;
+        private String memBalloonStatsPeriod;
+
+        public enum MemBalloonModel {
+            NONE("none"), VIRTIO("virtio");
+            String model;
+
+            MemBalloonModel(String model) {
+                this.model = model;
+            }
+
+            @Override
+            public String toString() {
+                return model;
+            }
+        }
+
+        public void defNoneMemBalloon() {
+            memBalloonModel = MemBalloonModel.NONE;
+        }
+
+        public void defVirtioMemBalloon(String period) {
+            memBalloonModel = MemBalloonModel.VIRTIO;
+            memBalloonStatsPeriod = period;
+        }
+
+        public MemBalloonModel getMemBalloonModel() {
+            return memBalloonModel;
+        }
+
+        public String getMemBalloonStatsPeriod() {
+            return memBalloonStatsPeriod;
+        }
+
+        @Override
+        public String toString() {
+            StringBuilder memBalloonBuilder = new StringBuilder();
+            memBalloonBuilder.append("<memballoon model='" + memBalloonModel + "'>\n");
+            if (StringUtils.isNotBlank(memBalloonStatsPeriod)) {
+                memBalloonBuilder.append("<stats period='" + memBalloonStatsPeriod +"'/>\n");
+            }
+            memBalloonBuilder.append("</memballoon>");
+            return memBalloonBuilder.toString();
+        }
+    }
+
     public static class InterfaceDef {
         public enum GuestNetType {
             BRIDGE("bridge"), DIRECT("direct"), NETWORK("network"), USER("user"), ETHERNET("ethernet"), INTERNAL("internal"), VHOSTUSER("vhostuser");
diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java
diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParserTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParserTest.java
diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtVMDefTest.java

Original file line number	Diff line number	Diff line change
`@@ -100,7 +100,7 @@`
`100`	`100`	`*`
`101`	`101`	`**/`
`102`	`102`	`public class Agent implements HandlerFactory, IAgentControl {`
`103`		`- private static final Logger s_logger = Logger.getLogger(Agent.class.getName());`
	`103`	`+ protected static Logger s_logger = Logger.getLogger(Agent.class);`
`104`	`104`
`105`	`105`	`public enum ExitStatus {`
`106`	`106`	`Normal(0), // Normal status = 0.`
`@@ -303,6 +303,7 @@ public void start() {`
`303`	`303`	`}`
`304`	`304`	`_shell.updateConnectedHost();`
`305`	`305`	`scavengeOldAgentObjects();`
	`306`	`+`
`306`	`307`	`}`
`307`	`308`
`308`	`309`	`public void stop(final String reason, final String detail) {`