From 709845f4a333ad2ace0183706433a0653ba159c6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Gabriel=20Beims=20Br=C3=A4scher?= <gabrascher@gmail.com>
Date: Tue, 12 Feb 2019 22:07:03 -0200
Subject: [PATCH] Keep iotune section in the VM's XML after live migration
 (#3171)

* Keep iotune section in the VM's XML after live migration

When live migrating a KVM VM among local storages, the VM loses the
<iotune> section on its XML, therefore, having no IO limitations.

This commit removes the piece of code that deletes the <iotune> section
in the XML.

* Add test for replaceStorage in LibvirtMigrateCommandWrapper

Signed-off-by: Wido den Hollander <wido@widodh.nl>

* Fix Javadoc for method replaceIpForVNCInDescFile
---
 .../wrapper/LibvirtMigrateCommandWrapper.java | 20 ++++----
 .../LibvirtMigrateCommandWrapperTest.java     | 48 +++++++++++++++++++
 2 files changed, 59 insertions(+), 9 deletions(-)

diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java
index 2e3bd20f75d..0c1370e2551 100644
--- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java
+++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java
@@ -346,13 +346,17 @@ public final class LibvirtMigrateCommandWrapper extends CommandWrapper<MigrateCo
         return xmlDesc;
     }
 
-    // Pass in a list of the disks to update in the XML (xmlDesc). Each disk passed in needs to have a serial number. If any disk's serial number in the
-    // list does not match a disk in the XML, an exception should be thrown.
-    // In addition to the serial number, each disk in the list needs the following info:
-    //   * The value of the 'type' of the disk (ex. file, block)
-    //   * The value of the 'type' of the driver of the disk (ex. qcow2, raw)
-    //   * The source of the disk needs an attribute that is either 'file' or 'dev' as well as its corresponding value.
-    private String replaceStorage(String xmlDesc, Map<String, MigrateCommand.MigrateDiskInfo> migrateStorage)
+    /**
+     * Pass in a list of the disks to update in the XML (xmlDesc). Each disk passed in needs to have a serial number. If any disk's serial number in the
+     * list does not match a disk in the XML, an exception should be thrown.
+     * In addition to the serial number, each disk in the list needs the following info:
+     * <ul>
+     *  <li>The value of the 'type' of the disk (ex. file, block)
+     *  <li>The value of the 'type' of the driver of the disk (ex. qcow2, raw)
+     *  <li>The source of the disk needs an attribute that is either 'file' or 'dev' as well as its corresponding value.
+     * </ul>
+     */
+    protected String replaceStorage(String xmlDesc, Map<String, MigrateCommand.MigrateDiskInfo> migrateStorage)
             throws IOException, ParserConfigurationException, SAXException, TransformerException {
         InputStream in = IOUtils.toInputStream(xmlDesc);
 
@@ -411,8 +415,6 @@ public final class LibvirtMigrateCommandWrapper extends CommandWrapper<MigrateCo
                                     diskNode.appendChild(newChildSourceNode);
                                 } else if ("auth".equals(diskChildNode.getNodeName())) {
                                     diskNode.removeChild(diskChildNode);
-                                } else if ("iotune".equals(diskChildNode.getNodeName())) {
-                                    diskNode.removeChild(diskChildNode);
                                 }
                             }
                         }
diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapperTest.java
index f703a4a324f..86bb6171904 100644
--- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapperTest.java
+++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapperTest.java
@@ -21,9 +21,13 @@ package com.cloud.hypervisor.kvm.resource.wrapper;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 
+import java.io.InputStream;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Map;
+import java.util.HashMap;
 
+import org.apache.commons.io.IOUtils;
 import org.junit.Assert;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -43,6 +47,12 @@ import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
 import com.cloud.hypervisor.kvm.resource.LibvirtConnection;
 import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.DiskDef;
 import com.cloud.utils.exception.CloudRuntimeException;
+import org.w3c.dom.Document;
+
+import javax.xml.parsers.DocumentBuilder;
+import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.xpath.XPathExpressionException;
+import javax.xml.xpath.XPathFactory;
 
 @RunWith(PowerMockRunner.class)
 @PrepareForTest({LibvirtConnection.class, LibvirtMigrateCommandWrapper.class})
@@ -96,6 +106,11 @@ public class LibvirtMigrateCommandWrapperTest {
 "        <backingStore/>\n" +
 "      </backingStore>\n" +
 "      <target dev='vda' bus='virtio'/>\n" +
+"      <iotune>\n" +
+"        <write_iops_sec>500</write_iops_sec>\n" +
+"        <write_iops_sec_max>5000</write_iops_sec_max>\n" +
+"        <write_iops_sec_max_length>60</write_iops_sec_max_length>\n" +
+"      </iotune>\n" +
 "      <serial>4650a2f7fce548e2beaa</serial>\n" +
 "      <alias name='virtio-disk0'/>\n" +
 "      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>\n" +
@@ -208,6 +223,11 @@ public class LibvirtMigrateCommandWrapperTest {
 "        <backingStore/>\n" +
 "      </backingStore>\n" +
 "      <target dev='vda' bus='virtio'/>\n" +
+"      <iotune>\n" +
+"        <write_iops_sec>500</write_iops_sec>\n" +
+"        <write_iops_sec_max>5000</write_iops_sec_max>\n" +
+"        <write_iops_sec_max_length>60</write_iops_sec_max_length>\n" +
+"      </iotune>\n" +
 "      <serial>4650a2f7fce548e2beaa</serial>\n" +
 "      <alias name='virtio-disk0'/>\n" +
 "      <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>\n" +
@@ -435,4 +455,32 @@ public class LibvirtMigrateCommandWrapperTest {
         inOrder.verify(virtResource, Mockito.times(timesCleanup)).cleanupDisk(disk);
     }
 
+    static void assertXpath(final Document doc, final String xPathExpr,
+                            final String expected) {
+        try {
+            Assert.assertEquals(expected, XPathFactory.newInstance().newXPath()
+                    .evaluate(xPathExpr, doc));
+        } catch (final XPathExpressionException e) {
+            Assert.fail("Could not evaluate xpath" + xPathExpr + ":"
+                    + e.getMessage());
+        }
+    }
+
+    @Test
+    public void testReplaceStorage() throws Exception {
+        Map<String, MigrateDiskInfo> mapMigrateStorage = new HashMap<String, MigrateDiskInfo>();
+
+        MigrateDiskInfo diskInfo = new MigrateDiskInfo("123456", DiskType.BLOCK, DriverType.RAW, Source.FILE, "sourctest");
+        mapMigrateStorage.put("/mnt/812ea6a3-7ad0-30f4-9cab-01e3f2985b98/4650a2f7-fce5-48e2-beaa-bcdf063194e6", diskInfo);
+        final String result = libvirtMigrateCmdWrapper.replaceStorage(fullfile, mapMigrateStorage);
+
+        InputStream in = IOUtils.toInputStream(result);
+        DocumentBuilderFactory docFactory = DocumentBuilderFactory.newInstance();
+        DocumentBuilder docBuilder = docFactory.newDocumentBuilder();
+        Document doc = docBuilder.parse(in);
+        assertXpath(doc, "/domain/devices/disk/iotune/write_iops_sec", "500");
+        assertXpath(doc, "/domain/devices/disk/@type", "block");
+        assertXpath(doc, "/domain/devices/disk/driver/@type", "raw");
+    }
+
 }