fix(instances): domain edits start from the persistent definition

Edits that redefine a domain read the wrong XML:
- boot menu/order, vCPU hotplug and QoS read XMLDesc(0), which has no
  secrets, so every such edit, even on a stopped VM, removed the VNC
  password from the definition;
- console, video, CPU, memory and title/description edits read the live
  XML, so on a running VM pending changes (e.g. a new memory size) were
  reverted and runtime state was written into the definition.

All of them now read VIR_DOMAIN_XML_INACTIVE | SECURE (PERSISTENT_XML).
The console setters take the first graphics device of that definition
instead of looking it up by the live type.

Disk resize no longer redefines the domain. A running VM's image files
grow through blockResize, since qemu-img cannot take the lock QEMU
holds; block volumes (e.g. LVM) are grown by the storage pool first and
QEMU is then told the new size.

Live tests reproduce the losses on a RHEL qemu-kvm host; unit tests
check the XML flags, the console setters and the resize paths in CI.

instances/livetest.py

@@ -101,7 +101,7 @@ def create_volume(conn, name, size_mib=64):
 OVMF_CODE = "/usr/share/edk2/ovmf/OVMF_CODE.fd"
 
 
-def define_vm(conn, name, disk_paths, uefi=False):
+def define_vm(conn, name, disk_paths, uefi=False, extra_devices=""):
     """A small q35 VM without an OS; disks are attached as vda, vdb, ..."""
     assert name.startswith(PREFIX)
     disks = "".join(
@@ -117,7 +117,7 @@ def define_vm(conn, name, disk_paths, uefi=False):
     )
     return conn.defineXML(
         f"<domain type='kvm'><name>{name}</name><memory unit='MiB'>128</memory><vcpu>1</vcpu>"
-        f"{firmware}<features><acpi/></features><devices>{disks}</devices></domain>"
+        f"{firmware}<features><acpi/></features><devices>{disks}{extra_devices}</devices></domain>"
     )
 
 

instances/test_live_xml.py

