fix(instances): new q35 VMs have spare PCIe ports, and a full VM says what to do

On a q35 machine each PCIe device (a virtio disk, a network card) takes
a root port of its own, libvirt makes only as many ports as the VM is
defined with, and a running VM gets no new one: adding a virtio disk to
such a VM failed with libvirt's "No more available PCI slots".

A new q35 VM is now defined with the PCIe root and 14 root ports, as
virt-install makes them: libvirt puts the VM's devices into them and
leaves the rest free for devices added while it runs.

When a running VM has no free slot, Add disk and Add network device say
what to do instead of libvirt's words: for a disk, the scsi bus when
the VM has a SCSI controller (its disks need no slot of their own), or
adding it with the VM shut down, where libvirt makes the port; for a
network device, shutting the VM down. A new volume whose attach was
refused stays in its pool, as before, to attach as an existing volume.

instances/tests/test_pci_slots.py

@@ -0,0 +1,104 @@
+"""A running q35 VM gives each PCIe device (a virtio disk, a NIC) a root port
+of its own and gets no new one: a new VM has spare ports, as virt-install
+makes them, and a refusal for want of one says what to do instead. A new
+volume whose attach fails stays in its pool: it is never deleted unasked."""
+
+from unittest.mock import MagicMock, patch
+
+from computes.models import Compute
+from django.contrib.auth import get_user_model
+from django.contrib.messages import get_messages
+from django.test import SimpleTestCase, TestCase
+from django.urls import reverse
+from instances.models import Instance
+from logs.models import Logs
+from libvirt import libvirtError
+from lxml import etree
+from vrtManager.create import PCIE_ROOT_PORTS, wvmCreate
+
+DOM_CAPS = {"domain": "kvm", "os_support": "no", "cpu_modes": ["host-model"], "cpu_custom_models": [],
+            "disk_bus": ["virtio", "sata", "scsi"]}
+NO_SLOT = libvirtError("internal error: No more available PCI slots")
+
+
+def defined_controllers(machine):
+    conn = wvmCreate.__new__(wvmCreate)
+    conn.wvm = MagicMock()
+    with patch.object(wvmCreate, "get_capabilities", return_value={"features": []}), \
+            patch.object(wvmCreate, "get_dom_capabilities", return_value=DOM_CAPS):
+        conn.create_instance(name="vm-1", memory=512, vcpu=1, vcpu_mode="host-model",
+                             uuid="00000000-0000-4000-8000-0000000000c2", arch="x86_64", machine=machine,
+                             firmware={}, volumes=[], networks="default", nwfilter="", virtio=True,
+                             listener_addr="0.0.0.0", add_cdrom="None", add_input="None")
+    domain = etree.fromstring(conn.wvm.defineXML.call_args.args[0])
+    return [c.get("model") for c in domain.findall("devices/controller[@type='pci']")]
+
+
+class NewVmPortsTests(SimpleTestCase):
+    def test_a_new_q35_vm_has_spare_root_ports(self):
+        # libvirt puts the VM's devices into these and adds none: the rest are free
+        self.assertEqual(defined_controllers("pc-q35-rhel9.8.0"), ["pcie-root"] + ["pcie-root-port"] * PCIE_ROOT_PORTS)
+
+    def test_a_pc_vm_has_none_its_pci_bus_has_slots(self):
+        self.assertEqual(defined_controllers("pc-i440fx-rhel7.6.0"), [])
+
+
+class NoPciSlotTests(TestCase):
+    def setUp(self):
+        self.client.force_login(get_user_model().objects.create_superuser("ps-admin", "ps@example.com", "x"))
+        compute = Compute.objects.create(name="ps", hostname="127.0.0.1:1", login="root", password="", type=1)
+        self.instance = Instance.objects.create(compute=compute, name="ps-vm",
+                                                uuid="aaaaaaaa-7777-2222-3333-444444444444")
+
+    def post(self, view, data, error):
+        with patch("instances.models.wvmInstance") as wvm, patch("instances.views.wvmStorage") as storage, \
+                patch("instances.views.wvmCreate") as create:
+            proxy = wvm.return_value
+            proxy.get_disk_devices.return_value = proxy.get_media_devices.return_value = []
+            proxy.get_disk_bus_types.return_value = ["virtio", "sata", "scsi"]
+            proxy.get_cache_modes.return_value = {"default": ""}
+            proxy.get_status.return_value = 1
+            proxy.attach_disk.side_effect = proxy.add_network.side_effect = error
+            storage.return_value.get_volumes.return_value = ["data.qcow2"]
+            storage.return_value.get_type.return_value = "dir"
+            storage.return_value.get_volume_type.return_value = "file"
+            storage.return_value.get_volume_format_type.return_value = "qcow2"
+            create.return_value.create_volume.return_value = "/pool/data.qcow2"
+            response = self.client.post(reverse(f"instances:{view}", args=[self.instance.id]), data,
+                                        HTTP_REFERER=reverse("instances:instance", args=[self.instance.id]))
+        return [str(m) for m in get_messages(response.wsgi_request)], storage.return_value.del_volume
+
+    def test_a_new_disk_without_a_slot_is_told_the_scsi_bus_and_its_volume_stays(self):
+        texts, del_volume = self.post("add_new_vol", {"storage": "default", "name": "data", "format": "qcow2",
+                                                      "size": "1", "cache": "default", "bus": "virtio"}, NO_SLOT)
+        self.assertEqual(len(texts), 1)
+        self.assertIn("choose the scsi bus", texts[0])
+        del_volume.assert_not_called()
+        # the refusal is logged as failed
+        self.assertFalse(Logs.objects.get(instance=self.instance.name).success)
+
+    def test_a_scsi_disk_without_a_slot_for_its_controller_is_told_to_shut_the_vm_down(self):
+        texts, _del_volume = self.post("add_new_vol", {"storage": "default", "name": "data", "format": "qcow2",
+                                                       "size": "1", "cache": "default", "bus": "scsi"}, NO_SLOT)
+        self.assertEqual(len(texts), 1)
+        self.assertIn("the SCSI controller the disk needs", texts[0])
+        self.assertNotIn("choose the scsi bus", texts[0])
+
+    def test_another_refusal_is_libvirts(self):
+        texts, del_volume = self.post("add_new_vol", {"storage": "default", "name": "data", "format": "qcow2",
+                                                      "size": "1", "cache": "default", "bus": "virtio"},
+                                      libvirtError("internal error: something else"))
+        self.assertEqual(texts, ["libvirt Error - internal error: something else"])
+        del_volume.assert_not_called()
+
+    def test_an_existing_volume_without_a_slot_is_told_the_scsi_bus_and_stays(self):
+        texts, del_volume = self.post("add_existing_vol", {"selected_storage": "default", "vols": "data.qcow2",
+                                                           "cache": "default", "bus": "virtio"}, NO_SLOT)
+        self.assertIn("choose the scsi bus", texts[0])
+        del_volume.assert_not_called()
+
+    def test_a_network_device_without_a_slot_is_told_to_shut_the_vm_down(self):
+        texts, _del_volume = self.post("add_network", {"add-net-network": "net:default", "add-net-model": "virtio"},
+                                       NO_SLOT)
+        self.assertEqual(len(texts), 1)
+        self.assertIn("shut it down", texts[0])

