From 9f8633055260e0f0630baec6a78f216b522012f9 Mon Sep 17 00:00:00 2001 From: catborise Date: Sun, 4 Oct 2026 00:27:55 +0300 Subject: [PATCH] fix(instances): ISO mount/unmount for every VM state and CD-ROM The old code edited the live XML, called attachDevice and redefined the domain from it: a stopped VM lost its VNC password, a paused VM crashed with UnboundLocalError, an unknown CD-ROM was silently ignored (on a running VM the last disk of the loop could be updated instead), and a missing ISO raised AttributeError. Media changes now use updateDeviceFlags on the requested CD-ROM, built from the live and the persistent definition separately. A CD-ROM missing from one definition, an unknown ISO, an ISO name present in more than one pool, or a change that reached only the running VM is reported to the user. The mount and unmount log messages were swapped. --- instances/test_live_iso.py | 96 ++++++++++++++++++++++++++++++++++++ instances/views.py | 27 +++++----- vrtManager/instance.py | 82 +++++++++++++++--------------- vrtManager/test_iso_media.py | 56 +++++++++++++++++++++ 4 files changed, 207 insertions(+), 54 deletions(-) create mode 100644 instances/test_live_iso.py create mode 100644 vrtManager/test_iso_media.py diff --git a/instances/test_live_iso.py b/instances/test_live_iso.py new file mode 100644 index 00000000..2a6f3f60 --- /dev/null +++ b/instances/test_live_iso.py @@ -0,0 +1,96 @@ +"""Live checks for ISO mount/unmount (R-12). Runs only with TEST_LIBVIRT_HOST set.""" + +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 +CDROMS = ( + "" + "" + "" +) +PERSISTENT = libvirt.VIR_DOMAIN_XML_INACTIVE | libvirt.VIR_DOMAIN_XML_SECURE + + +class LiveIsoTestCase(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) + pool = livetest.ensure_pool(cls.conn) + cls.inventory = livetest.inventory(cls.conn) + cls.iso = pool.createXML( + f"{P}media.iso1", 0 + ).path() + 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): + for dom in self.conn.listAllDomains(): + if dom.name().startswith(P): + if dom.isActive(): + dom.destroy() + dom.undefineFlags(livetest.UNDEFINE_FLAGS) + + def vm(self, state): + dom = livetest.define_vm(self.conn, P + "iso-" + state, [], extra_devices=CDROMS) + if state in ("running", "paused"): + dom.create() + if state == "paused": + dom.suspend() + return dom, wvmInstance(*self.args, None, uuid=dom.UUIDString()) + + def media(self, dom, flags): + tree = etree.fromstring(dom.XMLDesc(flags)) + return {d.find("target").get("dev"): d.xpath("string(source/@file)") for d in tree.xpath("devices/disk[@device='cdrom']")} + + def test_mount_and_unmount_in_every_state(self): + for state in ("stopped", "running", "paused"): + with self.subTest(state): + dom, vm = self.vm(state) + try: + vm.mount_iso("sdb", os.path.basename(self.iso)) + views = [PERSISTENT] + ([0] if dom.isActive() else []) + for flags in views: + self.assertEqual(self.media(dom, flags), {"sda": "", "sdb": self.iso}, f"flags={flags}") + self.assertIn("keep1234", dom.XMLDesc(PERSISTENT)) + + vm.umount_iso("sdb", self.iso) + for flags in views: + self.assertEqual(self.media(dom, flags), {"sda": "", "sdb": ""}, f"flags={flags}") + self.assertIn("keep1234", dom.XMLDesc(PERSISTENT)) + finally: + self.tearDown() + + def test_unknown_iso_or_device_changes_nothing(self): + dom, vm = self.vm("running") + before = (dom.XMLDesc(0), dom.XMLDesc(PERSISTENT)) + for dev, image in (("sdb", "no-such.iso"), ("sdz", os.path.basename(self.iso))): + with self.subTest(dev=dev, image=image): + with self.assertRaises(libvirt.libvirtError): + vm.mount_iso(dev, image) + self.assertEqual((dom.XMLDesc(0), dom.XMLDesc(PERSISTENT)), before) diff --git a/instances/views.py b/instances/views.py index 24ab3c1d..448396eb 100755 --- a/instances/views.py +++ b/instances/views.py @@ -1191,13 +1191,15 @@ def detach_cdrom(request, pk, dev): @serialize_instance_mutation def unmount_iso(request, pk): instance = get_instance(request.user, pk, perm_type="change") - image = request.POST.get("path", "") dev = request.POST.get("umount_iso", "") - instance.proxy.umount_iso(dev, image) - msg = _("Mount media: %(dev)s") % {"dev": dev} - addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - - return redirect(request.META.get("HTTP_REFERER") + "#disks") + try: + instance.proxy.umount_iso(dev) + except libvirtError as err: + messages.error(request, err) + else: + msg = _("Unmount media: %(dev)s") % {"dev": dev} + addlogmsg(request.user.username, instance.compute.name, instance.name, msg) + return redirect(_same_origin_referer(request, reverse("instances:instance", args=[pk])) + "#disks") @require_POST @@ -1207,11 +1209,14 @@ def mount_iso(request, pk): instance = get_instance(request.user, pk, perm_type="change") image = request.POST.get("media", "") dev = request.POST.get("mount_iso", "") - instance.proxy.mount_iso(dev, image) - msg = _("Unmount media: %(dev)s") % {"dev": dev} - addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - - return redirect(request.META.get("HTTP_REFERER") + "#disks") + try: + instance.proxy.mount_iso(dev, image) + except libvirtError as err: + messages.error(request, err) + else: + msg = _("Mount media: %(dev)s") % {"dev": dev} + addlogmsg(request.user.username, instance.compute.name, instance.name, msg) + return redirect(_same_origin_referer(request, reverse("instances:instance", args=[pk])) + "#disks") @require_POST diff --git a/vrtManager/instance.py b/vrtManager/instance.py index 4dea4753..2ea0bd52 100644 --- a/vrtManager/instance.py +++ b/vrtManager/instance.py @@ -707,52 +707,48 @@ class wvmInstance(wvmConnect): raise Exception("Invalid Device Type for boot order") self._defineXML(ElementTree.tostring(tree).decode()) + def _set_cdrom_media(self, dev, path): + """Insert path into, or with None eject, CD-ROM dev. + + The live and the persistent definition are updated separately from + their own device XML, so neither is rewritten from the other (R-12). + """ + definitions = [("persistent", PERSISTENT_XML, VIR_DOMAIN_AFFECT_CONFIG)] + if self.instance.isActive(): # running or paused + definitions.insert(0, ("running", VIR_DOMAIN_XML_SECURE, VIR_DOMAIN_AFFECT_LIVE)) + devices = [] + for label, xml_flags, _ in definitions: + tree = etree.fromstring(self._XMLDesc(xml_flags)) + found = tree.xpath("./devices/disk[@device='cdrom'][target/@dev=$dev]", dev=dev) + if not found: + raise libvirtError(f"CD-ROM {dev} is not in the {label} definition") + devices.append(found[0]) + for (label, _, affect), disk in zip(definitions, devices): + for source in disk.findall("source"): + disk.remove(source) + if path: + disk.insert(1, etree.Element("source", file=path)) + try: + self.instance.updateDeviceFlags(etree.tostring(disk).decode(), affect) + except libvirtError as err: + if label == "persistent" and len(definitions) == 2: + raise libvirtError(f"Media of {dev} changed in the running VM only: {err}") from err + raise + def mount_iso(self, dev, image): - def attach_iso(dev, disk, vol): - if disk.get("device") == "cdrom": - for elm in disk: - if elm.tag == "target" and elm.get("dev") == dev: - src_media = ElementTree.Element("source") - src_media.set("file", vol.path()) - disk.insert(2, src_media) - return True - - vol = None - storages = self.get_storages(only_actives=True) - for storage in storages: + paths = [] + for storage in self.get_storages(only_actives=True): stg = self.get_storage(storage) - if stg.info()[0] != 0: - for img in stg.listVolumes(): - if image == img: - vol = stg.storageVolLookupByName(image) - tree = ElementTree.fromstring(self._XMLDesc(0)) - for disk in tree.findall("devices/disk"): - if attach_iso(dev, disk, vol): - break - if self.get_status() == 1: - xml = ElementTree.tostring(disk).decode() - self.instance.attachDevice(xml) - xmldom = self._XMLDesc(VIR_DOMAIN_XML_SECURE) - if self.get_status() == 5: - xmldom = ElementTree.tostring(tree).decode() - self._defineXML(xmldom) + if image in stg.listVolumes(): + paths.append(stg.storageVolLookupByName(image).path()) + if not paths: + raise libvirtError(f"ISO image {image} was not found") + if len(paths) > 1: + raise libvirtError(f"ISO image {image} is in more than one storage pool") + self._set_cdrom_media(dev, paths[0]) - def umount_iso(self, dev, image): - tree = ElementTree.fromstring(self._XMLDesc(0)) - for disk in tree.findall("devices/disk"): - if disk.get("device") == "cdrom": - for elm in disk: - if elm.tag == "source" and elm.get("file") == image: - src_media = elm - if elm.tag == "target" and elm.get("dev") == dev: - disk.remove(src_media) - if self.get_status() == 1: - xml_disk = ElementTree.tostring(disk).decode() - self.instance.attachDevice(xml_disk) - xmldom = self._XMLDesc(VIR_DOMAIN_XML_SECURE) - if self.get_status() == 5: - xmldom = ElementTree.tostring(tree).decode() - self._defineXML(xmldom) + def umount_iso(self, dev, image=None): + self._set_cdrom_media(dev, None) def attach_disk( self, diff --git a/vrtManager/test_iso_media.py b/vrtManager/test_iso_media.py new file mode 100644 index 00000000..9c15d1f6 --- /dev/null +++ b/vrtManager/test_iso_media.py @@ -0,0 +1,56 @@ +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_AFFECT_CONFIG, VIR_DOMAIN_AFFECT_LIVE, libvirtError +from lxml import etree +from vrtManager.instance import wvmInstance + +DOMAIN = """ + + +""" + + +def vm(active): + inst = wvmInstance.__new__(wvmInstance) + inst.instance = MagicMock() + inst.instance.isActive.return_value = active + inst._XMLDesc = MagicMock(return_value=DOMAIN) + return inst + + +class IsoMediaTestCase(unittest.TestCase): + """R-12: media change goes through updateDeviceFlags for the right CD-ROM.""" + + def updates(self, inst): + return [ + (etree.fromstring(c.args[0]).find("target").get("dev"), etree.fromstring(c.args[0]).xpath("string(source/@file)"), c.args[1]) + for c in inst.instance.updateDeviceFlags.call_args_list + ] + + def test_stopped_vm_changes_only_the_persistent_definition(self): + inst = vm(active=False) + inst._set_cdrom_media("sdb", "/iso/a.iso") + self.assertEqual(self.updates(inst), [("sdb", "/iso/a.iso", VIR_DOMAIN_AFFECT_CONFIG)]) + + def test_running_vm_changes_live_then_persistent(self): + inst = vm(active=True) + inst._set_cdrom_media("sdb", None) + self.assertEqual( + self.updates(inst), [("sdb", "", VIR_DOMAIN_AFFECT_LIVE), ("sdb", "", VIR_DOMAIN_AFFECT_CONFIG)] + ) + + def test_unknown_cdrom_changes_nothing(self): + inst = vm(active=True) + with self.assertRaises(libvirtError): + inst._set_cdrom_media("sdz", "/iso/a.iso") + inst.instance.updateDeviceFlags.assert_not_called() + + +if __name__ == "__main__": + unittest.main()