@@ -0,0 +1,112 @@
+"""Live checks that persistent edits keep secrets and pending changes (R-01, R-02).
+
+Every edit that redefines the domain must start from the persistent
+definition with secrets (VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE).
+Runs only with TEST_LIBVIRT_HOST set (see instances/livetest.py).
+"""
+
+import os
+import unittest
+
+import libvirt
+from django.test import SimpleTestCase
+from lxml import etree
+from vrtManager.connection import connection_manager
+from vrtManager.instance import wvmInstance
+
+from . import livetest
+
+P = livetest.PREFIX
+MAC = "52:54:00:aa:01:01"
+DEVICES = (
+    f"<interface type='network'><mac address='{MAC}'/><source network='default'/><model type='virtio'/></interface>"
+    "<graphics type='vnc' autoport='yes' passwd='keep1234'/>"
+    "<video><model type='vga' primary='yes'/></video>"
+)
+PERSISTENT = libvirt.VIR_DOMAIN_XML_INACTIVE | libvirt.VIR_DOMAIN_XML_SECURE
+
+# name -> (edit, allowed while running)
+EDITS = {
+    "set_bootmenu": (lambda vm: vm.set_bootmenu(1), True),
+    "set_bootorder": (lambda vm: vm.set_bootorder({0: {"type": "disk", "dev": "vda"}}), True),
+    "set_vcpu_hotplug": (lambda vm: vm.set_vcpu_hotplug(True, 1), False),
+    "set_console_keymap": (lambda vm: vm.set_console_keymap("de"), True),
+    "set_console_listener_addr": (lambda vm: vm.set_console_listener_addr("127.0.0.1"), True),
+    "set_video_model": (lambda vm: vm.set_video_model("virtio"), True),
+    "resize_cpu": (lambda vm: vm.resize_cpu("1", "2"), True),
+    "resize_mem": (lambda vm: vm.resize_mem(128, 160), False),
+    "resize_disk": (lambda vm: vm.resize_disk([{"path": vm.get_disk_devices()[0]["path"], "size_new": 96 << 20}]), True),
+    "set_options": (lambda vm: vm.set_options({"title": "edited"}), True),
+    "set_qos": (lambda vm: vm.set_qos(MAC, "inbound", 100, 200, 300), True),
+    "unset_qos": (lambda vm: vm.unset_qos(MAC, "inbound"), True),
+}
+
+
+class LivePersistentEditTestCase(SimpleTestCase):
+    @classmethod
+    def setUpClass(cls):
+        if not livetest.enabled():
+            raise unittest.SkipTest("Set TEST_LIBVIRT_HOST to run the live libvirt tests")
+        cls.args = (
+            os.environ["TEST_LIBVIRT_HOST"],
+            os.environ.get("TEST_LIBVIRT_LOGIN", ""),
+            os.environ.get("TEST_LIBVIRT_PASSWORD", ""),
+            int(os.environ.get("TEST_LIBVIRT_TYPE", 4)),
+        )
+        cls.conn = connection_manager.get_connection(*cls.args)
+        livetest.cleanup(cls.conn)
+        livetest.ensure_pool(cls.conn)
+        cls.inventory = livetest.inventory(cls.conn)
+        super().setUpClass()
+
+    @classmethod
+    def tearDownClass(cls):
+        try:
+            livetest.remove_pool(cls.conn)
+        finally:
+            super().tearDownClass()
+        if livetest.inventory(cls.conn) != cls.inventory:
+            raise AssertionError("Live tests changed objects outside the test namespace")
+
+    def tearDown(self):
+        livetest.cleanup(self.conn)
+
+    def vm(self, name):
+        disk = livetest.create_volume(self.conn, P + name)
+        dom = livetest.define_vm(self.conn, P + name, [disk], extra_devices=DEVICES)
+        return dom, wvmInstance(*self.args, None, uuid=dom.UUIDString())
+
+    def persistent(self, dom):
+        return etree.fromstring(dom.XMLDesc(PERSISTENT))
+
+    def test_edits_of_a_stopped_vm_keep_the_vnc_password(self):
+        for name, (edit, _) in EDITS.items():
+            with self.subTest(name):
+                try:
+                    dom, vm = self.vm("xml-stopped")
+                    edit(vm)
+                    self.assertEqual(self.persistent(dom).xpath("string(devices/graphics/@passwd)"), "keep1234")
+                finally:
+                    livetest.cleanup(self.conn)
+
+    def test_edits_of_a_running_vm_keep_pending_changes(self):
+        for name, (edit, while_running) in EDITS.items():
+            if not while_running:
+                continue
+            with self.subTest(name):
+                try:
+                    dom, vm = self.vm("xml-running")
+                    dom.create()
+                    # A pending change: only the persistent definition has 192 MiB.
+                    tree = self.persistent(dom)
+                    tree.find("memory").text = str(192 * 1024)
+                    tree.find("currentMemory").text = str(192 * 1024)
+                    self.conn.defineXML(etree.tostring(tree).decode())
+
+                    edit(vm)
+
+                    after = self.persistent(dom)
+                    self.assertEqual(after.findtext("memory"), str(192 * 1024), "pending memory change lost")
+                    self.assertEqual(after.xpath("string(devices/graphics/@passwd)"), "keep1234")
+                finally:
+                    livetest.cleanup(self.conn)

vrtManager/instance.py

@@ -15,6 +15,8 @@ try:
         VIR_DOMAIN_RUNNING,
         VIR_DOMAIN_XML_SECURE,
         VIR_DOMAIN_XML_INACTIVE,
+        VIR_DOMAIN_BLOCK_RESIZE_BYTES,
+        VIR_STORAGE_VOL_FILE,
         VIR_MIGRATE_AUTO_CONVERGE,
         VIR_MIGRATE_COMPRESSED,
         VIR_MIGRATE_LIVE,
@@ -35,7 +37,7 @@ try:
     )
     from libvirt_qemu import VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT, qemuAgentCommand
 except Exception:
