fix(instances): a new disk on a running VM does not take a pending disk's name

The device name of a new disk or CD-ROM was chosen from the running VM
only. A name used only in the persistent definition, such as a disk
renamed by a pending bus change, could be chosen again: the disk was
attached to the running VM, the persistent attach was refused, and the
disk was gone after the next start. Names either definition uses are now
taken.

README.md

@@ -541,6 +541,7 @@ A rollback leaves the clone on a detached HEAD at the restored commit; the updat
 - 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.
 - On a host whose libvirt cannot change network interfaces (the udev backend: RHEL 9 and later, Debian 12), the interface pages offer no create, start, stop or delete; they only failed there.
 - On a running VM the disk list, its edit form, the QoS table, the title, description, CPU, boot order, video and console fields show the settings as they will be at the next start, so saving an edit no longer reverts another pending change. A boot order that names a device missing from that definition is refused instead of clearing the order. The settings tab of a running VM says so. A QoS edit keeps the inbound floor. Buttons shown as disabled while the VM runs can no longer be submitted with Enter.
+- A new disk or CD-ROM on a running VM gets a device name that neither the running VM nor its next-start settings use; it could take the name of a pending disk and stay attached until the next start only.
 - 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 paused VM's memory is resized as a running VM's: the current memory changes and the guest takes it once resumed, the maximum stays (both only reached the VM at its next boot).

instances/test_live_dataloss.py

@@ -429,6 +429,40 @@ class LiveDataLossTestCase(TestCase):
         self.assertEqual(tree.xpath("devices/disk[target/@dev='vda']/boot/@order"), ["2"])
         self.assertEqual(dom.XMLDesc(0), live)
 
+    def test_a_new_disk_does_not_take_a_name_pending_in_the_persistent_definition(self):
+        from lxml import html
+
+        disks = [livetest.create_volume(self.conn, P + n) for n in ("name-a", "name-b")]
+        dom, inst = self.vm("disk-name", disks, extra_devices="<controller type='scsi' model='virtio-scsi'/>")
+        dom.create()
+        # vdb becomes sata: renamed sda in the persistent definition only
+        page = self.client.get(reverse("instances:instance", args=[inst.id])).content
+        action = reverse("instances:edit_volume", args=[inst.id])
+        (form,) = html.fromstring(page).xpath("//form[@action=$a][.//input[@name='dev'][@value='vdb']]", a=action)
+        self.post("edit_volume", inst, {**dict(form.form_values()), "edit_volume": "", "vol_bus": "sata"})
+        self.assertEqual(self.disk_sources(dom, libvirt.VIR_DOMAIN_XML_INACTIVE).get("sda"), disks[1])
+
+        # scsi shares the sd names: the new disk must not be sda
+        self.post("add_new_vol", inst, {"storage": livetest.POOL, "name": P + "name-new", "format": "qcow2",
+                                        "size": "1", "bus": "scsi", "cache": "default"})
+        new = f"{livetest.POOL_PATH}/{P}name-new.qcow2"
+        self.assertEqual(self.disk_sources(dom).get("sdb"), new)
+        self.assertEqual(self.disk_sources(dom, libvirt.VIR_DOMAIN_XML_INACTIVE).get("sdb"), new)
+        dom.destroy()
+        dom.create()
+        self.assertEqual(self.disk_sources(dom), {"vda": disks[0], "sda": disks[1], "sdb": new})
+
+    def test_a_sata_disk_cannot_be_added_to_a_running_vm_and_nothing_changes(self):
+        dom, inst = self.vm("sata-hotplug", [livetest.create_volume(self.conn, P + "sata-a")])
+        dom.create()
+        before = (self.disk_sources(dom), self.disk_sources(dom, libvirt.VIR_DOMAIN_XML_INACTIVE))
+        response = self.post("add_new_vol", inst, {"storage": livetest.POOL, "name": P + "sata-new", "format": "qcow2",
+                                                   "size": "1", "bus": "sata", "cache": "default"})
+        self.assertEqual(response.status_code, 500)
+        self.assertEqual((self.disk_sources(dom), self.disk_sources(dom, libvirt.VIR_DOMAIN_XML_INACTIVE)), before)
+        # the new volume stays in the pool: it is never deleted unasked
+        self.assertIn(P + "sata-new.qcow2", self.conn.storagePoolLookupByName(livetest.POOL).listVolumes())
+
     def test_pending_qos_change_is_shown_and_kept_by_the_next_edit(self):
         from lxml import etree, html
 

instances/test_new_disk_name.py

