fix(instances): snapshot tab reads less and fails less

The snapshot tab listed the VM's snapshots three times per page: once
per kind and once more for all of them. The list of all is now made
from the two kinds, so the combined flag is no longer used anywhere.

The revert confirm named only the disks the persistent definition
would lose. A disk hot-plugged into the running VM alone is detached
too, so the live definition is compared with what ran when the
snapshot was taken.

A storage pool refresh error after libvirt had already made, reverted
or deleted the snapshot was shown as if the operation had failed. It
is now logged, and the left-behind check is skipped then, since a stale
pool still lists the removed files. A volume removed while that check
reads it is skipped too.

An error reading the block jobs no longer breaks the instance page;
create, revert and delete still query the jobs again and refuse.

instances/models.py

@@ -263,7 +263,7 @@ class Instance(models.Model):
 
     @cached_property
     def snapshots(self) -> list[dict]:
-        return self._snapshots(VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL | VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL)
+        return sorted(self.internal_snapshots + self.external_snapshots, reverse=True, key=lambda k: k["date"])
 
     @cached_property
     def internal_snapshots(self) -> list[dict]:
@@ -275,7 +275,11 @@ class Instance(models.Model):
 
     @cached_property
     def snapshot_jobs(self) -> list[dict]:
-        return self.proxy.get_snapshot_jobs()
+        # only for the page; create, revert and delete query the jobs again and refuse if that fails
+        try:
+            return self.proxy.get_snapshot_jobs()
+        except libvirtError:
+            return []
 
     @cached_property
     def inst_xml(self):

instances/tests/test_live_snapshots.py

@@ -110,6 +110,23 @@ class LiveExternalSnapshotTestCase(SimpleTestCase):
             vm.revert_external_snapshot("a")
             self.assertEqual(dom.state()[0], libvirt.VIR_DOMAIN_RUNNING)
 
+    def test_a_disk_hot_plugged_live_only_is_named_and_detached_by_a_revert(self):
+        dom, vm = self.vm("snap-hotplug", "running")
+        vm.create_external_snapshot("a")
+        extra = livetest.create_volume(self.conn, P + "snap-hotplug-extra")
+        dom.attachDeviceFlags(
+            f"<disk type='file' device='disk'><driver name='qemu' type='qcow2'/><source file='{extra}'/>"
+            "<target dev='vdb' bus='virtio'/></disk>",
+            libvirt.VIR_DOMAIN_AFFECT_LIVE,
+        )
+        [snap] = vm.get_snapshot(libvirt.VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL)
+        self.assertEqual(snap["detaches"], [{"dev": "vdb", "path": extra}])
+        self.assertEqual(vm.revert_external_snapshot("a"), [], "the detached disk is not a leftover")
+        self.assertNotIn("vdb", etree.fromstring(dom.XMLDesc(0)).xpath("devices/disk/target/@dev"))
+        self.assertEqual(dom.state()[0], libvirt.VIR_DOMAIN_RUNNING)
+        self.assertTrue(livetest.volume_exists(self.conn, extra))
+        vm.delete_external_snapshot("a")
+
     def test_a_mixed_vm_can_be_cleaned_up(self):
         dom, vm = self.vm("snap-mixed-clean")
         dom.snapshotCreateXML("<domainsnapshot><name>i</name></domainsnapshot>", 0)

instances/tests/test_snapshot_views.py

@@ -94,8 +94,11 @@ class SnapshotTabTestCase(TestCase):
             proxy.get_snapshot.side_effect = lambda flag: {
                 libvirt.VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL: list(internal),
                 libvirt.VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL: list(external),
-            }.get(flag, [*internal, *external])
-            proxy.get_snapshot_jobs.return_value = list(jobs)
+            }[flag]
+            if isinstance(jobs, Exception):
+                proxy.get_snapshot_jobs.side_effect = jobs
+            else:
+                proxy.get_snapshot_jobs.return_value = list(jobs)
             proxy.get_status.return_value = status
             proxy.get_nvram.return_value = nvram
             proxy.get_memory.return_value = proxy.get_cur_memory.return_value = 1024