-    from libvirt import libvirtError, VIR_DOMAIN_XML_SECURE, VIR_MIGRATE_LIVE
+    from libvirt import libvirtError, VIR_DOMAIN_XML_SECURE, VIR_DOMAIN_XML_INACTIVE, VIR_MIGRATE_LIVE
 
 from collections import OrderedDict
 from datetime import datetime
@@ -47,6 +49,12 @@ from vrtManager import util
 from vrtManager.connection import wvmConnect
 from vrtManager.storage import wvmStorage, wvmStorages
 
+# Edits that redefine the domain start from this: the persistent definition,
+# with secrets. Live XML would drop pending changes, non-secure XML the VNC
+# password (R-01, R-02).
+PERSISTENT_XML = VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE
+
+
 class wvmInstances(wvmConnect):
     def get_instance_status(self, name):
         inst = self.get_instance(name)
@@ -584,7 +592,7 @@ class wvmInstance(wvmConnect):
         return menu == "yes"
 
     def set_bootmenu(self, flag):
-        tree = ElementTree.fromstring(self._XMLDesc(0))
+        tree = ElementTree.fromstring(self._XMLDesc(PERSISTENT_XML))
         os = tree.find("os")
         menu = os.find("bootmenu")
 
@@ -660,7 +668,7 @@ class wvmInstance(wvmConnect):
             return
 
         def remove_bootorder():
-            tree = ElementTree.fromstring(self._XMLDesc(0))
+            tree = ElementTree.fromstring(self._XMLDesc(PERSISTENT_XML))
             os = tree.find("os")
             boot = os.findall("boot")
             # Remove old style boot order
@@ -969,13 +977,13 @@ class wvmInstance(wvmConnect):
                     xml += f"""<vcpu id='{i}' enabled='yes' hotpluggable='yes' order='{i+1}'/>"""
                 xml += """</vcpus>"""
 
-                tree = etree.fromstring(self._XMLDesc(0))
+                tree = etree.fromstring(self._XMLDesc(PERSISTENT_XML))
                 vcpus = tree.xpath("/domain/vcpus")
                 if not vcpus:
                     tree.append(etree.fromstring(xml))
                     self._defineXML(etree.tostring(tree).decode())
             else:
-                tree = etree.fromstring(self._XMLDesc(0))
+                tree = etree.fromstring(self._XMLDesc(PERSISTENT_XML))
                 vcpus = tree.xpath("/domain/vcpus")
                 for vcpu in vcpus:
                     parent = vcpu.getparent()
@@ -1092,14 +1100,10 @@ class wvmInstance(wvmConnect):
         return listener_addr
 
     def set_console_listener_addr(self, listener_addr):
-        xml = self._XMLDesc(VIR_DOMAIN_XML_SECURE)
-        root = ElementTree.fromstring(xml)
-        console_type = self.get_console_type()
-        try:
-            graphic = root.find("devices/graphics[@type='%s']" % console_type)
-        except SyntaxError:
-            # Little fix for old version ElementTree
-            graphic = root.find("devices/graphics")
+        root = ElementTree.fromstring(self._XMLDesc(PERSISTENT_XML))
+        # The console is the first graphics device of the definition being
+        # edited; the running VM may still have another type.
+        graphic = root.find("devices/graphics")
         if graphic is None:
             return False
         listen = graphic.find("listen[@type='address']")
@@ -1131,18 +1135,14 @@ class wvmInstance(wvmConnect):
         return console_type
 
     def set_console_type(self, console_type):
-        current_type = self.get_console_type()
-        if current_type == console_type:
-            return True
         if console_type == "":
             return False
-        xml = self._XMLDesc(VIR_DOMAIN_XML_SECURE)
-        root = ElementTree.fromstring(xml)
-        try:
-            graphic = root.find(f"devices/graphics[@type='{current_type}']")
-        except SyntaxError:
-            # Little fix for old version ElementTree
-            graphic = root.find("devices/graphics")
+        root = ElementTree.fromstring(self._XMLDesc(PERSISTENT_XML))
+        graphic = root.find("devices/graphics")
+        if graphic is None:
+            return False
+        if graphic.get("type") == console_type:
+            return True
         graphic.set("type", console_type)
         newxml = ElementTree.tostring(root).decode()
         self._defineXML(newxml)
