From 08bf39ef61b3cd68d52e8d52c7d10140f7cdc92a Mon Sep 17 00:00:00 2001 From: catborise Date: Sun, 4 Oct 2026 00:27:55 +0300 Subject: [PATCH] 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 | 4 +- instances/test_live_xml.py | 112 ++++++++++++++++++++++++++++ vrtManager/instance.py | 101 +++++++++++-------------- vrtManager/test_console_setters.py | 44 +++++++++++ vrtManager/test_persistent_edits.py | 46 ++++++++++++ vrtManager/test_resize_disk.py | 45 +++++++++++ 6 files changed, 293 insertions(+), 59 deletions(-) create mode 100644 instances/test_live_xml.py create mode 100644 vrtManager/test_console_setters.py create mode 100644 vrtManager/test_persistent_edits.py create mode 100644 vrtManager/test_resize_disk.py diff --git a/instances/livetest.py b/instances/livetest.py index 91b0a40e..41363f7d 100644 --- a/instances/livetest.py +++ b/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"{name}1281" - f"{firmware}{disks}" + f"{firmware}{disks}{extra_devices}" ) diff --git a/instances/test_live_xml.py b/instances/test_live_xml.py new file mode 100644 index 00000000..3d235452 --- /dev/null +++ b/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"" + "" + "" +) +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) diff --git a/vrtManager/instance.py b/vrtManager/instance.py index a65ec69f..4dea4753 100644 --- a/vrtManager/instance.py +++ b/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"""""" xml += """""" - 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 = "".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"" - 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) ): diff --git a/vrtManager/test_console_setters.py b/vrtManager/test_console_setters.py new file mode 100644 index 00000000..ef6808b8 --- /dev/null +++ b/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 = "" +PERSISTENT = "" + + +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() diff --git a/vrtManager/test_persistent_edits.py b/vrtManager/test_persistent_edits.py new file mode 100644 index 00000000..1c683d05 --- /dev/null +++ b/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 = """vm131072131072 +1hvm + + +""" + + +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() diff --git a/vrtManager/test_resize_disk.py b/vrtManager/test_resize_disk.py new file mode 100644 index 00000000..9ee5b381 --- /dev/null +++ b/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()