diff --git a/instances/tests/test_pci_slots.py b/instances/tests/test_pci_slots.py new file mode 100644 index 00000000..0efeac87 --- /dev/null +++ b/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]) diff --git a/instances/views.py b/instances/views.py index f7a0b927..c04afb13 100755 --- a/instances/views.py +++ b/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") diff --git a/vrtManager/create.py b/vrtManager/create.py index 72e371d9..f60cfe0a 100644 --- a/vrtManager/create.py +++ b/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""" + """ + # 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 += "" + xml += "" * PCIE_ROOT_PORTS + xml += """ """ return self._defineXML(xml)