@@ -1170,14 +1170,8 @@ class wvmInstance(wvmConnect):
         )
 
     def set_console_passwd(self, passwd):
-        xml = self._XMLDesc(VIR_DOMAIN_XML_SECURE)
-        root = ElementTree.fromstring(xml)
-        console_type = self.get_console_type()
-        try:
-            graphic = root.find(f"devices/graphics[@type='{console_type}']")
-        except SyntaxError:
-            # Little fix for old version ElementTree
-            graphic = root.find("devices/graphics")
+        root = ElementTree.fromstring(self._XMLDesc(PERSISTENT_XML))
+        graphic = root.find("devices/graphics")
         if graphic is None:
             return False
         if passwd:
@@ -1190,14 +1184,10 @@ class wvmInstance(wvmConnect):
         return self._defineXML(newxml)
 
     def set_console_keymap(self, keymap):
-        xml = self._XMLDesc(VIR_DOMAIN_XML_SECURE)
-        root = ElementTree.fromstring(xml)
-        console_type = self.get_console_type()
-        try:
-            graphic = root.find("devices/graphics[@type='%s']" % console_type)
-        except SyntaxError:
-            # Little fix for old version ElementTree
-            graphic = root.find("devices/graphics")
+        root = ElementTree.fromstring(self._XMLDesc(PERSISTENT_XML))
+        graphic = root.find("devices/graphics")
+        if graphic is None:
+            return False
         if keymap != "auto":
             graphic.set("keymap", keymap)
         else:
@@ -1226,7 +1216,7 @@ class wvmInstance(wvmConnect):
 
     def set_video_model(self, model):
         """Changes only primary video card"""
-        xml = self._XMLDesc(VIR_DOMAIN_XML_SECURE)
+        xml = self._XMLDesc(PERSISTENT_XML)
         tree = etree.fromstring(xml)
         video_models = tree.xpath("/domain/devices/video/model")
         video_xml = "<model type='{}'/>".format(model)
@@ -1245,7 +1235,7 @@ class wvmInstance(wvmConnect):
         if is_vcpus_enabled:
             self.set_vcpu_hotplug(False)
 
-        xml = self._XMLDesc(VIR_DOMAIN_XML_SECURE)
+        xml = self._XMLDesc(PERSISTENT_XML)
         tree = etree.fromstring(xml)
 
         vcpu_elem = tree.find("vcpu")
@@ -1270,7 +1260,7 @@ class wvmInstance(wvmConnect):
             self.set_memory(cur_memory, VIR_DOMAIN_AFFECT_CONFIG)
             return
 
-        xml = self._XMLDesc(VIR_DOMAIN_XML_SECURE)
+        xml = self._XMLDesc(PERSISTENT_XML)
         tree = etree.fromstring(xml)
 
         mem_elem = tree.find("memory")
@@ -1282,19 +1272,16 @@ class wvmInstance(wvmConnect):
         self._defineXML(new_xml)
 
     def resize_disk(self, disks):
-        """
-        Function change disks on vds.
-        """
-        xml = self._XMLDesc(VIR_DOMAIN_XML_SECURE)
-        tree = etree.fromstring(xml)
-
+        """Grow disks. QEMU holds the lock of a running domain's image files and
+        grows them itself; block volumes (e.g. LVM) are grown by the storage
+        pool first, then QEMU is told the new size."""
+        active = self.instance.isActive()
         for disk in disks:
-            source_dev = disk["path"]
-            vol = self.get_volume_by_path(source_dev)
-            vol.resize(disk["size_new"])
-
-        new_xml = etree.tostring(tree).decode()
-        self._defineXML(new_xml)
+            vol = self.get_volume_by_path(disk["path"])
+            if not active or vol.info()[0] != VIR_STORAGE_VOL_FILE:
+                vol.resize(disk["size_new"])
+            if active:
+                self.instance.blockResize(disk["path"], disk["size_new"], VIR_DOMAIN_BLOCK_RESIZE_BYTES)
 
     def get_iso_media(self):
         iso = []
