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

@@ -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

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

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

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

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

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 = """<domain><devices>
+  <disk type='file' device='disk'><source file='/pool/a.two'/><target dev='vda'/>
+    <backingStore type='file'><source file='/pool/a.one'/>
+      <backingStore type='file'><source file='/pool/a.qcow2'/><backingStore/></backingStore>
+    </backingStore>
+  </disk>
+</devices></domain>"""
+
+
+def chain_snap(name, overlay, memory=None):
+    snap = fake_snap(name)
+    mem = f"<memory snapshot='external' file='{memory}'/>" if memory else "<memory snapshot='no'/>"
+    snap.getXMLDesc.return_value = (
+        f"<domainsnapshot><name>{name}</name><creationTime>1700000000</creationTime>{mem}"
+        f"<disks><disk name='vda' snapshot='external'><source file='{overlay}'/></disk></disks>"
+        "<domain><devices/></domain></domainsnapshot>"
+    )
+    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("<source file='/pool/a.one'/>", "<source file='/pool/a.qcow2'/>").replace(
+            "<backingStore type='file'><source file='/pool/a.qcow2'/><backingStore/></backingStore>", "<backingStore/>"
+        )
+        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"<backingStore><path>{backing[path]}</path></backingStore>" if backing[path] else ""
+            return MagicMock(**{"XMLDesc.return_value": f"<volume><target><path>{path}</path></target>{parent}</volume>"})
+
+        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"<domain><devices><disk type='file' device='disk'><source file='{top}'/><target dev='vda'/></disk></devices></domain>"
+
+        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 = (
+            "<disk type='file' device='disk'><source file='/pool/later.qcow2'/><target dev='vdb'/></disk>"
+            "<disk type='file' device='disk'><source file='/pool/clone.qcow2'/><target dev='vdc'/>"
+            "<backingStore type='file'><source file='/templates/base.qcow2'/><backingStore/></backingStore></disk>"
+        )
+        before = CHAIN.replace("</devices>", later + "</devices>")
+        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 = (
+            "<disk type='file' device='disk'><source file='/pool/linked.qcow2'/><target dev='vdb'/>"
+            "<backingStore type='file'><source file='/pool/a.one'/><backingStore/></backingStore></disk>"
+        )
+        before = CHAIN.replace("</devices>", linked + "</devices>")
+        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")