From 3fd422db7e3695f03def18a2d05887b1d52f78f7 Mon Sep 17 00:00:00 2001 From: catborise Date: Mon, 12 Oct 2026 16:32:51 +0300 Subject: [PATCH] fix(instances): name snapshot files libvirt could not remove When libvirt cannot remove a snapshot file it merged or discarded, it only logs a warning and the delete or revert still succeeds. On NFS it removes them as the qemu user, so an image directory the qemu user may not write to left every merged overlay on disk without a word. Revert and delete now compare the files in use before and after and show a warning naming the ones still on disk. Only a snapshot's own files (overlays, memory) and overlays on a snapshot point count, and a disk the revert detached is never named, so a kept disk is not offered for removal. --- README.md | 2 +- instances/tests/test_live_snapshots.py | 15 ++- instances/tests/test_snapshot_views.py | 14 ++- instances/views.py | 20 +++- vrtManager/instance.py | 49 ++++++++- vrtManager/tests/test_external_snapshots.py | 105 ++++++++++++++++++++ 6 files changed, 194 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 1098b3aa..740b8f63 100644 --- a/README.md +++ b/README.md @@ -556,7 +556,7 @@ A rollback leaves the clone on a detached HEAD at the restored commit; the updat - gstfsd finds the guest's operating system the way libguestfs inspects it, so a root on LVM works, and it always answers (an error when it finds no or several operating systems). Update `/usr/local/bin/gstfsd` on every compute (see [Adding a Compute Node](#adding-a-compute-node), step 3). - A theme picked on the Settings page rewrites two tracked files; the update and rollback steps put back the shipped theme. Pick yours again afterwards, also after Docker recreates the container. - With the bundled nginx config, browsers revalidate static files on each load (304 when unchanged), so an update or a theme change shows without a hard reload. Files cached before this update can stay until a hard reload (Ctrl+F5) of each affected page, or until the site's browser cache is cleared once; an external caching proxy may need a purge. -- External snapshots use libvirt's snapshot API and need libvirt 9.9 or newer on the compute hosts. They work on UEFI VMs and running, paused or shut-off VMs, can save memory too, and a revert keeps disks added later (it detaches them). A VM's snapshots form one chain: after a revert, delete the newer snapshots before taking another. Snapshot names no longer get an `s1.` prefix; existing ones keep theirs and can still be reverted and deleted, except that a snapshot an older version took of a UEFI VM can only be deleted. +- External snapshots use libvirt's snapshot API and need libvirt 9.9 or newer on the compute hosts. They work on UEFI VMs and running, paused or shut-off VMs, can save memory too, and a revert keeps disks added later (it detaches them). A VM's snapshots form one chain: after a revert, delete the newer snapshots before taking another. Snapshot names no longer get an `s1.` prefix; existing ones keep theirs and can still be reverted and deleted, except that a snapshot an older version took of a UEFI VM can only be deleted. On NFS, libvirt removes merged snapshot files as the qemu user, who needs write access to the image directory; files it could not remove are named in a warning after the delete or revert. - Docker no longer publishes port 6080; the console goes through nginx on the panel's port (`WS_PUBLIC_PORT`, default 80). ## Usage diff --git a/instances/tests/test_live_snapshots.py b/instances/tests/test_live_snapshots.py index 2c73fb0e..a939b950 100644 --- a/instances/tests/test_live_snapshots.py +++ b/instances/tests/test_live_snapshots.py @@ -54,6 +54,12 @@ class LiveExternalSnapshotTestCase(SimpleTestCase): dom.suspend() return dom, wvmInstance(*self.args, None, uuid=dom.UUIDString()) + def pool_files(self, name): + """The test pool's files whose names start with the VM's name.""" + pool = self.conn.storagePoolLookupByName(livetest.POOL) + pool.refresh(0) + return {vol for vol in pool.listVolumes() if vol.startswith(P + name + ".")} + def top(self, dom): return etree.fromstring(dom.XMLDesc(0)).xpath("string(devices/disk[@device='disk']/source/@file)") @@ -66,10 +72,11 @@ class LiveExternalSnapshotTestCase(SimpleTestCase): base = self.top(dom) vm.create_external_snapshot("one", "first") vm.create_external_snapshot("two") - vm.revert_external_snapshot("one") + self.assertEqual(vm.revert_external_snapshot("one"), [], "files left behind") self.assertEqual(dom.state()[0], expected) - vm.delete_external_snapshot("two") - vm.delete_external_snapshot("one") + self.assertEqual(vm.delete_external_snapshot("two"), [], "files left behind") + self.assertEqual(vm.delete_external_snapshot("one"), [], "files left behind") + self.assertEqual(self.pool_files("snap-" + state), {os.path.basename(base)}, "an overlay stayed on disk") self.assertEqual(dom.snapshotNum(0), 0) self.assertEqual(dom.state()[0], expected) self.assertTrue(livetest.volume_exists(self.conn, base)) @@ -82,7 +89,7 @@ class LiveExternalSnapshotTestCase(SimpleTestCase): dom.suspend() vm.revert_external_snapshot("m") self.assertEqual(dom.state()[0], libvirt.VIR_DOMAIN_RUNNING) - vm.delete_external_snapshot("m") + self.assertEqual(vm.delete_external_snapshot("m"), []) self.assertFalse(livetest.volume_exists(self.conn, mem)) def test_uefi_vm_keeps_its_loader(self): diff --git a/instances/tests/test_snapshot_views.py b/instances/tests/test_snapshot_views.py index bbd09584..bb64ec1f 100644 --- a/instances/tests/test_snapshot_views.py +++ b/instances/tests/test_snapshot_views.py @@ -25,7 +25,7 @@ class SnapshotViewsTestCase(TestCase): admin = get_user_model().objects.create_superuser("snap_admin", "s@example.com", "pw") self.client.force_login(admin) - def post(self, view, data, error=None): + def post(self, view, data, error=None, left=()): with patch("instances.models.wvmInstance") as mock_wvm: proxy = mock_wvm.return_value method = { @@ -36,8 +36,10 @@ class SnapshotViewsTestCase(TestCase): "delete_snapshot": proxy.snapshot_delete, "snapshot": proxy.create_snapshot, }[view] + method.return_value = list(left) # files revert and delete left behind if error: method.side_effect = util.OperationError(error) + self.client.cookies.pop("messages", None) # only this request's messages response = self.client.post( reverse(f"instances:{view}", args=[self.instance.id]), data, @@ -60,6 +62,16 @@ class SnapshotViewsTestCase(TestCase): for power in ("start", "force_shutdown", "shutdown"): getattr(proxy, power).assert_not_called() + def test_files_left_behind_are_named_in_a_warning(self): + for view in ("revert_external_snapshot", "delete_external_snapshot"): + with self.subTest(view): + _, _, messages = self.post(view, {"name": "a"}, left=["/pool/a.one", "/pool/vm.one.mem"]) + warning = [m for m in messages if "/pool/a.one, /pool/vm.one.mem" in m] + self.assertEqual(len(warning), 1, messages) + self.assertIn("qemu user", warning[0]) + _, _, messages = self.post(view, {"name": "a"}) + self.assertFalse([m for m in messages if "not removed" in m], messages) + def test_refusals_become_messages(self): for view in ("create_external_snapshot", "revert_external_snapshot", "delete_external_snapshot", "revert_snapshot", "delete_snapshot", "snapshot"): diff --git a/instances/views.py b/instances/views.py index a353cfec..e9564dff 100755 --- a/instances/views.py +++ b/instances/views.py @@ -1371,6 +1371,20 @@ def revert_snapshot(request, pk): return _back(request, pk, "managesnapshot") +def _warn_left_behind(request, files): + """libvirt only logs it when it cannot remove a snapshot file.""" + if files: + messages.warning( + request, + _( + "These snapshot files were not removed, although nothing uses them any more: %(files)s. " + "On NFS libvirt removes them as the qemu user, who needs write access to their directory. " + "Remove them by hand." + ) + % {"files": ", ".join(files)}, + ) + + @require_POST @serialize_instance_mutation def create_external_snapshot(request, pk): @@ -1398,11 +1412,12 @@ def revert_external_snapshot(request, pk): raise PermissionDenied name = request.POST.get("name", "") try: - instance.proxy.revert_external_snapshot(name) + left = instance.proxy.revert_external_snapshot(name) except libvirtError as err: messages.error(request, _("Snapshot was not reverted: %(err)s") % {"err": error_text(request, err)}) else: messages.success(request, _("Successful revert snapshot: ") + name) + _warn_left_behind(request, left) msg = _("Revert external snapshot: %(snap)s") % {"snap": name} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) return _back(request, pk, "managesnapshot") @@ -1416,10 +1431,11 @@ def delete_external_snapshot(request, pk): raise PermissionDenied name = request.POST.get("name", "") try: - instance.proxy.delete_external_snapshot(name) + left = instance.proxy.delete_external_snapshot(name) except libvirtError as err: messages.error(request, _("Snapshot was not deleted: %(err)s") % {"err": error_text(request, err)}) else: + _warn_left_behind(request, left) msg = _("Delete external snapshot: %(snap)s") % {"snap": name} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) return _back(request, pk, "managesnapshot") diff --git a/vrtManager/instance.py b/vrtManager/instance.py index 8e479be8..ade58911 100644 --- a/vrtManager/instance.py +++ b/vrtManager/instance.py @@ -1598,7 +1598,8 @@ class wvmInstance(wvmConnect): def revert_external_snapshot(self, name): """libvirt returns the disks and the definition to the snapshot's. A disk-only snapshot keeps the VM running, paused or shut off as it is; - a snapshot with memory resumes the VM as it was saved.""" + a snapshot with memory resumes the VM as it was saved. Returns the + files libvirt left behind (see _left_behind).""" snap = self._snapshot(name, external=True) self._check_snapshot_state() tree = etree.fromstring(snap.getXMLDesc(0)) @@ -1618,6 +1619,7 @@ class wvmInstance(wvmConnect): flags = {1: VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING, 3: VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED}.get( self.get_status(), 0 ) + before = self._snapshot_files() # libvirt names the new overlays after the second, so a revert within # the second of the previous one finds them taken, after it has # already stopped a running VM; one retry a second later finishes it. @@ -1634,14 +1636,55 @@ class wvmInstance(wvmConnect): raise util.OperationError( f"The revert failed twice; try again in a moment ({retry_err}). The VM is {state}." ) from None - self.refresh_instance_pools() + return self._left_behind(before) def delete_external_snapshot(self, name): - """libvirt merges the snapshot's overlays and removes its files.""" + """libvirt merges the snapshot's overlays and removes its files. + Returns the files it left behind (see _left_behind).""" snap = self._snapshot(name, external=True) self._check_snapshot_state() + before = self._snapshot_files() snap.delete(0) + return self._left_behind(before) + + def _snapshot_files(self): + """(disks, snapshot files, snapshot points): the files each disk uses + by target, the external snapshots' own files (overlays, memory), and + the files snapshots were taken on, which libvirt's overlays sit on.""" + disks = { + disk.xpath("string(target/@dev)"): set(disk.xpath(".//source/@file")) + for disk in etree.fromstring(self.instance.XMLDesc(0)).xpath("devices/disk[@device='disk']") + } + own, points = set(), set() + for snap in self.instance.listAllSnapshots(VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL): + tree = etree.fromstring(snap.getXMLDesc(0)) + own.update(tree.xpath("disks/disk/source/@file|memory/@file")) + saved = self._saved_definition(tree) + if saved is not None: + points.update(self._disk_sources(saved).values()) + return disks, own, points | own + + 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.""" + disks, own, points = before self.refresh_instance_pools() + now_disks, now_own, _ = self._snapshot_files() + # only disks still attached: one a revert detached keeps its files, + # whatever they sit on + attached = set().union(*(files for dev, files in disks.items() if dev in now_disks)) + left = [] + for path in sorted((attached | own) - set().union(now_own, *now_disks.values())): + try: + vol = self.get_volume_by_path(path) + except libvirtError: + continue + if path in own or util.get_xml_path(vol.XMLDesc(0), "/volume/backingStore/path") in points: + left.append(path) + return left @staticmethod def _disk_sources(xml): diff --git a/vrtManager/tests/test_external_snapshots.py b/vrtManager/tests/test_external_snapshots.py index f8b65010..91732a9a 100644 --- a/vrtManager/tests/test_external_snapshots.py +++ b/vrtManager/tests/test_external_snapshots.py @@ -78,6 +78,7 @@ def proxy(status=1, internal=(), external=(), xml=DOMAIN, jobs=None, managed_sav dom.hasManagedSaveImage.return_value = managed_save dom.isActive.return_value = status in (1, 3) dom.blockJobInfo.side_effect = lambda dev, flags: (jobs or {}).get(dev, {}) + dom.XMLDesc.side_effect = lambda flags: xml p.instance = dom return p @@ -365,6 +366,110 @@ class DeleteTestCase(unittest.TestCase): snap.delete.assert_not_called() +CHAIN = """ + + + + + +""" + + +def chain_snap(name, overlay, memory=None): + snap = fake_snap(name) + mem = f"" if memory else "" + snap.getXMLDesc.return_value = ( + f"{name}1700000000{mem}" + f"" + "" + ) + return snap + + +class LeftBehindTestCase(unittest.TestCase): + """libvirt only logs a warning when it cannot remove a merged or + discarded file (on NFS it removes them as the qemu user), so the delete + and revert report the files nothing uses any more that still exist.""" + + def run_delete(self, still_there): + one = chain_snap("one", "/pool/a.one", memory="/pool/vm.one.mem") + two = chain_snap("two", "/pool/a.two") + p = proxy(external=[one, two]) + after = CHAIN.replace("", "").replace( + "", "" + ) + p.instance.XMLDesc.side_effect = [CHAIN, after] + p.instance.listAllSnapshots.side_effect = [[one, two], [two]] + + def volume(path): + if path not in still_there: + raise libvirtError("no volume") + return MagicMock() + + p.get_volume_by_path = volume + return p.delete_external_snapshot("one") + + def test_files_libvirt_could_not_remove_are_reported(self): + self.assertEqual( + self.run_delete({"/pool/a.one", "/pool/vm.one.mem", "/pool/a.two", "/pool/a.qcow2"}), + ["/pool/a.one", "/pool/vm.one.mem"], + ) + + def test_nothing_is_reported_when_libvirt_removed_them(self): + # files still in use (base, the newer overlay) are never reported + self.assertEqual(self.run_delete({"/pool/a.two", "/pool/a.qcow2"}), []) + + def revert(self, before, after, backing): + """Revert to one; backing maps each file still on disk to its backing file.""" + one = chain_snap("one", "/pool/a.one") + p = proxy(external=[one]) + p.instance.XMLDesc.side_effect = [before, after] + p.instance.listAllSnapshots.side_effect = [[one], [one]] + + def volume(path): + if path not in backing: + raise libvirtError("no volume") + parent = f"{backing[path]}" if backing[path] else "" + return MagicMock(**{"XMLDesc.return_value": f"{path}{parent}"}) + + p.get_volume_by_path = volume + return p.revert_external_snapshot("one") + + def test_revert_reports_a_discarded_overlay_left_on_disk(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": "/pool/a.one", "/pool/a.1700000001": "/pool/a.one"}) + self.assertEqual(left, ["/pool/a.after"]) + + def test_a_shut_off_vm_overlay_is_found_through_the_storage(self): + # a shut-off VM's definition does not show the backing chain + def plain(top): + return f"" + + left = self.revert(plain("/pool/a.after"), plain("/pool/a.1700000001"), {"/pool/a.after": "/pool/a.one"}) + self.assertEqual(left, ["/pool/a.after"]) + + def test_revert_never_reports_a_detached_disk(self): + # vdb and vdc were added after the snapshot: the revert detaches them + # and keeps their files on purpose, also when vdc is itself an overlay + later = ( + "" + "" + "" + ) + before = CHAIN.replace("", later + "") + left = self.revert(before, CHAIN, {"/pool/later.qcow2": None, "/pool/clone.qcow2": "/templates/base.qcow2"}) + self.assertEqual(left, []) + + def test_revert_never_reports_a_detached_overlay_of_a_snapshot_file(self): + linked = ( + "" + "" + ) + before = CHAIN.replace("", linked + "") + left = self.revert(before, CHAIN, {"/pool/linked.qcow2": "/pool/a.one"}) + self.assertEqual(left, []) + class KindTestCase(unittest.TestCase): def test_internal_endpoints_refuse_external_names(self): snap = fake_snap("e")