@@ -106,6 +109,7 @@ class SnapshotTabTestCase(TestCase):
             proxy.get_console_type.return_value = "vnc"
             response = self.client.get(reverse("instances:instance", args=[self.instance.id]))
         self.assertEqual(response.status_code, 200)
+        self.proxy = proxy
         return lxml_html.fromstring(response.content)
 
     def snap(self, name, **kw):
@@ -120,6 +124,19 @@ class SnapshotTabTestCase(TestCase):
         self.assertIn("vdb (/pool/later.qcow2)", text)
         self.assertIn("NVRAM", text)
 
+    def test_each_kind_is_listed_once_per_page(self):
+        doc = self.page(internal=[self.snap("i")], external=[self.snap("e")])
+        flags = sorted(c.args[0] for c in self.proxy.get_snapshot.call_args_list)
+        self.assertEqual(
+            flags, sorted([libvirt.VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL, libvirt.VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL])
+        )
+        self.assertEqual(doc.xpath("//i[contains(@title, 'snapshot(s)')]/@title"), ["There are 2 snapshot(s)"])
+
+    def test_a_failed_job_query_still_renders_the_tab(self):
+        # the actions themselves check for jobs again and refuse if that fails
+        doc = self.page(external=[self.snap("e")], jobs=libvirt.libvirtError("no job info"))
+        self.assertEqual(len(doc.xpath("//button[@name='revert_external_snapshot']")), 1)
+
     def test_memory_checkbox_only_for_a_running_vm(self):
         self.assertFalse(self.page(status=1).xpath("//input[@name='memory']")[0].get("disabled"))
         self.assertIsNotNone(self.page(status=5).xpath("//input[@name='memory']")[0].get("disabled"))

instances/tests/test_views.py

@@ -520,7 +520,8 @@ class InstancesTestCase(TestCase):
         )
         self.assertEqual(response.status_code, 302)
 
-        del self.instance.snapshots
+        for cached in ("snapshots", "internal_snapshots", "external_snapshots"):
+            self.instance.__dict__.pop(cached, None)
         self.assertEqual(len(self.instance.snapshots), 1)
 
         response = self.client.post(
@@ -541,7 +542,8 @@ class InstancesTestCase(TestCase):
         )
         self.assertEqual(response.status_code, 302)
 
-        del self.instance.snapshots
+        for cached in ("snapshots", "internal_snapshots", "external_snapshots"):
+            self.instance.__dict__.pop(cached, None)
         self.assertEqual(len(self.instance.snapshots), 0)
 
     def test_autostart(self):

vrtManager/instance.py

@@ -1574,7 +1574,7 @@ class wvmInstance(wvmConnect):
         if not memory:
             flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY
         self._snapshotCreateXML(xml, flags)
-        self.refresh_instance_pools()
+        self._refresh_pools_after_snapshot()
 
     def _snapshot_memory_xml(self, name, tree):
         """The <memory> element of a snapshot that saves memory: a file in the
@@ -1664,14 +1664,28 @@ class wvmInstance(wvmConnect):
                 points.update(self._disk_sources(saved).values())
         return disks, own, points | own
 
+    def _refresh_pools_after_snapshot(self):
+        """Refresh the pools once libvirt has changed the snapshot files. The
+        snapshot operation is done by then, so an error is only logged;
+        returns whether the pools are current."""
+        try:
+            self.refresh_instance_pools()
+        except libvirtError as err:
+            logger.warning("Storage pools of %s were not refreshed after a snapshot change: %s", self.instance.name(), err)
+            return False
+        return True
+
     def _left_behind(self, before):
         """Files libvirt merged or discarded but could not remove: it only
         logs a warning then (on NFS it removes them as the qemu user, who
         needs write access to their directory). Only a snapshot's own file or
         an overlay on a snapshot point counts; a disk a revert detached is
-        kept on purpose. A file outside every storage pool is not checked."""
+        kept on purpose. A file outside every storage pool is not checked,
+        nor is anything when the pools could not be refreshed: a stale pool
+        still lists the removed files."""
         disks, own, points = before
