fix(instances): a running VM's disk edit keeps the other pending disk changes

On a running VM a disk edit changes the definition for the next start,
but the disk list and its edit form showed the running VM's disks. The
form posts every field, so an edit wrote the running values back over
earlier pending changes (cache, io, serial, ...), and a disk only in the
next definition could not be edited. The list now shows the persistent
disks. A volume is deleted only when both definitions use it at that
device, since the list may now show another source than the running VM.

README.md

@@ -540,6 +540,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.
 - 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 and its edit form show the disks as they will be at the next start, so saving an edit no longer reverts another pending disk change.
 - 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/models.py

@@ -156,7 +156,9 @@ class Instance(models.Model):
 
     @cached_property
     def config_disks(self) -> list[dict]:
-        """Disks of the persistent definition, which a clone copies."""
+        """Disks of the persistent definition, which the disk list, its edit
+        form and a clone use: on a running VM a pending change shows, and an
+        edit does not post the live value back over it."""
         return self.proxy.get_disk_devices(config=True)
 
     @cached_property

instances/templates/instances/settings_tab.html

@@ -257,7 +257,7 @@
                         </tr>
                     </thead>
                     <tbody>
-                    {% for disk in instance.disks %}
+                    {% for disk in instance.config_disks %}
                         <tr>
                             <td>
                                 <button type="submit" class="btn btn-sm btn-secondary"

instances/test_live_dataloss.py

@@ -335,7 +335,7 @@ class LiveDataLossTestCase(TestCase):
 
         self.assertNotIn("<title>for-old</title>", dom_new.XMLDesc(0))
 
-    # NIC edits of a running VM are pending until restart; the form must show them
+    # Edits of a running VM are pending until restart; the forms must show them
 
     def test_pending_nic_change_is_shown_and_can_be_edited_again(self):
         nic = "<interface type='network'><mac address='52:54:00:aa:05:01'/><source network='default'/></interface>"
@@ -354,3 +354,25 @@ class LiveDataLossTestCase(TestCase):
         change("52:54:00:aa:05:02", "52:54:00:aa:05:03")
         self.assertIn("52:54:00:aa:05:03", dom.XMLDesc(libvirt.VIR_DOMAIN_XML_INACTIVE))
 
+    def test_pending_disk_change_is_shown_and_kept_by_the_next_edit(self):
+        from lxml import etree, html
+
+        dom, inst = self.vm("disk-pending", [livetest.create_volume(self.conn, P + "disk-pending")])
+        dom.create()
+        live = dom.XMLDesc(0)
+
+        def edit_form():
+            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='vda']]", a=action)
+            return {**dict(form.form_values()), "edit_volume": ""}  # what the browser posts
+
+        self.post("edit_volume", inst, {**edit_form(), "vol_cache": "none"})
+        form = edit_form()
+        self.assertEqual(form["vol_cache"], "none")
+        self.post("edit_volume", inst, {**form, "vol_serial": "wvc-serial"})
+
+        disk = etree.fromstring(dom.XMLDesc(libvirt.VIR_DOMAIN_XML_INACTIVE)).find("devices/disk")
+        self.assertEqual((disk.find("driver").get("cache"), disk.findtext("serial")), ("none", "wvc-serial"))
+        self.assertEqual(dom.XMLDesc(0), live)
+

instances/test_pending_edits.py

