From ff4ffab3821cc8b7a72231e6d39b7caca3130571 Mon Sep 17 00:00:00 2001 From: catborise Date: Mon, 12 Oct 2026 22:13:49 +0300 Subject: [PATCH] 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 | 8 ++- instances/tests/test_live_snapshots.py | 17 ++++++ instances/tests/test_snapshot_views.py | 21 +++++++- instances/tests/test_views.py | 6 ++- vrtManager/instance.py | 47 ++++++++++++----- vrtManager/tests/test_external_snapshots.py | 58 ++++++++++++++++++++- 6 files changed, 136 insertions(+), 21 deletions(-) diff --git a/instances/models.py b/instances/models.py index 0bf4f468..39b3f024 100644 --- a/instances/models.py +++ b/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): diff --git a/instances/tests/test_live_snapshots.py b/instances/tests/test_live_snapshots.py index a939b950..d1b17c28 100644 --- a/instances/tests/test_live_snapshots.py +++ b/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"" + "", + 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("i", 0) diff --git a/instances/tests/test_snapshot_views.py b/instances/tests/test_snapshot_views.py index bb64ec1f..55b2fac7 100644 --- a/instances/tests/test_snapshot_views.py +++ b/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")) diff --git a/instances/tests/test_views.py b/instances/tests/test_views.py index d94a60d5..662eabb1 100644 --- a/instances/tests/test_views.py +++ b/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): diff --git a/vrtManager/instance.py b/vrtManager/instance.py index ade58911..7cc4f44c 100644 --- a/vrtManager/instance.py +++ b/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 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 diff --git a/vrtManager/tests/test_external_snapshots.py b/vrtManager/tests/test_external_snapshots.py index 91732a9a..38b58bfe 100644 --- a/vrtManager/tests/test_external_snapshots.py +++ b/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: is what ran, what was defined + snap = fake_snap("one") + snap.getXMLDesc.return_value = snapshot_xml("one", disks=("vda", "vdc")).replace( + "", + "" + "", + ) + persistent = DOMAIN.replace("", "") + 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"{backing[path]}" if backing[path] else "" return MagicMock(**{"XMLDesc.return_value": f"{path}{parent}"}) @@ -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")