instances/views.py

@@ -711,6 +711,26 @@ def invalid_disk_options(
     ]
 
 
+# libvirt's words when a running q35 VM has no free root port for a PCIe device
+PCI_SLOTS_FULL = "No more available PCI slots"
+# a disk on the scsi bus needs no slot of its own when the VM already has a SCSI controller
+DISK_NO_PCI_SLOT = _("The running VM has no free PCI slot for another disk on this bus: choose the scsi bus "
+                     "if the VM has a SCSI controller, or shut the VM down and add the disk then. A new volume "
+                     "stays in its pool, to attach as an existing volume.")
+SCSI_NO_PCI_SLOT = _("The running VM has no free PCI slot for the SCSI controller the disk needs: shut the VM "
+                     "down and add the disk then. A new volume stays in its pool, to attach as an existing volume.")
+
+
+def no_pci_slot(request, err, advice):
+    """True, with advice shown, when libvirt refused a running VM's new device
+    for want of a PCI slot: q35 gives each PCIe device a root port of its own
+    and a running VM gets no new one."""
+    if PCI_SLOTS_FULL not in str(err):
+        return False
+    messages.error(request, advice)
+    return True
+
+
 def reject_disk_options(request, pk, invalid):
     messages.error(
         request,
@@ -1255,57 +1275,62 @@ def add_new_vol(request, pk):
     if invalid:
         return reject_disk_options(request, pk, invalid)
 
-    with audit_vm(request, instance, f"attach new disk {storage}/{name}"):
-        conn_create = wvmCreate(
-            instance.compute.hostname,
-            instance.compute.login,
-            instance.compute.password,
-            instance.compute.type,
-        )
-        # a name either definition uses is taken: on an active VM a disk can be
-        # pending in the persistent one only
-        target_dev = utils.get_new_disk_dev(
-            instance.media + instance.config_media, instance.disks + instance.config_disks, bus
-        )
+    try:
+        with audit_vm(request, instance, f"attach new disk {storage}/{name}"):
+            conn_create = wvmCreate(
+                instance.compute.hostname,
+                instance.compute.login,
+                instance.compute.password,
+                instance.compute.type,
+            )
+            # a name either definition uses is taken: on an active VM a disk can be
+            # pending in the persistent one only
+            target_dev = utils.get_new_disk_dev(
+                instance.media + instance.config_media, instance.disks + instance.config_disks, bus
+            )
 
-        source = conn_create.create_volume(
-            storage,
-            name,
-            size,
-            format,
-            meta_prealloc,
-            int(app_settings.INSTANCE_VOLUME_DEFAULT_OWNER_UID),
-            int(app_settings.INSTANCE_VOLUME_DEFAULT_OWNER_GID),
-        )
+            source = conn_create.create_volume(
+                storage,
+                name,
+                size,
+                format,
+                meta_prealloc,
+                int(app_settings.INSTANCE_VOLUME_DEFAULT_OWNER_UID),
+                int(app_settings.INSTANCE_VOLUME_DEFAULT_OWNER_GID),
+            )
 
-        conn_pool = wvmStorage(
-            instance.compute.hostname,
-            instance.compute.login,
-            instance.compute.password,
-            instance.compute.type,
-            storage,
-        )
+            conn_pool = wvmStorage(
+                instance.compute.hostname,
+                instance.compute.login,
+                instance.compute.password,
+                instance.compute.type,
+                storage,
+            )
 
-        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)
+            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()
-        else:  # add more disk types to handle different pool and disk types
-            source_info = None
+            if pool_type == "rbd":
+                source_info = conn_pool.get_rbd_source()
+            else:  # add more disk types to handle different pool and disk types
+                source_info = None
 
