feat(instances): external snapshots use libvirt's snapshot API

Create, revert and delete are one libvirt call each. The pflash-to-rom
loader rewrite, the hand-written block commit and the s1./s2. names are
gone, so UEFI VMs can take external snapshots, a revert keeps disks added
later, and deleting an older snapshot keeps the newer one.

Create refuses what libvirt could not revert or delete later: internal
snapshots on the VM, a saved state, a running snapshot job, disks that
are not files, CD-ROM media, a branch after a revert, and memory without
a running VM or room in the first disk's pool. A disk-only revert keeps
the VM running, paused or shut off; one with memory resumes it as saved.
A revert in the same second as the previous one, which libvirt fails
after stopping the VM, is retried once.
A snapshot the old code took of a UEFI VM saved the loader as rom; its
revert is refused (deleting it still works).

The snapshots tab lists internal and external snapshots separately, can
save memory too, shows running snapshot jobs, and the revert confirmation
names the disks it detaches and that NVRAM is not restored.

instances/tests/test_destroy.py

@@ -50,7 +50,6 @@ class DestroyOrderTestCase(TestCase):
     def test_failed_undefine_deletes_no_disk(self):
         proxy = self.destroy(undefine_error=libvirtError("refused"))
         proxy.get_volume_by_path.assert_not_called()
-        proxy.delete_all_disks.assert_not_called()
         self.assertTrue(Instance.objects.filter(pk=self.instance.pk).exists())
 
     def test_destroy_and_delete_vol_hold_the_compute_exclusively(self):

instances/tests/test_live_dataloss.py

@@ -92,7 +92,6 @@ class LiveDataLossTestCase(TestCase):
 
     # External snapshots
 
-    @unittest.expectedFailure
     def test_revert_external_snapshot_keeps_disk_added_later(self):
         base = livetest.create_volume(self.conn, P + "r03-a")
         dom, inst = self.vm("r03-a", [base])
@@ -104,11 +103,11 @@ class LiveDataLossTestCase(TestCase):
             f"<source file='{later}'/><target dev='vdb' bus='virtio'/></disk>",
             libvirt.VIR_DOMAIN_AFFECT_CONFIG,
         )
-        self.post("revert_external_snapshot", inst, {"name": "s1.snap", "date": "", "desc": ""})
+        self.post("revert_external_snapshot", inst, {"name": "snap"})
 
         self.assertTrue(livetest.volume_exists(self.conn, later), "revert deleted a disk the snapshot never had")
+        self.assertEqual(set(self.disk_sources(dom, libvirt.VIR_DOMAIN_XML_INACTIVE)), {"vda"})
 
-    @unittest.expectedFailure
     def test_delete_older_external_snapshot_keeps_newer_snapshot_files(self):
         base = livetest.create_volume(self.conn, P + "r03-b")
         dom, inst = self.vm("r03-b", [base])
@@ -116,11 +115,11 @@ class LiveDataLossTestCase(TestCase):
         self.post("create_external_snapshot", inst, {"name": "newer"})
         sources = set(self.disk_sources(dom).values())
 
-        self.post("delete_external_snapshot", inst, {"name": "s1.older"})
+        self.post("delete_external_snapshot", inst, {"name": "older"})
 
         for path in sources:
             self.assertTrue(livetest.volume_exists(self.conn, path), f"{path} was deleted")
-        self.assertIn("s1.newer", dom.snapshotListNames(0))
+        self.assertIn("newer", dom.snapshotListNames(0))
 
     # Internal snapshots of UEFI VMs
 

instances/tests/test_live_snapshots.py

