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")