You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2022/11/07 14:29:23 UTC
[GitHub] [cloudstack] GutoVeronezi commented on a diff in pull request #6358: Fix memory stats for KVM
GutoVeronezi commented on code in PR #6358:
URL: https://github.com/apache/cloudstack/pull/6358#discussion_r1015483402
##########
agent/conf/agent.properties:
##########
@@ -202,6 +202,11 @@ 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.
+# Default value: 0
Review Comment:
As the commented property already has the default value, the line `# Default value: 0` is not necessary.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -1298,9 +1308,85 @@ 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>();
+ 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 (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) {
Review Comment:
```suggestion
if (vmBalloonStatsPeriod == 0) {
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -440,6 +440,19 @@ public class LibvirtComputingResource extends ServerResourceBase implements Serv
protected Boolean enableManuallySettingCpuTopologyOnKvmVm = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.ENABLE_MANUALLY_SETTING_CPU_TOPOLOGY_ON_KVM_VM);
+ /**
+ * Virsh command to set the memory balloon stats period.<br><br>
+ * 1st parameter: the VM name;<br>
+ * 2nd parameter: the period (in seconds).
+ */
+ private static final String COMMAND_SET_MEM_BALLOON_STATS_PERIOD = "virsh dommemstat %s --period %s --live";
Review Comment:
ping @joseflauzino
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -1298,9 +1308,85 @@ 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>();
+ 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 (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())) {
+ 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()));
+ continue;
+ }
+ 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()));
Review Comment:
We could invert those statements to avoid hadouken ifs.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java:
##########
@@ -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)) {
Review Comment:
```suggestion
if (memBalloons != null && memBalloons.getLength() != 0) {
```
--
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-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org