-        instance.proxy.attach_disk(
-            target_dev,
-            source,
-            source_info=source_info,
-            pool_type=pool_type,
-            disk_type=disk_type,
-            target_bus=bus,
-            format_type=format,
-            cache_mode=cache,
-        )
+            instance.proxy.attach_disk(
+                target_dev,
+                source,
+                source_info=source_info,
+                pool_type=pool_type,
+                disk_type=disk_type,
+                target_bus=bus,
+                format_type=format,
+                cache_mode=cache,
+            )
+    except libvirtError as err:
+        # the new volume stays in the pool, to attach from Existing volume: it is never deleted unasked
+        if not no_pci_slot(request, err, SCSI_NO_PCI_SLOT if bus == "scsi" else DISK_NO_PCI_SLOT):
+            raise
     return _back(request, pk, "disks")
 
 
@@ -1352,17 +1377,21 @@ def add_existing_vol(request, pk):
         instance.media + instance.config_media, instance.disks + instance.config_disks, bus
     )
 
-    with audit_vm(request, instance, f"attach disk {storage}/{name} as {target_dev}"):
-        instance.proxy.attach_disk(
-            target_dev,
-            source,
-            source_info=source_info,
-            pool_type=pool_type,
-            disk_type=disk_type,
-            target_bus=bus,
-            format_type=format_type,
-            cache_mode=cache,
-        )
+    try:
+        with audit_vm(request, instance, f"attach disk {storage}/{name} as {target_dev}"):
+            instance.proxy.attach_disk(
+                target_dev,
+                source,
+                source_info=source_info,
+                pool_type=pool_type,
+                disk_type=disk_type,
+                target_bus=bus,
+                format_type=format_type,
+                cache_mode=cache,
+            )
+    except libvirtError as err:
+        if not no_pci_slot(request, err, SCSI_NO_PCI_SLOT if bus == "scsi" else DISK_NO_PCI_SLOT):
+            raise
     return _back(request, pk, "disks")
 
 
@@ -1904,17 +1933,22 @@ def add_network(request, pk):
     (source, source_type) = utils.get_network_tuple(network)
     model = request.POST.get("add-net-model")
 
-    with audit_vm(request, instance, f"add interface on {source}"):
-        if source_type == "iface":
-            iface = wvmInterface(
-                instance.compute.hostname,
-                instance.compute.login,
-                instance.compute.password,
-                instance.compute.type,
-                source,
-            )
-            source_type = iface.get_type()
-        instance.proxy.add_network(mac, source, source_type, model=model, nwfilter=nwfilter)
+    try:
+        with audit_vm(request, instance, f"add interface on {source}"):
+            if source_type == "iface":
+                iface = wvmInterface(
+                    instance.compute.hostname,
+                    instance.compute.login,
+                    instance.compute.password,
+                    instance.compute.type,
+                    source,
+                )
+                source_type = iface.get_type()
+            instance.proxy.add_network(mac, source, source_type, model=model, nwfilter=nwfilter)
+    except libvirtError as err:
+        if not no_pci_slot(request, err, _("The running VM has no free PCI slot for another network device: "
+                                           "shut it down and add the device then.")):
+            raise
     return _back(request, pk, "network")
 
 

vrtManager/create.py

@@ -9,6 +9,8 @@ from vrtManager.connection import wvmConnect
 
 # libvirt's CPU modes a new VM can have; any other value is a CPU model's name (mode custom)
 CPU_MODES = ("host-model", "host-passthrough", "maximum")
+# the PCIe root ports of a new q35 VM, its devices' and the spare ones together
+PCIE_ROOT_PORTS = 14
 
 
 def cpu_choices(dom_caps):
@@ -379,7 +381,14 @@ class wvmCreate(wvmConnect):
 
         xml += f""" <video>
                       <model type='{video}'/>
-                   </video>
+                   </video>"""
+        # q35 plugs each PCIe device (a virtio disk, a NIC) into a root port of
+        # its own and libvirt makes only as many as the VM is defined with: as
+        # virt-install, 14 ports, so a running VM can take more devices
+        if "q35" in machine:
+            xml += "<controller type='pci' model='pcie-root'/>"
+            xml += "<controller type='pci' model='pcie-root-port'/>" * PCIE_ROOT_PORTS
+        xml += """
               </devices>
             </domain>"""
         return self._defineXML(xml)