-        self.refresh_instance_pools()
+        if not self._refresh_pools_after_snapshot():
+            return []
         now_disks, now_own, _ = self._snapshot_files()
         # only disks still attached: one a revert detached keeps its files,
         # whatever they sit on
@@ -1680,10 +1694,11 @@ class wvmInstance(wvmConnect):
         for path in sorted((attached | own) - set().union(now_own, *now_disks.values())):
             try:
                 vol = self.get_volume_by_path(path)
-            except libvirtError:
+                if path not in own and util.get_xml_path(vol.XMLDesc(0), "/volume/backingStore/path") not in points:
+                    continue
+            except libvirtError:  # not in a pool, or removed meanwhile
                 continue
-            if path in own or util.get_xml_path(vol.XMLDesc(0), "/volume/backingStore/path") in points:
-                left.append(path)
+            left.append(path)
         return left
 
     @staticmethod
@@ -1703,15 +1718,23 @@ class wvmInstance(wvmConnect):
 
     def get_snapshot(self, flag):
         """Snapshots of one kind (VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL or
-        _EXTERNAL; both flags for all), with whether each saved memory, is the
-        current one, and which disks of the persistent definition a revert to
-        it detaches."""
-        current_disks = self._disk_sources(self._XMLDesc(PERSISTENT_XML))
+        _EXTERNAL), with whether each saved memory, is the current one, and
+        which disks a revert to it detaches: from the persistent definition,
+        and on a running VM from the live one too (a disk hot-plugged only)."""
+        persistent_disks = self._disk_sources(self._XMLDesc(PERSISTENT_XML))
+        live_disks = self._disk_sources(self._XMLDesc(0)) if self.instance.isActive() else {}
         snapshots = []
         for snap in self.instance.listAllSnapshots(flag):
             tree = etree.fromstring(snap.getXMLDesc(0))
             definition = self._saved_definition(tree)
             saved_disks = self._disk_sources(definition) if definition is not None else {}
+            # a revert of a running VM restores what ran when the snapshot was taken
+            ran = tree.find("domain")
+            saved_live_disks = self._disk_sources(ran) if ran is not None else saved_disks
+            detaches = {dev: path for dev, path in persistent_disks.items() if dev not in saved_disks}
+            for dev, path in live_disks.items():
+                if dev not in saved_live_disks:
+                    detaches.setdefault(dev, path)
             snapshots.append(
                 {
                     "date": datetime.fromtimestamp(int(tree.findtext("creationTime"))),
@@ -1719,9 +1742,7 @@ class wvmInstance(wvmConnect):
                     "description": tree.findtext("description") or "",
                     "memory": tree.find("memory[@snapshot='external']") is not None,
                     "current": bool(snap.isCurrent(0)),
-                    "detaches": [
-                        {"dev": dev, "path": path} for dev, path in current_disks.items() if dev not in saved_disks
-                    ],
+                    "detaches": [{"dev": dev, "path": path} for dev, path in detaches.items()],
                 }
             )
         return snapshots

vrtManager/tests/test_external_snapshots.py

@@ -98,13 +98,38 @@ class SnapshotListTestCase(unittest.TestCase):
         [entry] = p.get_snapshot(EXTERNAL)
         self.assertEqual(entry["detaches"], [{"dev": "vdb", "path": "/pool/later.qcow2"}])
 
-    def test_detaches_comes_from_the_persistent_definition(self):
+    def test_detaches_covers_the_persistent_and_the_live_definition(self):
         # vdc was hot-plugged into the running VM only; vdb waits for the next start
         live = DOMAIN.replace("/pool/later.qcow2", "/pool/live-only.qcow2").replace("dev='vdb'", "dev='vdc'")
         p = proxy(external=[fake_snap("one")])
         p._XMLDesc = lambda flags: DOMAIN if flags == PERSISTENT_XML else live
         [entry] = p.get_snapshot(EXTERNAL)
-        self.assertEqual(entry["detaches"], [{"dev": "vdb", "path": "/pool/later.qcow2"}])
+        self.assertEqual(
+            entry["detaches"],
+            [{"dev": "vdb", "path": "/pool/later.qcow2"}, {"dev": "vdc", "path": "/pool/live-only.qcow2"}],
+        )
+
+    def test_the_live_definition_is_compared_with_the_snapshots_live_one(self):
+        # taken while vdc was hot-plugged: <domain> is what ran, <inactiveDomain> what was defined
+        snap = fake_snap("one")
+        snap.getXMLDesc.return_value = snapshot_xml("one", disks=("vda", "vdc")).replace(
+            "</domainsnapshot>",
+            "<inactiveDomain><devices><disk type='file' device='disk'><source file='/pool/vda.qcow2'/>"
+            "<target dev='vda'/></disk></devices></inactiveDomain></domainsnapshot>",
+        )
+        persistent = DOMAIN.replace("<disk type='file' device='disk'><source file='/pool/later.qcow2'/><target dev='vdb' bus='virtio'/></disk>", "")
+        live = DOMAIN.replace("/pool/later.qcow2", "/pool/live-only.qcow2").replace("dev='vdb'", "dev='vdc'")
+        p = proxy(external=[snap])
+        p._XMLDesc = lambda flags: persistent if flags == PERSISTENT_XML else live
+        [entry] = p.get_snapshot(EXTERNAL)
+        self.assertEqual(entry["detaches"], [])
+
+    def test_a_shut_off_vm_is_compared_with_the_persistent_definition_only(self):
+        p = proxy(status=5, external=[fake_snap("one", disks=("vda", "vdb"))])
+        p._XMLDesc = MagicMock(return_value=DOMAIN)
+        [entry] = p.get_snapshot(EXTERNAL)
+        self.assertEqual(entry["detaches"], [])
+        p._XMLDesc.assert_called_once_with(PERSISTENT_XML)
 
     def test_lists_are_by_kind(self):
         p = proxy(internal=[fake_snap("i")], external=[fake_snap("e")])
@@ -429,6 +454,8 @@ class LeftBehindTestCase(unittest.TestCase):
         def volume(path):
             if path not in backing:
                 raise libvirtError("no volume")
+            if isinstance(backing[path], Exception):  # removed between the lookup and the read
+                return MagicMock(**{"XMLDesc.side_effect": backing[path]})
             parent = f"<backingStore><path>{backing[path]}</path></backingStore>" if backing[path] else ""
             return MagicMock(**{"XMLDesc.return_value": f"<volume><target><path>{path}</path></target>{parent}</volume>"})
 
@@ -470,6 +497,33 @@ class LeftBehindTestCase(unittest.TestCase):
         left = self.revert(before, CHAIN, {"/pool/linked.qcow2": "/pool/a.one"})
         self.assertEqual(left, [])
 
+    def test_a_volume_gone_while_checking_is_not_reported(self):
+        before = CHAIN.replace("/pool/a.two", "/pool/a.after")
+        after = CHAIN.replace("/pool/a.two", "/pool/a.1700000001")
+        left = self.revert(before, after, {"/pool/a.after": libvirtError("no volume")})
+        self.assertEqual(left, [])
+
+
+class PoolRefreshTestCase(unittest.TestCase):
+    """The pools are refreshed after libvirt made, reverted or deleted the
+    snapshot; a refresh error does not turn that into a failure."""
+
+    def test_create_succeeds_when_the_refresh_fails(self):
+        p = proxy()
+        p.refresh_instance_pools.side_effect = libvirtError("pool busy")
+        p.create_external_snapshot("one")
+        p._snapshotCreateXML.assert_called_once()
+
+    def test_delete_and_revert_succeed_and_report_nothing_when_the_refresh_fails(self):
+        for action in ("delete_external_snapshot", "revert_external_snapshot"):
+            with self.subTest(action):
+                snap = fake_snap("one", current=True)
+                p = proxy(external=[snap])
+                p.refresh_instance_pools.side_effect = libvirtError("pool busy")
+                # a stale pool still lists every file
+                p.get_volume_by_path = MagicMock()
+                self.assertEqual(getattr(p, action)("one"), [])
+
 class KindTestCase(unittest.TestCase):
     def test_internal_endpoints_refuse_external_names(self):
         snap = fake_snap("e")