@@ -0,0 +1,63 @@
+"""A new disk or CD-ROM gets a name neither definition uses: on an active
+VM a disk can be pending in the persistent definition only, or still in the
+running VM only, and the attach goes to both."""
+
+from unittest.mock import patch
+
+from computes.models import Compute
+from django.contrib.auth import get_user_model
+from django.test import TestCase
+from django.urls import reverse
+from instances.models import Instance
+
+
+def dev(name, bus):
+    return {"dev": name, "bus": bus, "image": f"{name}.qcow2", "storage": "default", "path": f"/pool/{name}.qcow2",
+            "format": "qcow2", "size": 1 << 30}
+
+
+class NewDiskNameTestCase(TestCase):
+    def setUp(self):
+        self.client.force_login(get_user_model().objects.create_superuser("nd-admin", "nd@example.com", "x"))
+        compute = Compute.objects.create(name="nd", hostname="127.0.0.1:1", login="root", password="", type=1)
+        self.instance = Instance.objects.create(compute=compute, name="nd-vm", uuid="aaaaaaaa-5555-2222-3333-444444444444")
+
+    def attached_name(self, view, data, live=((), ()), config=((), ())):
+        """The name view attaches with; live and config are (disks, media) of each definition."""
+        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.side_effect = lambda **kw: list((config if kw.get("config") else live)[0])
+            proxy.get_media_devices.side_effect = lambda **kw: list((config if kw.get("config") else live)[1])
+            proxy.get_disk_bus_types.return_value = ["virtio", "sata", "scsi"]
+            proxy.get_cache_modes.return_value = {"default": "", "none": ""}
+            proxy.get_machine_type.return_value = "q35"
+            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"
+            self.client.post(reverse(f"instances:{view}", args=[self.instance.id]), data)
+        proxy.attach_disk.assert_called_once()
+        return proxy.attach_disk.call_args.args[0]
+
+    VIEWS = (
+        ("add_new_vol", {"storage": "default", "name": "data", "format": "qcow2", "size": "1", "cache": "default"}),
+        ("add_existing_vol", {"selected_storage": "default", "vols": "data.qcow2", "cache": "default"}),
+        ("add_cdrom", {}),
+    )
+
+    def test_a_name_either_definition_uses_is_not_given_again(self):
+        vda = dev("vda", "virtio")
+        cases = (
+            # bus, live (disks, media), persistent (disks, media), expected name
+            ("scsi", ([vda], []), ([vda, dev("sda", "sata")], []), "sdb"),  # pending sata rename; scsi shares sd
+            ("sata", ([vda], []), ([vda], [dev("sda", "sata")]), "sdb"),  # pending CD-ROM
+            ("virtio", ([vda, dev("vdb", "virtio")], []), ([vda], []), "vdc"),  # not yet released by the guest
+            ("virtio", ([vda], []), ([vda], []), "vdb"),  # both the same, as on a stopped VM
+        )
+        for view, data in self.VIEWS:
+            for bus, live, config, expected in cases:
+                with self.subTest(view=view, bus=bus, expected=expected):
+                    self.assertEqual(self.attached_name(view, {**data, "bus": bus}, live, config), expected)

instances/views.py

@@ -968,8 +968,6 @@ def resize_disk(request, pk):
 @serialize_instance_mutation
 def add_new_vol(request, pk):
     instance = get_instance(request.user, pk, perm_type="change")
-    media = instance.proxy.get_media_devices()
-    disks = instance.proxy.get_disk_devices()
     storage = request.POST.get("storage", "")
     name = request.POST.get("name", "")
     format = request.POST.get("format", app_settings.INSTANCE_VOLUME_DEFAULT_FORMAT)
@@ -990,7 +988,11 @@ def add_new_vol(request, pk):
         instance.compute.password,
         instance.compute.type,
     )
-    target_dev = utils.get_new_disk_dev(media, disks, bus)
+    # 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,
@@ -1052,9 +1054,6 @@ def add_existing_vol(request, pk):
     if invalid:
         return reject_disk_options(request, pk, invalid)
 
-    media = instance.proxy.get_media_devices()
-    disks = instance.proxy.get_disk_devices()
-
     conn_create = wvmStorage(
         instance.compute.hostname,
         instance.compute.login,
@@ -1080,7 +1079,9 @@ def add_existing_vol(request, pk):
         # 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)
+    target_dev = utils.get_new_disk_dev(
+        instance.media + instance.config_media, instance.disks + instance.config_disks, bus
+    )
 
     instance.proxy.attach_disk(
         target_dev,
@@ -1252,7 +1253,9 @@ def add_cdrom(request, pk):
     if invalid:
         return reject_disk_options(request, pk, invalid)
 
-    target = utils.get_new_disk_dev(instance.media, instance.disks, bus)
+    target = utils.get_new_disk_dev(
+        instance.media + instance.config_media, instance.disks + instance.config_disks, bus
+    )
     instance.proxy.attach_disk(
         target,
         "",