@@ -0,0 +1,70 @@
+"""On an active VM an edit changes the persistent definition and is pending
+until the next start. The edit forms show that definition, so a save does
+not post the running VM's value back over a pending change."""
+
+from unittest.mock import PropertyMock, 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
+from lxml import html
+
+
+def disk(dev, cache, serial):
+    return {
+        "dev": dev, "bus": "virtio", "image": f"{dev}.qcow2", "storage": "default", "path": f"/pool/{dev}.qcow2",
+        "format": "qcow2", "backing_file": None, "size": 1 << 30, "used": 1 << 20, "cache": cache, "io": "default",
+        "discard": "default", "detect_zeroes": "default", "readonly": False, "shareable": False, "serial": serial,
+    }
+
+
+class PendingEditsTestCase(TestCase):
+    def setUp(self):
+        self.client.force_login(get_user_model().objects.create_superuser("pe-admin", "pe@example.com", "x"))
+        compute = Compute.objects.create(name="pe", hostname="127.0.0.1:1", login="root", password="", type=1)
+        self.instance = Instance.objects.create(compute=compute, name="pe-vm", uuid="aaaaaaaa-1111-2222-3333-444444444444")
+
+    def page(self, live, config):  # disks of the live and the persistent definition
+        with patch("instances.models.wvmInstance") as wvm, patch.object(
+            Compute, "status", new_callable=PropertyMock, return_value=True
+        ):
+            proxy = wvm.return_value
+            proxy.get_status.return_value = 1
+            proxy.get_memory.return_value = proxy.get_cur_memory.return_value = 1024
+            proxy.get_max_memory.return_value = 4096
+            proxy.get_vcpu.return_value = proxy.get_cur_vcpu.return_value = 1
+            proxy.get_max_cpus.return_value = [1, 2]
+            proxy.get_vcpus.return_value = {}
+            for getter in ("get_networks", "get_ifaces", "get_storages", "get_media_devices", "get_net_devices",
+                           "get_snapshot", "get_external_snapshots"):
+                getattr(proxy, getter).return_value = []
+            proxy.get_disk_devices.side_effect = lambda **kw: config if kw.get("config") else live
+            proxy.get_console_type.return_value = "vnc"
+            proxy.get_cache_modes.return_value = {"default": "Default", "none": "Disabled"}
+            response = self.client.get(reverse("instances:instance", args=[self.instance.id]))
+        self.assertEqual(response.status_code, 200)
+        return html.fromstring(response.content)
+
+    def test_the_disk_edit_form_shows_the_pending_definition(self):
+        # running: cache=none and a new disk vdb pending, vda's serial live only
+        doc = self.page(
+            live=[disk("vda", "default", "live-serial")],
+            config=[disk("vda", "none", ""), disk("vdb", "default", "")],
+        )
+        forms = doc.xpath("//form[@action=$a]", a=reverse("instances:edit_volume", args=[self.instance.id]))
+        self.assertEqual([f.xpath("string(.//input[@name='dev']/@value)") for f in forms], ["vda", "vdb"])
+        vda = forms[0]
+        self.assertEqual(vda.xpath(".//select[@name='vol_cache']/option[@selected]/@value"), ["none"])
+        self.assertEqual(vda.xpath("string(.//input[@name='vol_serial']/@value)"), "")
+
+    def test_a_disk_with_a_pending_source_is_not_deleted(self):
+        # running: vda is A, its definition for the next start uses B
+        with patch("instances.models.wvmInstance") as wvm, patch("instances.views.wvmStorage") as storage:
+            proxy = wvm.return_value
+            live, pending = disk("vda", "default", ""), {**disk("vda", "default", ""), "path": "/pool/b.qcow2"}
+            proxy.get_disk_devices.side_effect = lambda **kw: [pending] if kw.get("config") else [live]
+            self.client.post(reverse("instances:delete_vol", args=[self.instance.id]), {"dev": "vda"})
+        proxy.detach_disk.assert_not_called()
+        storage.return_value.del_volume.assert_not_called()

instances/views.py

@@ -1189,6 +1189,13 @@ def delete_vol(request, pk):
             _("Disk %(dev)s is not a storage volume of this instance") % {"dev": dev},
         )
         return _back(request, pk, "disks")
+    # the page lists the persistent disks: delete only a volume both definitions use there
+    if not any(d["dev"] == dev and d["path"] == disk["path"] for d in instance.config_disks):
+        messages.error(
+            request,
+            _("Disk %(dev)s has a pending change; delete it once the VM is shut down") % {"dev": dev},
+        )
+        return _back(request, pk, "disks")
 
     conn_delete = wvmStorage(
         instance.compute.hostname,