From 11fe2409f905378ea1c9c2f3696204906098b25f Mon Sep 17 00:00:00 2001 From: catborise Date: Fri, 9 Oct 2026 17:33:09 +0300 Subject: [PATCH] fix(instances): device edits fit every definition and device type A volume of an LVM or iSCSI pool can be attached: by its own path, with the format its volume reports (raw when the pool knows none). Detaching a disk takes each definition's own description of it, so a disk only in the persistent definition goes too. Every NIC type can be a boot device; bridge and direct NICs were dropped from the boot order. The video model is a value, never parsed as XML. QoS changes clear the page's read cache. The NIC model "default" leaves the choice to libvirt instead of writing model "default", and rt18139 is rtl8139: a VM with either did not start (checked on a RHEL 10 host). A migration corrects the default NIC type setting. --- README.md | 1 + .../migrations/0016_nic_type_rtl8139.py | 22 ++++ appsettings/test_migrations.py | 16 ++- instances/test_disk_isolation.py | 47 ++++++++ instances/views.py | 11 +- vrtManager/connection.py | 2 +- vrtManager/create.py | 3 +- vrtManager/instance.py | 67 +++++------ vrtManager/test_device_edits.py | 110 ++++++++++++++++++ vrtManager/test_util.py | 8 ++ vrtManager/test_xml_safety.py | 3 +- vrtManager/util.py | 8 ++ 12 files changed, 250 insertions(+), 48 deletions(-) create mode 100644 appsettings/migrations/0016_nic_type_rtl8139.py create mode 100644 vrtManager/test_device_edits.py diff --git a/README.md b/README.md index 68518723..7f0436a2 100644 --- a/README.md +++ b/README.md @@ -538,6 +538,7 @@ A rollback leaves the clone on a detached HEAD at the restored commit; the updat - A VM's stats are polled every 5 s like the host graph, the first point as soon as the tab opens; each point is the average since the previous one, disk rates in MB/s (they were MiB/s labelled Mb/s); a counter the host does not report shows a gap instead of 0. - An ISO uploads to an SSH compute about 3.5 times faster (SFTP writes no longer wait for each 32 KiB); a failed write still fails the upload. - The VM page shows each NIC's own QoS and each CD-ROM's own pool (they showed the previous device's), and a NIC without a source or model no longer breaks it. +- A volume of an LVM or iSCSI pool can be attached to a VM, and bridge and direct NICs can be boot devices. The NIC model `default` lets libvirt choose, and `rt18139` is `rtl8139`: a VM with either did not start; a migration corrects the default NIC type setting. - A running VM without a guest agent shows the IPv4 address the host learned by ARP (the lookup always failed before). - After a live migration the VM's NVRAM file is deleted on the source, when it is in libvirt's `/var/lib/libvirt/qemu/nvram` and the destination does not share that directory (a VM created there later with the same name would start with those UEFI variables). - gstfsd finds the guest's operating system the way libguestfs inspects it, so a root on LVM works, and it always answers (an error when it finds no or several operating systems). Update `/usr/local/bin/gstfsd` on every compute (see [Adding a Compute Node](#adding-a-compute-node), step 3). diff --git a/appsettings/migrations/0016_nic_type_rtl8139.py b/appsettings/migrations/0016_nic_type_rtl8139.py new file mode 100644 index 00000000..d875a05e --- /dev/null +++ b/appsettings/migrations/0016_nic_type_rtl8139.py @@ -0,0 +1,22 @@ +from django.db import migrations + + +def fix_rtl8139(apps, schema_editor): + """The NIC type is rtl8139: a VM with model rt18139 does not start. Not + undone on the way back, as nothing needs the misspelling.""" + AppSettings = apps.get_model("appsettings", "AppSettings") + for setting in AppSettings.objects.using(schema_editor.connection.alias).filter(key="INSTANCE_NIC_DEFAULT_TYPE"): + setting.choices = setting.choices.replace("rt18139", "rtl8139") + if setting.value == "rt18139": + setting.value = "rtl8139" + setting.save() + + +class Migration(migrations.Migration): + dependencies = [ + ("appsettings", "0015_remove_clone_instance_auto_migrate"), + ] + + operations = [ + migrations.RunPython(fix_rtl8139, migrations.RunPython.noop), + ] diff --git a/appsettings/test_migrations.py b/appsettings/test_migrations.py index 8730c63c..1b2209f2 100644 --- a/appsettings/test_migrations.py +++ b/appsettings/test_migrations.py @@ -4,10 +4,11 @@ an id must not collide with them.""" import importlib import unittest +from types import SimpleNamespace from django.apps import apps from django.db import connection -from django.test import TransactionTestCase +from django.test import TestCase, TransactionTestCase from appsettings.models import AppSettings @@ -26,3 +27,16 @@ class SequenceAfterFixedIdsTestCase(TransactionTestCase): cursor.execute("SELECT setval(pg_get_serial_sequence('appsettings_appsettings', 'id'), 1, false)") login_lockout.add_settings(apps, schema_editor) self.assertEqual(AppSettings.objects.filter(key__startswith="LOGIN_").count(), 2) + + +nic_type = importlib.import_module("appsettings.migrations.0016_nic_type_rtl8139") + + +class NicTypeTestCase(TestCase): + def test_the_misspelt_nic_type_becomes_rtl8139(self): + setting = AppSettings.objects.get(key="INSTANCE_NIC_DEFAULT_TYPE") + self.assertEqual(setting.choices, "default,e1000,e1000e,rtl8139,virtio") + AppSettings.objects.filter(pk=setting.pk).update(value="rt18139", choices="default,e1000,e1000e,rt18139,virtio") + nic_type.fix_rtl8139(apps, SimpleNamespace(connection=connection)) + setting.refresh_from_db() + self.assertEqual((setting.value, setting.choices), ("rtl8139", "default,e1000,e1000e,rtl8139,virtio")) diff --git a/instances/test_disk_isolation.py b/instances/test_disk_isolation.py index 26ec1036..d5bbf559 100644 --- a/instances/test_disk_isolation.py +++ b/instances/test_disk_isolation.py @@ -191,6 +191,34 @@ class DiskViewsTenantIsolationTestCase(TestCase): self.assertEqual(res.status_code, 302) proxy.attach_disk.assert_not_called() + def test_add_existing_vol_attaches_the_volume_by_its_own_path(self): + self.client.force_login(self.superuser) + with patch("instances.models.wvmInstance") as mock_wvm, patch( + "instances.views.wvmStorage" + ) as mock_storage: + proxy = mock_wvm.return_value + proxy.get_disk_devices.return_value = [VM_DISK] + proxy.get_media_devices.return_value = [] + proxy.get_disk_bus_types.return_value = ["virtio", "sata", "scsi"] + proxy.get_cache_modes.return_value = {"default": "", "none": ""} + pool = mock_storage.return_value + pool.get_volumes.return_value = ["unit:0:0:1"] + pool.get_type.return_value = "iscsi" + pool.get_volume_type.return_value = "block" + pool.get_volume_format_type.return_value = None # an iSCSI pool knows no format + pool.get_target_path.return_value = "/dev/disk/by-path" + pool.get_volume.return_value.path.return_value = "/dev/disk/by-path/ip-192.0.2.1:3260-iscsi-iqn.example-lun-1" + res = self._post( + "add_existing_vol", + {"selected_storage": "iscsi", "vols": "unit:0:0:1", "bus": "virtio", "cache": "default"}, + ) + self.assertEqual(res.status_code, 302) + pool.get_volume.assert_called_once_with("unit:0:0:1") + self.assertEqual( + proxy.attach_disk.call_args.args[1], "/dev/disk/by-path/ip-192.0.2.1:3260-iscsi-iqn.example-lun-1" + ) + self.assertEqual(proxy.attach_disk.call_args.kwargs["format_type"], "raw") + def _mock_disk_options(self, proxy): proxy.get_disk_devices.return_value = [VM_DISK] proxy.get_media_devices.return_value = [] @@ -299,6 +327,25 @@ class DiskViewsTenantIsolationTestCase(TestCase): ) mock_create.return_value.create_volume.assert_not_called() + def test_add_new_vol_attaches_the_format_the_pool_created(self): + self.client.force_login(self.superuser) + with patch("instances.models.wvmInstance") as mock_wvm, patch( + "instances.views.wvmCreate" + ) as mock_create, patch("instances.views.wvmStorage") as mock_storage: + proxy = mock_wvm.return_value + self._mock_disk_options(proxy) + mock_create.return_value.create_volume.return_value = "/mnt/nfs/disk.img" + pool = mock_storage.return_value + pool.get_type.return_value = "netfs" + pool.get_volume_type.return_value = "file" + pool.get_volume_format_type.return_value = "raw" # a netfs pool creates raw + self._post( + "add_new_vol", + {"storage": "nfs", "name": "disk", "format": "qcow2", "size": "1", "bus": "virtio", "cache": "default"}, + ) + pool.get_volume_format_type.assert_called_once_with("disk.img") + self.assertEqual(proxy.attach_disk.call_args.kwargs["format_type"], "raw") + def test_edit_volume_accepts_a_disk_without_any_driver_format(self): self.client.force_login(self.superuser) no_format_disk = dict(VM_DISK, format=None) diff --git a/instances/views.py b/instances/views.py index f78d9aac..b8d214bc 100755 --- a/instances/views.py +++ b/instances/views.py @@ -1013,6 +1013,8 @@ def add_new_vol(request, pk): pool_type = conn_pool.get_type() disk_type = conn_pool.get_volume_type(os.path.basename(source)) + # the pool decides: only a dir pool keeps the asked format, the others create raw + format = util.vol_driver_type(conn_pool.get_volume_format_type(os.path.basename(source)), pool_type) if pool_type == "rbd": source_info = conn_pool.get_rbd_source() @@ -1067,18 +1069,19 @@ def add_existing_vol(request, pk): if name not in conn_create.get_volumes(): return reject_disk_options(request, pk, ["vols"]) - format_type = conn_create.get_volume_format_type(name) - disk_type = conn_create.get_volume_type(name) pool_type = conn_create.get_type() + format_type = util.vol_driver_type(conn_create.get_volume_format_type(name), pool_type) + disk_type = conn_create.get_volume_type(name) if pool_type == "rbd": source_info = conn_create.get_rbd_source() path = conn_create.get_source_name() + source = f"{path}/{name}" else: source_info = None - path = conn_create.get_target_path() + # an iSCSI volume's path is not the pool target plus its name + source = conn_create.get_volume(name).path() target_dev = utils.get_new_disk_dev(media, disks, bus) - source = f"{path}/{name}" instance.proxy.attach_disk( target_dev, diff --git a/vrtManager/connection.py b/vrtManager/connection.py index 6abe7efd..8001470b 100644 --- a/vrtManager/connection.py +++ b/vrtManager/connection.py @@ -811,7 +811,7 @@ class wvmConnect(object): """ :return: network card models """ - return ["default", "e1000", "e1000e","rt18139", "virtio"] + return ["default", "e1000", "e1000e", "rtl8139", "virtio"] def get_image_formats(self): """ diff --git a/vrtManager/create.py b/vrtManager/create.py index 91f846de..f3f47bed 100644 --- a/vrtManager/create.py +++ b/vrtManager/create.py @@ -87,8 +87,7 @@ class wvmCreate(wvmConnect): def get_volume_format_type(self, path): vol = self.get_volume_by_path(path) - vol_type = util.get_xml_path(vol.XMLDesc(0), "/volume/target/format/@type") - return "raw" if vol_type in ["unknown", "iso"] else vol_type or "raw" + return util.vol_driver_type(util.get_xml_path(vol.XMLDesc(0), "/volume/target/format/@type")) def get_volume_path(self, volume, pool=None): storages = [pool] if pool else self.get_storages(only_actives=True) diff --git a/vrtManager/instance.py b/vrtManager/instance.py index 8cc33652..c1d4d937 100644 --- a/vrtManager/instance.py +++ b/vrtManager/instance.py @@ -950,26 +950,14 @@ class wvmInstance(wvmConnect): for idx, dev in devorder.items(): order = etree.fromstring("".format(idx + 1)) - if dev["type"] == "disk": - devices = tree.findall("./devices/disk[@device='disk']") - for d in devices: - device = d.find("./target[@dev='{}']".format(dev["dev"])) - if device is not None: - d.append(order) - elif dev["type"] == "cdrom": - devices = tree.findall("./devices/disk[@device='cdrom']") - for d in devices: - device = d.find("./target[@dev='{}']".format(dev["dev"])) - if device is not None: - d.append(order) - elif dev["type"] == "network": - devices = tree.findall("./devices/interface[@type='network']") - for d in devices: - device = d.find("mac[@address='{}']".format(dev["dev"])) - if device is not None: - d.append(order) + if dev["type"] in ("disk", "cdrom"): + devices = tree.xpath("./devices/disk[@device=$device][target/@dev=$dev]", device=dev["type"], dev=dev["dev"]) + elif dev["type"] == "network": # any NIC type: network, bridge, direct + devices = tree.xpath("./devices/interface[mac/@address=$mac]", mac=dev["dev"]) else: raise Exception("Invalid Device Type for boot order") + for d in devices: + d.append(order) self._defineXML(etree.tostring(tree).decode()) def _set_cdrom_media(self, dev, path): @@ -1068,6 +1056,8 @@ class wvmInstance(wvmConnect): if disk_type == "file": xml_disk += f"" + elif disk_type == "block": # a volume of an LVM, disk or iSCSI pool + xml_disk += f"" elif disk_type == "network": if pool_type == "rbd": auth_type = source_info.get("auth_type") @@ -1100,17 +1090,16 @@ class wvmInstance(wvmConnect): self.instance.attachDeviceFlags(xml_disk, affect) def detach_disk(self, target_dev): - tree = etree.fromstring(self._XMLDesc(0)) - - disk_el = tree.xpath("./devices/disk/target[@dev=$dev]", dev=target_dev)[ - 0 - ].getparent() - xml_disk = etree.tostring(disk_el).decode() - - # Paused and other active states need the live detach too. - if self.instance.isActive(): - self.instance.detachDeviceFlags(xml_disk, VIR_DOMAIN_AFFECT_LIVE) - self.instance.detachDeviceFlags(xml_disk, VIR_DOMAIN_AFFECT_CONFIG) + """Detach target_dev from each definition that has it, as that definition describes it.""" + found = False + for xml_flags, affect in self._definitions(): + tree = etree.fromstring(self._XMLDesc(xml_flags)) + disks = tree.xpath("./devices/disk[target/@dev=$dev]", dev=target_dev) + if disks: + found = True + self.instance.detachDeviceFlags(etree.tostring(disks[0]).decode(), affect) + if not found: + raise util.OperationError(f"Disk {target_dev} is not in the VM's definition") def has_disk(self, target_dev): """True while the live or the persistent definition still has target_dev.""" @@ -1502,13 +1491,13 @@ class wvmInstance(wvmConnect): xml = self._XMLDesc(PERSISTENT_XML) tree = etree.fromstring(xml) video_models = tree.xpath("/domain/devices/video/model") - video_xml = "".format(model) - for model in video_models: - if model.get("primary") == "yes" or len(video_models) == 1: - parent = model.getparent() - parent.remove(model) - parent.append(etree.fromstring(video_xml)) + for video_model in video_models: + if video_model.get("primary") == "yes" or len(video_models) == 1: + parent = video_model.getparent() + parent.remove(video_model) + etree.SubElement(parent, "model", type=model) self._defineXML(etree.tostring(tree).decode()) + return def resize_cpu(self, cur_vcpu, vcpu): """ @@ -1968,7 +1957,7 @@ class wvmInstance(wvmConnect): etree.SubElement(iface, "source", dev=source, mode="bridge") else: etree.SubElement(iface, "source", bridge=source) - if model: + if model and model != "default": # default: no model, libvirt chooses etree.SubElement(iface, "model", type=model) if nwfilter: etree.SubElement(iface, "filterref", filter=nwfilter) @@ -2027,7 +2016,7 @@ class wvmInstance(wvmConnect): iface.find("mac").set("address", mac) for old_model in iface.findall("model"): iface.remove(old_model) - if model: + if model and model != "default": # default: no model, libvirt chooses etree.SubElement(iface, "model", type=model) for old_filter in iface.findall("filterref"): iface.remove(old_filter) @@ -2204,7 +2193,7 @@ class wvmInstance(wvmConnect): else: band.append(etree.fromstring(xml)) new_xml = etree.tostring(tree).decode() - self.wvm.defineXML(new_xml) + self._defineXML(new_xml) def unset_qos(self, mac, direction): tree = etree.fromstring(self._XMLDesc(PERSISTENT_XML)) @@ -2219,7 +2208,7 @@ class wvmInstance(wvmConnect): if parent_mac[0] == mac: band_el.remove(direct) - self.wvm.defineXML(etree.tostring(tree).decode()) + self._defineXML(etree.tostring(tree).decode()) def add_guest_agent(self): channel_xml = """ diff --git a/vrtManager/test_device_edits.py b/vrtManager/test_device_edits.py new file mode 100644 index 00000000..856a4bd8 --- /dev/null +++ b/vrtManager/test_device_edits.py @@ -0,0 +1,110 @@ +"""Device edits of a VM: each definition is changed as it describes the +device, every NIC type can be a boot device, posted values stay values, and +definition changes go through _defineXML.""" + +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 import util +from vrtManager.instance import PERSISTENT_XML, wvmInstance +from libvirt import VIR_DOMAIN_AFFECT_CONFIG, VIR_DOMAIN_AFFECT_LIVE, VIR_DOMAIN_XML_SECURE + +PERSISTENT = """hvm + + + + + + +""" +LIVE = """ + +""" + + +def proxy(active=True): + p = wvmInstance.__new__(wvmInstance) # no connection: only these calls + p.instance = MagicMock(**{"isActive.return_value": active}) + p._XMLDesc = lambda flags: PERSISTENT if flags == PERSISTENT_XML else LIVE + p._defineXML = MagicMock() + p.wvm = MagicMock() + return p + + +def defined(p): + (xml,), _ = p._defineXML.call_args + return etree.fromstring(xml) + + +class DetachDiskTestCase(unittest.TestCase): + def test_each_definition_is_detached_as_it_describes_the_disk(self): + p = proxy() + p.detach_disk("vda") + (live, live_flags), (config, config_flags) = (c.args for c in p.instance.detachDeviceFlags.call_args_list) + self.assertEqual((live_flags, config_flags), (VIR_DOMAIN_AFFECT_LIVE, VIR_DOMAIN_AFFECT_CONFIG)) + self.assertIn("virtio-disk0", live) + self.assertNotIn("alias", config) + + def test_a_disk_only_in_the_persistent_definition_is_detached_there(self): + p = proxy() + p.detach_disk("vdb") + self.assertEqual([c.args[1] for c in p.instance.detachDeviceFlags.call_args_list], [VIR_DOMAIN_AFFECT_CONFIG]) + + def test_an_unknown_disk_is_refused(self): + p = proxy() + with self.assertRaises(util.OperationError): + p.detach_disk("vdz") + p.instance.detachDeviceFlags.assert_not_called() + + +class DeviceEditsTestCase(unittest.TestCase): + def test_a_block_volume_is_attached_by_its_device_path(self): + p = proxy(active=False) + p._definitions = lambda: [(PERSISTENT_XML, VIR_DOMAIN_AFFECT_CONFIG)] + p.attach_disk("vdc", "/dev/vg/c", disk_type="block", target_bus="virtio") + xml, _ = p.instance.attachDeviceFlags.call_args.args + self.assertEqual(etree.fromstring(xml).find("source").attrib, {"dev": "/dev/vg/c"}) + + def test_a_bridge_nic_can_be_a_boot_device(self): + p = proxy() + p.set_bootorder({0: {"type": "network", "dev": "52:54:10:00:00:02"}, 1: {"type": "disk", "dev": "vdb"}}) + tree = defined(p) + self.assertEqual(tree.xpath("devices/interface[@type='bridge']/boot/@order"), ["1"]) + self.assertEqual(tree.xpath("devices/disk[target/@dev='vdb']/boot/@order"), ["2"]) + + def test_the_video_model_is_a_value(self): + p = proxy() + p.set_video_model("vga' heads='9") + model = defined(p).find("devices/video/model") + self.assertEqual(model.attrib, {"type": "vga' heads='9"}) + p._defineXML.assert_called_once() + + def test_qos_changes_go_through_define_xml(self): + for change in ( + lambda p: p.set_qos("52:54:10:00:00:01", "outbound", 1, 2, 3), + lambda p: p.unset_qos("52:54:10:00:00:01", "inbound"), + ): + p = proxy() + change(p) + p._defineXML.assert_called_once() + p.wvm.defineXML.assert_not_called() + + def test_the_default_nic_model_leaves_the_choice_to_libvirt(self): + p = proxy() + p.change_network("52:54:10:00:00:01", "52:54:10:00:00:01", "default", "net", "default", "") + self.assertIsNone(defined(p).find("devices/interface[@type='network']/model")) + p = proxy(active=False) + p.add_network("52:54:10:00:00:09", "default", model="default") + xml, _ = p.instance.attachDeviceFlags.call_args.args + self.assertIsNone(etree.fromstring(xml).find("model")) + + +if __name__ == "__main__": + unittest.main() diff --git a/vrtManager/test_util.py b/vrtManager/test_util.py index b1279563..137e076d 100644 --- a/vrtManager/test_util.py +++ b/vrtManager/test_util.py @@ -17,6 +17,7 @@ from vrtManager.util import ( validate_macaddr, validate_uuid, vol_dev_type, + vol_driver_type, xml_escape, ) @@ -128,6 +129,13 @@ class TestVrtManagerUtil(unittest.TestCase): self.assertEqual(vol_dev_type("sata"), "sd") self.assertIsNone(vol_dev_type("unknown_bus")) + def test_vol_driver_type(self): + for vol_format in ("qcow2", "qcow", "raw"): + self.assertEqual(vol_driver_type(vol_format), vol_format) + for vol_format in (None, "", "none", "unknown", "iso"): + self.assertEqual(vol_driver_type(vol_format), "raw") + self.assertEqual(vol_driver_type("linux-lvm", "disk"), "raw") # a partition + class OperationErrorTestCase(unittest.TestCase): diff --git a/vrtManager/test_xml_safety.py b/vrtManager/test_xml_safety.py index d96b41e5..3703963f 100644 --- a/vrtManager/test_xml_safety.py +++ b/vrtManager/test_xml_safety.py @@ -6,6 +6,7 @@ from django.conf import settings if not settings.configured: settings.configure(MAC_OUI="52:54:10") +from vrtManager import util from vrtManager.instance import wvmInstance from vrtManager.network import wvmNetworks, wvmNetwork @@ -291,7 +292,7 @@ class TestDiskXmlEscaping(unittest.TestCase): "" ) ) - with self.assertRaises(IndexError): + with self.assertRaises(util.OperationError): inst.detach_disk("x' or '1'='1") inst.instance.detachDeviceFlags.assert_not_called() diff --git a/vrtManager/util.py b/vrtManager/util.py index c1bdb84f..30d7b6d2 100644 --- a/vrtManager/util.py +++ b/vrtManager/util.py @@ -216,3 +216,11 @@ def vol_dev_type(vol_bus): elif vol_bus in ["scsi", "sata", "usb"]: return "sd" + +def vol_driver_type(vol_format, pool_type=None): + """The disk driver type for a volume of vol_format: raw for a volume + whose pool knows no image format of it (none, unknown, an ISO, a disk + pool's partition), as libvirt uses a disk without a driver type.""" + if pool_type == "disk" or vol_format in (None, "", "none", "unknown", "iso"): + return "raw" + return vol_format