@@ -1852,7 +1839,7 @@ class wvmInstance(wvmConnect):
         """
         Function change description, title
         """
-        xml = self._XMLDesc(VIR_DOMAIN_XML_SECURE)
+        xml = self._XMLDesc(PERSISTENT_XML)
         tree = etree.fromstring(xml)
 
         self._set_options(tree, options)
@@ -1917,7 +1904,7 @@ class wvmInstance(wvmConnect):
         else:
             xml = f"<outbound average='{average}' peak='{peak}' burst='{burst}'/>"
 
-        tree = etree.fromstring(self._XMLDesc(0))
+        tree = etree.fromstring(self._XMLDesc(PERSISTENT_XML))
 
         macs = tree.xpath("/domain/devices/interface/mac")
         for cur_mac in macs:
@@ -1940,7 +1927,7 @@ class wvmInstance(wvmConnect):
         self.wvm.defineXML(new_xml)
 
     def unset_qos(self, mac, direction):
-        tree = etree.fromstring(self._XMLDesc(0))
+        tree = etree.fromstring(self._XMLDesc(PERSISTENT_XML))
         for direct in tree.xpath(
             "/domain/devices/interface/bandwidth/{}".format(direction)
         ):

vrtManager/test_console_setters.py

@@ -0,0 +1,44 @@
+import unittest
+from unittest.mock import MagicMock
+
+from django.conf import settings
+
+if not settings.configured:
+    settings.configure(MAC_OUI="52:54:10")
+
+from lxml import etree
+from vrtManager.instance import PERSISTENT_XML, wvmInstance
+
+LIVE = "<domain><devices><graphics type='vnc' passwd='live'><listen type='address'/></graphics></devices></domain>"
+PERSISTENT = "<domain><devices><graphics type='spice' passwd='old'><listen type='address'/></graphics></devices></domain>"
+
+
+def running_vm_with_pending_graphics_type():
+    vm = wvmInstance.__new__(wvmInstance)
+    vm.wvm = MagicMock()
+    vm._XMLDesc = MagicMock(side_effect=lambda flags: PERSISTENT if flags == PERSISTENT_XML else LIVE)
+    return vm
+
+
+class ConsoleSettersTestCase(unittest.TestCase):
+    """The graphics device to edit comes from the persistent definition."""
+
+    def defined_graphics(self, vm):
+        return etree.fromstring(vm.wvm.defineXML.call_args[0][0]).find("devices/graphics")
+
+    def test_setters_edit_the_persistent_graphics_device(self):
+        edits = {
+            "set_console_passwd": (lambda vm: vm.set_console_passwd("new"), "passwd", "new"),
+            "set_console_keymap": (lambda vm: vm.set_console_keymap("de"), "keymap", "de"),
+            "set_console_listener_addr": (lambda vm: vm.set_console_listener_addr("127.0.0.1"), "listen", "127.0.0.1"),
+            "set_console_type": (lambda vm: vm.set_console_type("vnc"), "type", "vnc"),
+        }
+        for name, (edit, attr, value) in edits.items():
+            with self.subTest(name):
+                vm = running_vm_with_pending_graphics_type()
+                edit(vm)
+                self.assertEqual(self.defined_graphics(vm).get(attr), value)
+
+
+if __name__ == "__main__":
+    unittest.main()

vrtManager/test_persistent_edits.py