@@ -0,0 +1,144 @@
+"""Live checks for external snapshots on libvirt's own API. Runs only with
+TEST_LIBVIRT_HOST set (see instances/tests/livetest.py)."""
+
+import os
+import unittest
+
+import libvirt
+from django.test import SimpleTestCase
+from lxml import etree
+from vrtManager import util
+from vrtManager.connection import connection_manager
+from vrtManager.instance import wvmInstance
+
+from . import livetest
+
+P = livetest.PREFIX
+
+
+class LiveExternalSnapshotTestCase(SimpleTestCase):
+    @classmethod
+    def setUpClass(cls):
+        if not livetest.enabled():
+            raise unittest.SkipTest("Set TEST_LIBVIRT_HOST to run the live libvirt tests")
+        cls.args = (
+            os.environ["TEST_LIBVIRT_HOST"],
+            os.environ.get("TEST_LIBVIRT_LOGIN", ""),
+            os.environ.get("TEST_LIBVIRT_PASSWORD", ""),
+            int(os.environ.get("TEST_LIBVIRT_TYPE", 4)),
+        )
+        cls.conn = connection_manager.get_connection(*cls.args)
+        livetest.cleanup(cls.conn)
+        livetest.ensure_pool(cls.conn)
+        cls.inventory = livetest.inventory(cls.conn)
+        super().setUpClass()
+
+    @classmethod
+    def tearDownClass(cls):
+        try:
+            livetest.remove_pool(cls.conn)
+        finally:
+            super().tearDownClass()
+        if livetest.inventory(cls.conn) != cls.inventory:
+            raise AssertionError("Live tests changed objects outside the test namespace")
+
+    def tearDown(self):
+        livetest.cleanup(self.conn)
+
+    def vm(self, name, state="stopped", uefi=False, extra_devices=""):
+        base = livetest.create_volume(self.conn, P + name)
+        dom = livetest.define_vm(self.conn, P + name, [base], uefi=uefi, extra_devices=extra_devices)
+        if state in ("running", "paused"):
+            dom.create()
+        if state == "paused":
+            dom.suspend()
+        return dom, wvmInstance(*self.args, None, uuid=dom.UUIDString())
+
+    def top(self, dom):
+        return etree.fromstring(dom.XMLDesc(0)).xpath("string(devices/disk[@device='disk']/source/@file)")
+
+    def test_create_revert_delete_keep_the_state(self):
+        for state, expected in (("stopped", libvirt.VIR_DOMAIN_SHUTOFF),
+                                ("running", libvirt.VIR_DOMAIN_RUNNING),
+                                ("paused", libvirt.VIR_DOMAIN_PAUSED)):
+            with self.subTest(state):
+                dom, vm = self.vm("snap-" + state, state)
+                base = self.top(dom)
+                vm.create_external_snapshot("one", "first")
+                vm.create_external_snapshot("two")
+                vm.revert_external_snapshot("one")
+                self.assertEqual(dom.state()[0], expected)
+                vm.delete_external_snapshot("two")
+                vm.delete_external_snapshot("one")
+                self.assertEqual(dom.snapshotNum(0), 0)
+                self.assertEqual(dom.state()[0], expected)
+                self.assertTrue(livetest.volume_exists(self.conn, base))
+
+    def test_memory_snapshot_resumes_and_its_file_goes_with_it(self):
+        dom, vm = self.vm("snap-mem", "running")
+        vm.create_external_snapshot("m", memory=True)
+        mem = os.path.join(livetest.POOL_PATH, f"{dom.UUIDString()}.m.mem")
+        self.assertTrue(livetest.volume_exists(self.conn, mem))
+        dom.suspend()
+        vm.revert_external_snapshot("m")
+        self.assertEqual(dom.state()[0], libvirt.VIR_DOMAIN_RUNNING)
+        vm.delete_external_snapshot("m")
+        self.assertFalse(livetest.volume_exists(self.conn, mem))
+
+    def test_uefi_vm_keeps_its_loader(self):
+        dom, vm = self.vm("snap-uefi", "running", uefi=True)
+        os_before = etree.tostring(etree.fromstring(dom.XMLDesc(libvirt.VIR_DOMAIN_XML_INACTIVE)).find("os"))
+        vm.create_external_snapshot("u")
+        vm.revert_external_snapshot("u")
+        vm.delete_external_snapshot("u")
+        self.assertEqual(etree.tostring(etree.fromstring(dom.XMLDesc(libvirt.VIR_DOMAIN_XML_INACTIVE)).find("os")), os_before)
+        self.assertEqual(dom.snapshotNum(0), 0)
+
+    def test_back_to_back_reverts_keep_a_running_vm_running(self):
+        # Without the retry, the second revert within one second stopped the VM.
+        dom, vm = self.vm("snap-collide", "running")
+        vm.create_external_snapshot("a")
+        for _ in range(3):
+            vm.revert_external_snapshot("a")
+            vm.revert_external_snapshot("a")
+            self.assertEqual(dom.state()[0], libvirt.VIR_DOMAIN_RUNNING)
+
+    def test_a_mixed_vm_can_be_cleaned_up(self):
+        dom, vm = self.vm("snap-mixed-clean")
+        dom.snapshotCreateXML("<domainsnapshot><name>i</name></domainsnapshot>", 0)
+        dom.snapshotCreateXML(
+            "<domainsnapshot><name>e</name></domainsnapshot>", libvirt.VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY
+        )
+        # libvirt refuses an external snapshot whose parent is internal, so the
+        # internal one goes first
+        vm.snapshot_delete("i")
+        vm.delete_external_snapshot("e")
+        self.assertEqual(dom.snapshotNum(0), 0)
+
+    def test_chain_stays_linear(self):
+        dom, vm = self.vm("snap-linear")
+        vm.create_external_snapshot("a")
+        vm.create_external_snapshot("b")
+        vm.revert_external_snapshot("a")
+        with self.assertRaisesRegex(util.OperationError, "Newer snapshots"):
+            vm.create_external_snapshot("c")
+        vm.delete_external_snapshot("b")
+        vm.create_external_snapshot("c")
+
+    def test_refusals(self):
+        iso = livetest.create_volume(self.conn, P + "snap-media")
+        cdrom = (f"<disk type='file' device='cdrom'><driver name='qemu' type='qcow2'/><source file='{iso}'/>"
+                 "<target dev='sda' bus='sata'/><readonly/></disk>")
+        _, vm = self.vm("snap-cdrom", extra_devices=cdrom)
+        with self.assertRaisesRegex(util.OperationError, "Eject"):
+            vm.create_external_snapshot("x")
+
+        dom, vm = self.vm("snap-saved", "running")
+        dom.managedSave(0)
+        with self.assertRaisesRegex(util.OperationError, "saved state"):
+            vm.create_external_snapshot("x")
+
+        dom, vm = self.vm("snap-mixed")
+        dom.snapshotCreateXML("<domainsnapshot><name>i</name></domainsnapshot>", 0)
+        with self.assertRaisesRegex(util.OperationError, "internal snapshots"):
+            vm.create_external_snapshot("x")

