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