@@ -0,0 +1,46 @@
+import unittest
+from unittest.mock import MagicMock
+
+from django.conf import settings
+
+if not settings.configured:
+    settings.configure(MAC_OUI="52:54:10")
+
+from vrtManager.instance import PERSISTENT_XML, wvmInstance
+
+DOMAIN = """<domain><name>vm</name><memory>131072</memory><currentMemory>131072</currentMemory>
+<vcpu>1</vcpu><os><type>hvm</type></os><devices>
+<interface type='network'><mac address='52:54:00:00:00:01'/><source network='default'/></interface>
+<graphics type='vnc' passwd='secret'><listen type='address' address='0.0.0.0'/></graphics>
+<video><model type='vga' primary='yes'/></video></devices></domain>"""
+
+
+class PersistentEditsTestCase(unittest.TestCase):
+    """R-01/R-02: edits read the persistent definition with secrets."""
+
+    def test_edits_read_the_persistent_secure_definition(self):
+        edits = {
+            "set_bootmenu": lambda vm: vm.set_bootmenu(1),
+            "set_console_keymap": lambda vm: vm.set_console_keymap("de"),
+            "set_video_model": lambda vm: vm.set_video_model("virtio"),
+            "resize_cpu": lambda vm: vm.resize_cpu("1", "2"),
+            "set_options": lambda vm: vm.set_options({"title": "t"}),
+            "set_qos": lambda vm: vm.set_qos("52:54:00:00:00:01", "inbound", 1, 2, 3),
+        }
+        for name, edit in edits.items():
+            with self.subTest(name):
+                vm = wvmInstance.__new__(wvmInstance)
+                vm.instance = MagicMock()
+                vm.wvm = MagicMock()
+                vm._XMLDesc = MagicMock(return_value=DOMAIN)
+                vm.get_vcpus = MagicMock(return_value=None)
+                vm.get_status = MagicMock(return_value=5)
+                edit(vm)
+                flags = {call.args[0] for call in vm._XMLDesc.call_args_list}
+                self.assertIn(PERSISTENT_XML, flags)
+                defined = (vm.wvm.defineXML.call_args or vm._XMLDesc.call_args)[0][0]
+                self.assertIn("secret", str(defined))
+
+
+if __name__ == "__main__":
+    unittest.main()

vrtManager/test_resize_disk.py

@@ -0,0 +1,45 @@
+import unittest
+from unittest.mock import MagicMock
+
+from django.conf import settings
+
+if not settings.configured:
+    settings.configure(MAC_OUI="52:54:10")
+
+from libvirt import VIR_DOMAIN_BLOCK_RESIZE_BYTES, VIR_STORAGE_VOL_BLOCK, VIR_STORAGE_VOL_FILE
+from vrtManager.instance import wvmInstance
+
+SIZE = 10 << 30
+
+
+def vm(active, vol_type):
+    inst = wvmInstance.__new__(wvmInstance)
+    inst.instance = MagicMock()
+    inst.instance.isActive.return_value = active
+    vol = MagicMock()
+    vol.info.return_value = [vol_type, 0, 0]
+    inst.get_volume_by_path = MagicMock(return_value=vol)
+    inst.resize_disk([{"path": "/dev/vg/disk", "size_new": SIZE}])
+    return inst, vol
+
+
+class ResizeDiskTestCase(unittest.TestCase):
+    def test_running_block_volume_grows_in_the_pool_then_in_qemu(self):
+        inst, vol = vm(active=True, vol_type=VIR_STORAGE_VOL_BLOCK)
+        vol.resize.assert_called_once_with(SIZE)
+        inst.instance.blockResize.assert_called_once_with("/dev/vg/disk", SIZE, VIR_DOMAIN_BLOCK_RESIZE_BYTES)
+
+    def test_running_image_file_grows_through_qemu_only(self):
+        # qemu-img cannot take the image lock QEMU holds
+        inst, vol = vm(active=True, vol_type=VIR_STORAGE_VOL_FILE)
+        vol.resize.assert_not_called()
+        inst.instance.blockResize.assert_called_once()
+
+    def test_stopped_vm_grows_the_volume(self):
+        inst, vol = vm(active=False, vol_type=VIR_STORAGE_VOL_FILE)
+        vol.resize.assert_called_once_with(SIZE)
+        inst.instance.blockResize.assert_not_called()
+
+
+if __name__ == "__main__":
+    unittest.main()