instances/tests/test_page_controls.py

@@ -69,7 +69,7 @@ class PageControls:
             }
             for getter in ("get_networks", "get_ifaces", "get_storages", "get_disk_devices",
                            "get_media_devices", "get_net_devices", "get_snapshot",
-                           "get_external_snapshots"):
+                           "get_snapshot_jobs"):
                 getattr(proxy, getter).return_value = []
             proxy.get_console_type.return_value = "vnc"
             proxy.get_status.return_value = status

instances/tests/test_pending_edits.py

@@ -45,7 +45,7 @@ class PendingEditsTestCase(TestCase):
             proxy.get_max_cpus.return_value = [1, 2, 3, 4]
             proxy.get_vcpus.return_value = {}
             for getter in ("get_networks", "get_ifaces", "get_storages", "get_media_devices", "get_net_devices",
-                           "get_snapshot", "get_external_snapshots"):
+                           "get_snapshot", "get_snapshot_jobs"):
                 getattr(proxy, getter).return_value = []
             proxy.get_disk_devices.side_effect = lambda **kw: config if kw.get("config") else live
             proxy.get_all_qos.side_effect = lambda **kw: qos[1] if kw.get("config") else qos[0]

instances/tests/test_snapshot_views.py

@@ -0,0 +1,124 @@
+"""The snapshot views call the backend once and show its refusals as
+messages; they no longer start, pause or stop the VM themselves."""
+
+from datetime import datetime
+from unittest.mock import PropertyMock, patch
+
+import libvirt
+from django.contrib.auth import get_user_model
+from django.contrib.messages import get_messages
+from django.test import TestCase
+from django.urls import reverse
+from lxml import html as lxml_html
+from vrtManager import util
+
+from computes.models import Compute
+from instances.models import Instance
+
+
+class SnapshotViewsTestCase(TestCase):
+    def setUp(self):
+        compute = Compute.objects.create(name="snap-compute", hostname="127.0.0.1:1", login="root", password="", type=1)
+        self.instance = Instance.objects.create(
+            compute=compute, name="snap-vm", uuid="dddddddd-eeee-ffff-0000-222222222222"
+        )
+        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):
+        with patch("instances.models.wvmInstance") as mock_wvm:
+            proxy = mock_wvm.return_value
+            method = {
+                "create_external_snapshot": proxy.create_external_snapshot,
+                "revert_external_snapshot": proxy.revert_external_snapshot,
+                "delete_external_snapshot": proxy.delete_external_snapshot,
+                "revert_snapshot": proxy.snapshot_revert,
+                "delete_snapshot": proxy.snapshot_delete,
+                "snapshot": proxy.create_snapshot,
+            }[view]
+            if error:
+                method.side_effect = util.OperationError(error)
+            response = self.client.post(
+                reverse(f"instances:{view}", args=[self.instance.id]),
+                data,
+                HTTP_REFERER=reverse("instances:instance", args=[self.instance.id]),
+            )
+        self.assertEqual(response.status_code, 302)
+        return proxy, method, [str(m) for m in get_messages(response.wsgi_request)]
+
+    def test_create_passes_name_description_and_memory(self):
+        _, method, _ = self.post("create_external_snapshot", {"name": "a", "description": "d", "memory": "on"})
+        method.assert_called_once_with("a", "d", True)
+        _, method, _ = self.post("create_external_snapshot", {"name": "a", "description": ""})
+        method.assert_called_once_with("a", "", False)
+
+    def test_revert_and_delete_call_the_backend_once_and_leave_power_alone(self):
+        for view in ("revert_external_snapshot", "delete_external_snapshot"):
+            with self.subTest(view):
+                proxy, method, _ = self.post(view, {"name": "a"})
+                method.assert_called_once_with("a")
+                for power in ("start", "force_shutdown", "shutdown"):
+                    getattr(proxy, power).assert_not_called()
+
+    def test_refusals_become_messages(self):
+        for view in ("create_external_snapshot", "revert_external_snapshot", "delete_external_snapshot",
+                     "revert_snapshot", "delete_snapshot", "snapshot"):
+            with self.subTest(view):
+                _, _, messages = self.post(view, {"name": "a"}, error="refused for a reason")
+                self.assertTrue(any("refused for a reason" in m for m in messages), messages)
+
+
+class SnapshotTabTestCase(TestCase):
+    setUp = SnapshotViewsTestCase.setUp  # same VM and superuser, without rerunning the view tests
+
+    def page(self, external=(), internal=(), jobs=(), status=1, nvram=None):
+        with patch("instances.models.wvmInstance") as mock_wvm, patch.object(
+            Compute, "status", new_callable=PropertyMock, return_value=True
+        ):
+            proxy = mock_wvm.return_value
+            for getter in ("get_networks", "get_ifaces", "get_storages", "get_disk_devices",
+                           "get_media_devices", "get_net_devices"):
+                getattr(proxy, getter).return_value = []
+            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)
+            proxy.get_status.return_value = status
+            proxy.get_nvram.return_value = nvram
+            proxy.get_memory.return_value = proxy.get_cur_memory.return_value = 1024
+            proxy.get_max_memory.return_value = 4096
+            proxy.get_vcpu.return_value = proxy.get_cur_vcpu.return_value = 1
+            proxy.get_max_cpus.return_value = [1]
+            proxy.get_vcpus.return_value = {}
+            proxy.get_console_type.return_value = "vnc"
+            response = self.client.get(reverse("instances:instance", args=[self.instance.id]))
+        self.assertEqual(response.status_code, 200)
+        return lxml_html.fromstring(response.content)
+
+    def snap(self, name, **kw):
+        return {"name": name, "date": datetime(2026, 1, 1), "description": "", "memory": False,
+                "current": False, "detaches": [], **kw}
+
+    def test_revert_confirm_names_detached_disks_and_nvram(self):
+        doc = self.page(external=[self.snap("e", detaches=[{"dev": "vdb", "path": "/pool/later.qcow2"}])],
+                        nvram="/var/lib/libvirt/qemu/nvram/x_VARS.fd")
+        [button] = doc.xpath("//button[@name='revert_external_snapshot']")
+        text = button.get("data-confirm")
+        self.assertIn("vdb (/pool/later.qcow2)", text)
+        self.assertIn("NVRAM", text)
+
+    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"))
+
+    def test_a_running_job_shows_progress_and_disables_actions(self):
+        doc = self.page(external=[self.snap("e")], jobs=[{"dev": "vda", "cur": 1, "end": 4}])
+        self.assertIn("vda 25%", doc.text_content())
+        self.assertTrue(all(b.get("disabled") is not None
+                            for b in doc.xpath("//div[@id='snapshots']//button[@type='submit']")))
+
+    def test_lists_are_separate(self):
+        doc = self.page(internal=[self.snap("i")], external=[self.snap("e")])
+        self.assertEqual(doc.xpath("//table[@id='internal-snapshots']//strong/text()"), ["i"])
+        self.assertEqual(doc.xpath("//table[@id='external-snapshots']//strong/text()"), ["e"])