From ff1508b3c20295c7e008581a06f4e1956ee22ac8 Mon Sep 17 00:00:00 2001 From: catborise Date: Mon, 12 Oct 2026 14:07:33 +0300 Subject: [PATCH] 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. --- README.md | 1 + instances/models.py | 28 +- .../templates/instances/snapshots_tab.html | 179 ++++---- instances/tests/test_destroy.py | 1 - instances/tests/test_live_dataloss.py | 9 +- instances/tests/test_live_snapshots.py | 144 +++++++ instances/tests/test_page_controls.py | 2 +- instances/tests/test_pending_edits.py | 2 +- instances/tests/test_snapshot_views.py | 124 ++++++ instances/views.py | 44 +- vrtManager/instance.py | 299 +++++++------ vrtManager/tests/test_external_snapshots.py | 394 ++++++++++++++++++ vrtManager/tests/test_xml_safety.py | 78 +--- 13 files changed, 994 insertions(+), 311 deletions(-) create mode 100644 instances/tests/test_live_snapshots.py create mode 100644 instances/tests/test_snapshot_views.py create mode 100644 vrtManager/tests/test_external_snapshots.py diff --git a/README.md b/README.md index f052db96..1098b3aa 100644 --- a/README.md +++ b/README.md @@ -556,6 +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. - 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/models.py b/instances/models.py index f49d500b..0bf4f468 100644 --- a/instances/models.py +++ b/instances/models.py @@ -2,7 +2,12 @@ from computes.models import Compute from django.db import models from django.utils.functional import cached_property from django.utils.translation import gettext_lazy as _ -from libvirt import VIR_DOMAIN_XML_SECURE, libvirtError +from libvirt import ( + VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL, + VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL, + VIR_DOMAIN_XML_SECURE, + libvirtError, +) from vrtManager.instance import wvmInstance from webvirtcloud.settings import QEMU_CONSOLE_LISTENER_ADDRESSES @@ -253,13 +258,24 @@ class Instance(models.Model): def video_models(self): return self.proxy.get_video_models(self.arch, self.machine) - @cached_property - def snapshots(self) -> list[dict]: - return sorted(self.proxy.get_snapshot(), reverse=True, key=lambda k: k["date"]) + def _snapshots(self, flag): + return sorted(self.proxy.get_snapshot(flag), reverse=True, key=lambda k: k["date"]) @cached_property - def external_snapshots(self): - return sorted(self.proxy.get_external_snapshots(), reverse=True, key=lambda k: k["date"]) + def snapshots(self) -> list[dict]: + return self._snapshots(VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL | VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL) + + @cached_property + def internal_snapshots(self) -> list[dict]: + return self._snapshots(VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL) + + @cached_property + def external_snapshots(self) -> list[dict]: + return self._snapshots(VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL) + + @cached_property + def snapshot_jobs(self) -> list[dict]: + return self.proxy.get_snapshot_jobs() @cached_property def inst_xml(self): diff --git a/instances/templates/instances/snapshots_tab.html b/instances/templates/instances/snapshots_tab.html index 6cecd372..9700b592 100755 --- a/instances/templates/instances/snapshots_tab.html +++ b/instances/templates/instances/snapshots_tab.html @@ -1,6 +1,7 @@ {% load i18n %} {% load bootstrap_icons %}
+ {% with busy=instance.snapshot_jobs %}
+ {% if busy %} +
+ {% trans "A snapshot job is running:" %} + {% for job in busy %}{{ job.dev }} {% widthratio job.cur job.end 100 %}%{% if not forloop.last %}, {% endif %}{% endfor %}. + {% trans "Reload the page to see its progress." %} +
+ {% endif %}
{% if instance.status != 5 %}

{% trans "With running machine, internal snapshots may take more than an hour, depending on how much memory has on your instance and how large the disk is." %}

@@ -34,116 +42,137 @@ | - {% if instance.external_snapshots|length > 0 %} - + {% if instance.external_snapshots or busy %} + {% else %} - + {% endif %}
- {% if instance.status != 5 %} -

{% trans "You can get external snapshots within this tab." %}

- {% else %} -

{% trans "Create an external snapshot" %}

- {% endif %} -

{% trans "External snapshots are experimental in this stage, use it if you know what you are doing. 'Revert Snapshot' may require manual operation with CLI." %}

-
+

{% trans "Create an external snapshot. Each disk keeps its state in a new file next to it." %}

+ {% csrf_token %}
- + | - {% if instance.external_snapshots|length > 0 or instance.snapshots|length > 0 %} - + {% if instance.internal_snapshots or busy %} + {% else %} - + {% endif %}
-

{% trans "WebVirtCloud supports only one external snapshot at the moment." %}

+
+ + +
+

{% trans "A VM with snapshots cannot be migrated." %}

- {% if instance.snapshots or instance.external_snapshots %} + {% if instance.internal_snapshots or instance.external_snapshots %}

{% trans "Choose a snapshot for restore/delete" %}

+ {% if instance.internal_snapshots %} +
{% trans "Internal" %}
- +
- + - {% if instance.snapshots %} - {% for snap in instance.snapshots %} - - - - - - - - {% endfor %} - {% elif instance.external_snapshots %} - {% for ext_snap in instance.external_snapshots %} - - - - - + + + + - - - {% endfor %} - {% endif %} + {% endif %} + + + + + {% endfor %}
{% trans "Date" %} {% trans "Name" %}{% trans "Type - Description" %}{% trans "Description" %} {% trans "Action" %}
{{ snap.date|date:"M d H:i:s" }}{{ snap.name }}({% trans "Internal" %}) - {{ snap.description }} -
- {% csrf_token %} - - {% if instance.status == 5 %} - - {% else %} - - {% endif %} -
- 		-
{% csrf_token %} - - -
-
{{ ext_snap.date|date:"M d H:i:s" }}{{ ext_snap.name }}({% trans "External" %}) - {{ ext_snap.description }} -
- {% csrf_token %} - - - -
{{ snap.date|date:"M d H:i:s" }}{{ snap.name }}{{ snap.description }} + + {% csrf_token %} + + {% if instance.status == 5 %} + - - -
{% csrf_token %} - - -
-
+
{% csrf_token %} + + +
+
+ {% endif %} + {% if instance.external_snapshots %} +
{% trans "External" %}
+
+ + + + + + + + + {% for ext_snap in instance.external_snapshots %} + + + + + + + + {% endfor %} + +
{% trans "Date" %}{% trans "Name" %}{% trans "Description" %}{% trans "Action" %}
{{ ext_snap.date|date:"M d H:i:s" }} + {{ ext_snap.name }} + {% if ext_snap.current %}{% trans "current" %}{% endif %} + {% if ext_snap.memory %}{% trans "memory" %}{% endif %} + {{ ext_snap.description }} +
+ {% csrf_token %} + + +
+ 		+
{% csrf_token %} + + +
+
+
+ {% endif %} {% else %}

{% trans "You do not have any snapshots" %}

{% endif %}
+ {% endwith %} diff --git a/instances/tests/test_destroy.py b/instances/tests/test_destroy.py index 61a02521..f42dd89e 100644 --- a/instances/tests/test_destroy.py +++ b/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): diff --git a/instances/tests/test_live_dataloss.py b/instances/tests/test_live_dataloss.py index 6e131c1f..7629c333 100644 --- a/instances/tests/test_live_dataloss.py +++ b/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"", 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 diff --git a/instances/tests/test_live_snapshots.py b/instances/tests/test_live_snapshots.py new file mode 100644 index 00000000..2c73fb0e --- /dev/null +++ b/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("i", 0) + dom.snapshotCreateXML( + "e", 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"" + "") + _, 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("i", 0) + with self.assertRaisesRegex(util.OperationError, "internal snapshots"): + vm.create_external_snapshot("x") diff --git a/instances/tests/test_page_controls.py b/instances/tests/test_page_controls.py index 8ff71945..effcfa6b 100644 --- a/instances/tests/test_page_controls.py +++ b/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 diff --git a/instances/tests/test_pending_edits.py b/instances/tests/test_pending_edits.py index 56e93e44..1727a345 100644 --- a/instances/tests/test_pending_edits.py +++ b/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] diff --git a/instances/tests/test_snapshot_views.py b/instances/tests/test_snapshot_views.py new file mode 100644 index 00000000..bbd09584 --- /dev/null +++ b/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"]) diff --git a/instances/views.py b/instances/views.py index 7365a8ac..a353cfec 100755 --- a/instances/views.py +++ b/instances/views.py @@ -35,7 +35,6 @@ from libvirt import (VIR_DOMAIN_UNDEFINE_KEEP_NVRAM, VIR_DOMAIN_UNDEFINE_MANAGED_SAVE, VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, VIR_DOMAIN_UNDEFINE_NVRAM, - VIR_DOMAIN_START_PAUSED, libvirtError) from logs.views import addlogmsg from webvirtcloud.middleware import error_text @@ -1342,7 +1341,11 @@ def delete_snapshot(request, pk): if not request.user.has_perm("instances.snapshot_instances"): raise PermissionDenied snap_name = request.POST.get("name", "") - instance.proxy.snapshot_delete(snap_name) + try: + instance.proxy.snapshot_delete(snap_name) + except libvirtError as err: + messages.error(request, _("Snapshot was not deleted: %(err)s") % {"err": error_text(request, err)}) + return _back(request, pk, "managesnapshot") msg = _("Delete snapshot: %(snap)s") % {"snap": snap_name} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) return _back(request, pk, "managesnapshot") @@ -1376,9 +1379,14 @@ def create_external_snapshot(request, pk): raise PermissionDenied name = request.POST.get("name", "") desc = request.POST.get("description", "") - instance.proxy.create_external_snapshot("s1." + name, instance, desc=desc) - msg = _("Create external snapshot: %(snap)s") % {"snap": name} - addlogmsg(request.user.username, instance.compute.name, instance.name, msg) + memory = request.POST.get("memory") == "on" + try: + instance.proxy.create_external_snapshot(name, desc, memory) + except libvirtError as err: + messages.error(request, _("Snapshot was not created: %(err)s") % {"err": error_text(request, err)}) + else: + msg = _("Create external snapshot: %(snap)s") % {"snap": name} + addlogmsg(request.user.username, instance.compute.name, instance.name, msg) return _back(request, pk, "managesnapshot") @@ -1388,15 +1396,15 @@ def revert_external_snapshot(request, pk): instance = get_instance(request.user, pk, perm_type="change") if not request.user.has_perm("instances.snapshot_instances"): raise PermissionDenied - instance_state = True if instance.proxy.get_status() != 5 else False name = request.POST.get("name", "") - date = request.POST.get("date", "") - desc = request.POST.get("desc", "") - instance.proxy.force_shutdown() if instance_state else None - instance.proxy.revert_external_snapshot(name, date, desc) - instance.proxy.start() if instance_state else None - msg = _("Revert external snapshot: %(snap)s") % {"snap": name} - addlogmsg(request.user.username, instance.compute.name, instance.name, msg) + try: + 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) + msg = _("Revert external snapshot: %(snap)s") % {"snap": name} + addlogmsg(request.user.username, instance.compute.name, instance.name, msg) return _back(request, pk, "managesnapshot") @@ -1406,18 +1414,14 @@ def delete_external_snapshot(request, pk): instance = get_instance(request.user, pk, perm_type="change") if not request.user.has_perm("instances.snapshot_instances"): raise PermissionDenied - instance_state = True if instance.proxy.get_status() == 5 else False name = request.POST.get("name", "") - - instance.proxy.start(VIR_DOMAIN_START_PAUSED) if instance_state else None - try: 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: msg = _("Delete external snapshot: %(snap)s") % {"snap": name} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - finally: - instance.proxy.force_shutdown() if instance_state else None - return _back(request, pk, "managesnapshot") diff --git a/vrtManager/instance.py b/vrtManager/instance.py index a5ac0b52..8e479be8 100644 --- a/vrtManager/instance.py +++ b/vrtManager/instance.py @@ -3,6 +3,7 @@ import ipaddress import json import logging import os.path +import re import string import threading import time @@ -31,14 +32,13 @@ try: VIR_MIGRATE_PERSIST_DEST, VIR_MIGRATE_UNDEFINE_SOURCE, VIR_MIGRATE_UNSAFE, + VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC, VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY, - VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY, + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED, + VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING, + VIR_ERR_CONFIG_UNSUPPORTED, VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL, VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL, - VIR_DOMAIN_BLOCK_COMMIT_DELETE, - VIR_DOMAIN_BLOCK_COMMIT_ACTIVE, - VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT, - VIR_DOMAIN_START_PAUSED, libvirtError, ) from libvirt_qemu import VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT, qemuAgentCommand @@ -59,6 +59,9 @@ from vrtManager.storage import wvmStorage, wvmStorages # with secrets. Live XML would drop pending changes, non-secure XML the VNC # password. PERSISTENT_XML = VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE + +# External snapshot names become part of overlay and memory file names. +SNAPSHOT_NAME = re.compile(r"[A-Za-z0-9_-][A-Za-z0-9._-]{0,63}") # the longest window wvmInstance.usage() averages over USAGE_MAX_WINDOW = 30 @@ -1509,17 +1512,12 @@ class wvmInstance(wvmConnect): [path for path in own if path in used_elsewhere], ) - def delete_all_disks(self): - self.refresh_instance_pools() - disks = self.get_disk_devices() - for disk in disks: - vol = self.get_volume_by_path(disk.get("path")) - vol.delete(0) - def _snapshotCreateXML(self, xml, flag): self.instance.snapshotCreateXML(xml, flag) def create_snapshot(self, name, desc=None): + if self.instance.snapshotListNames(VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL): + raise util.OperationError("The VM has external snapshots; internal snapshots cannot be mixed with them") state = "shutoff" if self.get_status() == 5 else "running" name_esc = util.xml_escape(name) desc_esc = util.xml_escape(desc) if desc else "" @@ -1540,139 +1538,184 @@ class wvmInstance(wvmConnect): """ self._snapshotCreateXML(xml, 0) - def change_snapshot_xml(self): - try: - raw_xml = self._XMLDesc(VIR_DOMAIN_XML_SECURE) - tree = ElementTree.fromstring(raw_xml) - loader = tree.find("./os/loader") - if loader is not None and loader.get("type") == "pflash": - loader.set("type", "rom") - self._defineXML(ElementTree.tostring(tree, encoding="unicode")) - return True - return False - except Exception as e: - logger.error("Failed to change snapshot XML loader: %s", e) - raise - - def recover_snapshot_xml(self): - try: - raw_xml = self._XMLDesc(VIR_DOMAIN_XML_SECURE) - tree = ElementTree.fromstring(raw_xml) - loader = tree.find("./os/loader") - if loader is not None and loader.get("type") == "rom": - loader.set("type", "pflash") - self._defineXML(ElementTree.tostring(tree, encoding="unicode")) - return True - return False - except Exception as e: - logger.error("Failed to recover snapshot XML loader: %s", e) - raise - - def create_external_snapshot(self, name, date=None, desc=None): - creation_time = time.time() - state = "shutoff" if self.get_status() == 5 else "running" - name_esc = util.xml_escape(name) - desc_esc = util.xml_escape(desc) if desc else "" - # - xml = """ - %s - %s - %s - %d - """ % ( - name_esc, - desc_esc, - state, - creation_time, + def create_external_snapshot(self, name, desc="", memory=False): + """A disk-only snapshot, or with memory too (running VMs only). Every + disk gets a libvirt-named overlay next to it; the memory file goes + next to the first disk.""" + if not SNAPSHOT_NAME.fullmatch(name): + raise util.OperationError( + "Snapshot names are 1-64 letters, digits, '.', '_' or '-' and do not start with '.'" + ) + self._check_snapshot_state() + if self.instance.snapshotListNames(VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL): + raise util.OperationError("The VM has internal snapshots; external snapshots cannot be mixed with them") + if name in self.instance.snapshotListNames(0): + raise util.OperationError(f"A snapshot named {name} already exists") + if self.instance.hasCurrentSnapshot(0): + if self.instance.snapshotCurrent(0).numChildren(0): + raise util.OperationError("Newer snapshots exist after the last revert; delete them first") + elif self.instance.snapshotListNames(0): + # only after changes outside WebVirtCloud; a create would start a second root + raise util.OperationError("No snapshot is current; revert to a snapshot first") + tree = etree.fromstring(self._XMLDesc(0)) + for disk in tree.xpath("devices/disk"): + dev = disk.xpath("string(target/@dev)") + if disk.get("device") in ("cdrom", "floppy"): + if disk.xpath("source/@file|source/@dev|source/@name"): + raise util.OperationError(f"Eject the media from {dev} first") + elif disk.get("type") != "file": + raise util.OperationError(f"Disk {dev} is not a file; external snapshots need file disks") + memory_xml = self._snapshot_memory_xml(name, tree) if memory else "" + xml = ( + f"{name}" + f"{util.xml_escape(desc)}{memory_xml}" ) - - changed = self.change_snapshot_xml() - try: - xml += self._XMLDesc(VIR_DOMAIN_XML_SECURE) - xml += """0 - """ - - self._snapshotCreateXML(xml, VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) - finally: - if changed: - self.recover_snapshot_xml() + flags = VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC + if not memory: + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY + self._snapshotCreateXML(xml, flags) + self.refresh_instance_pools() + + def _snapshot_memory_xml(self, name, tree): + """The element of a snapshot that saves memory: a file in the + storage pool of the first disk, which must have room for it.""" + if self.get_status() != 1: + raise util.OperationError("Memory is saved only from a running VM") + disks = list(self._disk_sources(tree).values()) + if not disks: + raise util.OperationError("The VM has no disk to keep its memory next to") + # the top overlay is known to its pool only after a refresh + self.refresh_instance_pools() + try: + pool = self.get_volume_by_path(disks[0]).storagePoolLookupByVolume() + except libvirtError: + raise util.OperationError(f"{disks[0]} is not in a storage pool; memory is saved only into one") from None + if pool.info()[3] < self.get_cur_memory() * 1024 * 1024: + raise util.OperationError(f"Storage pool {pool.name()} has less free space than the VM's memory") + path = os.path.join(os.path.dirname(disks[0]), f"{self.get_uuid()}.{name}.mem") + return f"" + + 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.""" + snap = self._snapshot(name, external=True) + self._check_snapshot_state() + tree = etree.fromstring(snap.getXMLDesc(0)) + # Snapshots of UEFI VMs taken before WebVirtCloud used libvirt's + # snapshot API saved the loader switched to rom; a revert would + # restore that. + saved = self._saved_definition(tree) + saved_loader = saved.xpath("string(os/loader/@type)") if saved is not None else "" + current_loader = util.get_xml_path(self._XMLDesc(PERSISTENT_XML), "/domain/os/loader/@type") + if saved_loader == "rom" and current_loader == "pflash": + raise util.OperationError( + "This snapshot was taken by an older version on a UEFI VM and cannot be reverted; delete it instead" + ) + if tree.find("memory[@snapshot='external']") is not None: + flags = 0 + else: + flags = {1: VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING, 3: VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED}.get( + self.get_status(), 0 + ) + # 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. + try: + self.instance.revertToSnapshot(snap, flags) + except libvirtError as err: + if err.get_error_code() != VIR_ERR_CONFIG_UNSUPPORTED or "already exists" not in str(err): + raise + time.sleep(1) + try: + self.instance.revertToSnapshot(snap, flags) + except libvirtError as retry_err: + state = "running" if self.instance.isActive() else "shut off" + 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() - def get_external_snapshots(self): - return self.get_snapshot(VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL) - def delete_external_snapshot(self, name): - disk_info = self.get_disk_devices() - for disk in disk_info: - target_dev = disk["dev"] - backing_file = disk["backing_file"] - snap_source_file = disk["path"] - self.instance.blockCommit(target_dev, backing_file, snap_source_file, - flags=VIR_DOMAIN_BLOCK_COMMIT_DELETE| - VIR_DOMAIN_BLOCK_COMMIT_ACTIVE) - while True: - info = self.instance.blockJobInfo(target_dev, 0) - if info.get('cur') == info.get('end'): - self.instance.blockJobAbort(target_dev,flags=VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT) - time.sleep(2) - break - # Check again pool for snapshot delta volume; if it exist, remove it manually - with contextlib.suppress(libvirtError): - vol_snap = self.get_volume_by_path(snap_source_file) - pool = vol_snap.storagePoolLookupByVolume() - pool.refresh(0) - vol_snap.delete(0) + """libvirt merges the snapshot's overlays and removes its files.""" + snap = self._snapshot(name, external=True) + self._check_snapshot_state() + snap.delete(0) + self.refresh_instance_pools() - snap = self.instance.snapshotLookupByName(name, 0) - snap.delete(VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY) + @staticmethod + def _disk_sources(xml): + """Target dev -> source file of the disks (not CD-ROMs) in a domain definition.""" + tree = etree.fromstring(xml) if isinstance(xml, str) else xml + return { + disk.xpath("string(target/@dev)"): disk.xpath("string(source/@file|source/@dev|source/@name)") + for disk in tree.xpath("devices/disk[@device='disk']") + } - - def revert_external_snapshot(self, name, date, desc): - snap = self.instance.snapshotLookupByName(name, 0) - snap_xml = snap.getXMLDesc(0) - snapXML = ElementTree.fromstring(snap_xml) + @staticmethod + def _saved_definition(tree): + """The definition a revert to this snapshot restores as the persistent one.""" + definition = tree.find("inactiveDomain") + return tree.find("domain") if definition is None else definition - self.start(flags=VIR_DOMAIN_START_PAUSED) if self.get_status() == 5 else None - self.delete_all_disks() - - self.force_shutdown() - - snap.delete(VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY) - - disks = snapXML.findall('inactiveDomain/devices/disk') - if not disks: disks = snapXML.findall('domain/devices/disk') - for disk in disks: - self.instance.updateDeviceFlags(ElementTree.tostring(disk).decode("UTF-8")) - name = name.replace("s1", "s2") - self.create_external_snapshot(name, date, desc) - - def get_snapshot(self, flag=VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL|VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL): + 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)) snapshots = [] - snapshot_list = self.instance.snapshotListNames(flag) - for snapshot in snapshot_list: - snap = self.instance.snapshotLookupByName(snapshot, 0) - snap_description = util.get_xml_path( - snap.getXMLDesc(0), "/domainsnapshot/description" - ) - snap_time_create = util.get_xml_path( - snap.getXMLDesc(0), "/domainsnapshot/creationTime" - ) + 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 {} snapshots.append( { - "date": datetime.fromtimestamp(int(snap_time_create)), - "name": snapshot, - "description": snap_description, + "date": datetime.fromtimestamp(int(tree.findtext("creationTime"))), + "name": snap.getName(), + "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 + ], } ) return snapshots + def get_snapshot_jobs(self): + """Block jobs (snapshot merges) running on the VM's disks.""" + if not self.instance.isActive(): + return [] + jobs = [] + for dev in self._disk_sources(self._XMLDesc(0)): + info = self.instance.blockJobInfo(dev, 0) + if info: + jobs.append({"dev": dev, "cur": info["cur"], "end": info["end"]}) + return jobs + + def _snapshot(self, name, external): + """The named snapshot; a snapshot of the other kind is refused.""" + flag = VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL if external else VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL + if name not in self.instance.snapshotListNames(flag): + kind = "external" if external else "internal" + raise util.OperationError(f"The VM has no {kind} snapshot named {name}") + return self.instance.snapshotLookupByName(name, 0) + + def _check_snapshot_state(self): + """Refusals shared by external snapshot create, revert and delete. The + internal/external mix is checked only on create, so a VM that is + already mixed can still be cleaned up.""" + if self.instance.hasManagedSaveImage(0): + raise util.OperationError("The VM has a saved state; start it or remove the saved state first") + if self.get_snapshot_jobs(): + raise util.OperationError("A snapshot job is still running on this VM; try again when it ends") + def snapshot_delete(self, snapshot): - snap = self.instance.snapshotLookupByName(snapshot, 0) - snap.delete(0) + self._snapshot(snapshot, external=False).delete(0) def snapshot_revert(self, snapshot): - snap = self.instance.snapshotLookupByName(snapshot, 0) - self.instance.revertToSnapshot(snap, 0) + self.instance.revertToSnapshot(self._snapshot(snapshot, external=False), 0) def get_managed_save_image(self): return self.instance.hasManagedSaveImage(0) diff --git a/vrtManager/tests/test_external_snapshots.py b/vrtManager/tests/test_external_snapshots.py new file mode 100644 index 00000000..f8b65010 --- /dev/null +++ b/vrtManager/tests/test_external_snapshots.py @@ -0,0 +1,394 @@ +"""External snapshots go through libvirt's snapshot API; WebVirtCloud only +checks preconditions and picks the flags. libvirt is mocked here; the live +cases are in instances/tests/test_live_snapshots.py.""" + +import unittest +from datetime import datetime +from unittest.mock import MagicMock + +from django.conf import settings + +if not settings.configured: + settings.configure(MAC_OUI="52:54:10") + +import libvirt +from libvirt import libvirtError +from vrtManager import util +from vrtManager.instance import PERSISTENT_XML, wvmInstance + +INTERNAL = libvirt.VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL +EXTERNAL = libvirt.VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL + +DOMAIN = """ + + + + + +""" + + +def snapshot_xml(name, memory=False, disks=("vda",)): + devices = "".join( + f"" for d in disks + ) + mem = "" if memory else "" + return ( + f"{name}d & e" + f"1700000000{mem}" + f"{devices}" + ) + + +def fake_snap(name, memory=False, disks=("vda",), current=False, children=0): + snap = MagicMock() + snap.getName.return_value = name + snap.getXMLDesc.return_value = snapshot_xml(name, memory, disks) + snap.isCurrent.return_value = current + snap.numChildren.return_value = children + return snap + + +def proxy(status=1, internal=(), external=(), xml=DOMAIN, jobs=None, managed_save=False): + """A wvmInstance without a connection. internal/external are fake snapshots.""" + p = wvmInstance.__new__(wvmInstance) + p._read_cache = None + p._XMLDesc = lambda flags: xml + p.get_status = lambda: status + p.get_uuid = lambda: "11111111-2222-3333-4444-555555555555" + p.get_cur_memory = lambda: 1024 + p.refresh_instance_pools = MagicMock() + p._snapshotCreateXML = MagicMock() + p.wvm = MagicMock() + dom = MagicMock() + snaps = {s.getName(): s for s in (*internal, *external)} + + def list_names(flag): + return [s.getName() for s in {INTERNAL: internal, EXTERNAL: external}.get(flag, (*internal, *external))] + + def list_all(flag): + return list({INTERNAL: internal, EXTERNAL: external}[flag]) + + dom.snapshotListNames.side_effect = list_names + dom.listAllSnapshots.side_effect = list_all + dom.snapshotLookupByName.side_effect = lambda name, flags: snaps[name] + current = [s for s in snaps.values() if s.isCurrent()] + dom.hasCurrentSnapshot.return_value = bool(current) + dom.snapshotCurrent.return_value = current[0] if current else None + 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, {}) + p.instance = dom + return p + + +class SnapshotListTestCase(unittest.TestCase): + def test_entries_carry_memory_current_and_description(self): + p = proxy(external=[fake_snap("one", memory=True, current=True)]) + [entry] = p.get_snapshot(EXTERNAL) + self.assertEqual(entry["name"], "one") + self.assertEqual(entry["description"], "d & e") + self.assertEqual(entry["date"], datetime.fromtimestamp(1700000000)) + self.assertTrue(entry["memory"]) + self.assertTrue(entry["current"]) + + def test_detaches_lists_disks_missing_from_the_snapshot(self): + p = proxy(external=[fake_snap("one", disks=("vda",))]) + [entry] = p.get_snapshot(EXTERNAL) + self.assertEqual(entry["detaches"], [{"dev": "vdb", "path": "/pool/later.qcow2"}]) + + def test_detaches_comes_from_the_persistent_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"}]) + + def test_lists_are_by_kind(self): + p = proxy(internal=[fake_snap("i")], external=[fake_snap("e")]) + self.assertEqual([s["name"] for s in p.get_snapshot(INTERNAL)], ["i"]) + self.assertEqual([s["name"] for s in p.get_snapshot(EXTERNAL)], ["e"]) + + +class SnapshotJobsTestCase(unittest.TestCase): + def test_running_jobs_are_reported_per_disk(self): + p = proxy(jobs={"vda": {"cur": 5, "end": 10, "type": 3, "bandwidth": 0}}) + self.assertEqual(p.get_snapshot_jobs(), [{"dev": "vda", "cur": 5, "end": 10}]) + + def test_a_stopped_vm_has_no_jobs(self): + p = proxy(status=5) + self.assertEqual(p.get_snapshot_jobs(), []) + p.instance.blockJobInfo.assert_not_called() + + +class SnapshotLookupTestCase(unittest.TestCase): + def test_a_name_of_the_other_kind_is_refused(self): + p = proxy(internal=[fake_snap("i")], external=[fake_snap("e")]) + with self.assertRaises(util.OperationError): + p._snapshot("i", external=True) + with self.assertRaises(util.OperationError): + p._snapshot("e", external=False) + self.assertEqual(p._snapshot("e", external=True).getName(), "e") + + +class SnapshotStateTestCase(unittest.TestCase): + def test_managed_save_is_refused(self): + with self.assertRaisesRegex(util.OperationError, "saved state"): + proxy(managed_save=True)._check_snapshot_state() + + def test_running_job_is_refused(self): + with self.assertRaisesRegex(util.OperationError, "still running"): + proxy(jobs={"vda": {"cur": 1, "end": 2}})._check_snapshot_state() + + def test_a_mixed_vm_passes_so_it_can_be_cleaned_up(self): + proxy(internal=[fake_snap("i")], external=[fake_snap("e")])._check_snapshot_state() + + def test_a_clean_vm_passes(self): + proxy()._check_snapshot_state() + + +DISK_ONLY_ATOMIC = libvirt.VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY | libvirt.VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC + + +def created(p): + xml, flags = p._snapshotCreateXML.call_args[0] + return xml, flags + + +class CreateTestCase(unittest.TestCase): + def test_disk_only_snapshot(self): + p = proxy() + p.create_external_snapshot("snap-1", "desc") + xml, flags = created(p) + self.assertEqual(flags, DISK_ONLY_ATOMIC) + self.assertIn("snap-1", xml) + self.assertNotIn(" & 'y'") + xml, _ = created(p) + self.assertIn("<x> &", xml) + self.assertNotIn("", xml) + + def test_memory_snapshot_goes_into_the_first_disks_pool(self): + p = proxy() + p.get_volume_by_path = MagicMock() + pool = p.get_volume_by_path.return_value.storagePoolLookupByVolume.return_value + pool.info.return_value = [2, 0, 0, 2 * 1024**3] + p.create_external_snapshot("m", memory=True) + xml, flags = created(p) + p.get_volume_by_path.assert_called_once_with("/pool/a.qcow2") + self.assertEqual(flags, libvirt.VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) + self.assertIn( + "", xml + ) + + def refused(self, p, pattern, **kwargs): + with self.assertRaisesRegex(util.OperationError, pattern): + p.create_external_snapshot(kwargs.pop("name", "s"), **kwargs) + p._snapshotCreateXML.assert_not_called() + + def test_invalid_names_are_refused(self): + for name in ("", ".hidden", "a/b", "a b", "x" * 65, ""): + with self.subTest(name=name): + self.refused(proxy(), "Snapshot names", name=name) + + def test_names_are_checked_only_on_create(self): + # snapshots made by the old code are named s1. and stay usable + p = proxy(external=[fake_snap("s1.old", current=True)]) + p.instance.revertToSnapshot = MagicMock() + p.revert_external_snapshot("s1.old") + p.instance.revertToSnapshot.assert_called_once() + + def test_a_taken_name_is_refused(self): + self.refused(proxy(external=[fake_snap("s", current=True)]), "already exists") + + def test_shared_state_checks_apply(self): + self.refused(proxy(managed_save=True), "saved state") + + def test_internal_snapshots_are_refused(self): + self.refused(proxy(internal=[fake_snap("i")]), "internal snapshots") + + def test_newer_snapshots_after_a_revert_are_refused(self): + self.refused(proxy(external=[fake_snap("old", current=True, children=1)]), "Newer snapshots") + + def test_snapshots_without_a_current_one_are_refused(self): + # a create would start a second root + self.refused(proxy(external=[fake_snap("old")]), "revert to a snapshot first") + + def test_a_disk_that_is_not_a_file_is_refused(self): + xml = "" + self.refused(proxy(xml=xml), "vda is not a file") + + def test_cdrom_with_media_is_refused(self): + xml = DOMAIN.replace("", "/ovmf/CODE.fd") + snap = fake_snap("s1.old", current=True) + snap.getXMLDesc.return_value = snapshot_xml("s1.old").replace( + "", "/ovmf/CODE.fd" + ) + p = proxy(external=[snap], xml=uefi) + with self.assertRaisesRegex(util.OperationError, "older version"): + p.revert_external_snapshot("s1.old") + p.instance.revertToSnapshot.assert_not_called() + + + def test_an_old_running_uefi_snapshot_is_judged_by_its_inactive_definition(self): + # a running VM's snapshot keeps the live definition (pflash) and the + # persistent one (rom); the revert restores the persistent one + uefi = DOMAIN.replace("", "/ovmf/CODE.fd") + snap = fake_snap("s1.old", current=True) + snap.getXMLDesc.return_value = snapshot_xml("s1.old").replace( + "", "/ovmf/CODE.fd" + ).replace( + "", + "/ovmf/CODE.fd" + "", + ) + p = proxy(external=[snap], xml=uefi) + with self.assertRaisesRegex(util.OperationError, "older version"): + p.revert_external_snapshot("s1.old") + p.instance.revertToSnapshot.assert_not_called() + + def test_a_failed_retry_still_reports_the_vm_state(self): + # the collision already stopped the VM; another error on the retry + # must not hide that + p = proxy(external=[fake_snap("s")]) + p.instance.revertToSnapshot.side_effect = [self.collision(), libvirtError("disk busy")] + p.instance.isActive.return_value = False + with unittest.mock.patch("vrtManager.instance.time.sleep"): + with self.assertRaisesRegex(util.OperationError, "disk busy.*The VM is shut off"): + p.revert_external_snapshot("s") + + +class DeleteTestCase(unittest.TestCase): + def test_delete_is_one_libvirt_call(self): + snap = fake_snap("s", current=True) + p = proxy(external=[snap]) + p.delete_external_snapshot("s") + snap.delete.assert_called_once_with(0) + p.refresh_instance_pools.assert_called_once() + + def test_running_job_is_refused(self): + snap = fake_snap("s") + p = proxy(external=[snap], jobs={"vda": {"cur": 1, "end": 2}}) + with self.assertRaisesRegex(util.OperationError, "still running"): + p.delete_external_snapshot("s") + snap.delete.assert_not_called() + + +class KindTestCase(unittest.TestCase): + def test_internal_endpoints_refuse_external_names(self): + snap = fake_snap("e") + p = proxy(external=[snap]) + for call in (p.snapshot_revert, p.snapshot_delete): + with self.subTest(call.__name__), self.assertRaises(util.OperationError): + call("e") + snap.delete.assert_not_called() + p.instance.revertToSnapshot.assert_not_called() + + def test_external_endpoints_refuse_internal_names(self): + snap = fake_snap("i") + p = proxy(internal=[snap]) + for call in (p.revert_external_snapshot, p.delete_external_snapshot): + with self.subTest(call.__name__), self.assertRaises(util.OperationError): + call("i") + snap.delete.assert_not_called() + + def test_internal_create_refuses_a_vm_with_external_snapshots(self): + p = proxy(external=[fake_snap("e")]) + with self.assertRaisesRegex(util.OperationError, "external snapshots"): + p.create_snapshot("i") + p._snapshotCreateXML.assert_not_called() + + +if __name__ == "__main__": + unittest.main() diff --git a/vrtManager/tests/test_xml_safety.py b/vrtManager/tests/test_xml_safety.py index 3703963f..06bca9ee 100644 --- a/vrtManager/tests/test_xml_safety.py +++ b/vrtManager/tests/test_xml_safety.py @@ -55,28 +55,10 @@ class TestXmlSafety(unittest.TestCase): with self.assertRaises(ValueError): wn.set_qos("inbound", "not_a_number", 200, 300) - def test_snapshot_escaping_and_recovery_on_failure(self): - inst = wvmInstance.__new__(wvmInstance) - inst.get_status = MagicMock(return_value=5) # shutoff - inst.change_snapshot_xml = MagicMock() - inst.recover_snapshot_xml = MagicMock() - inst._XMLDesc = MagicMock(return_value="") - inst._snapshotCreateXML = MagicMock(side_effect=RuntimeError("libvirt exploded")) - - bad_name = "snap"" - bad_desc = "desc&" - - with self.assertRaises(RuntimeError): - inst.create_snapshot(bad_name, bad_desc) - - inst.change_snapshot_xml.assert_not_called() - inst.recover_snapshot_xml.assert_not_called() - def test_snapshot_xml_payload_is_escaped(self): inst = wvmInstance.__new__(wvmInstance) + inst.instance = MagicMock(**{"snapshotListNames.return_value": []}) # no snapshots yet inst.get_status = MagicMock(return_value=5) # shutoff - inst.change_snapshot_xml = MagicMock() - inst.recover_snapshot_xml = MagicMock() inst._XMLDesc = MagicMock(return_value="") inst._snapshotCreateXML = MagicMock(return_value=None) @@ -92,73 +74,20 @@ class TestXmlSafety(unittest.TestCase): self.assertIn("<inject>&quotes", created_xml) self.assertIn("<tag>&and", created_xml) - def test_external_snapshot_recovery(self): - inst = wvmInstance.__new__(wvmInstance) - inst.get_status = MagicMock(return_value=1) - inst.change_snapshot_xml = MagicMock() - inst.recover_snapshot_xml = MagicMock() - inst._XMLDesc = MagicMock(return_value="") - inst._snapshotCreateXML = MagicMock(return_value=None) - inst.refresh_instance_pools = MagicMock() - - inst.create_external_snapshot("ext_snap_1", desc="normal snapshot") - - inst.change_snapshot_xml.assert_called_once() - inst.recover_snapshot_xml.assert_called_once() - inst.refresh_instance_pools.assert_called_once() - - def test_secure_boot_loader_change_and_recovery(self): - inst = wvmInstance.__new__(wvmInstance) - inst.wvm = MagicMock() - uefi_xml = "/usr/share/OVMF/OVMF.fd" - inst._XMLDesc = MagicMock(return_value=uefi_xml) - - inst.change_snapshot_xml() - inst.wvm.defineXML.assert_called_once() - changed_xml = inst.wvm.defineXML.call_args[0][0] - self.assertIn("type=\"rom\"", changed_xml) - self.assertIn("secure=\"yes\"", changed_xml) - - # Test recovery from rom back to pflash - inst._XMLDesc = MagicMock(return_value=changed_xml) - inst.wvm.defineXML.reset_mock() - inst.recover_snapshot_xml() - inst.wvm.defineXML.assert_called_once() - recovered_xml = inst.wvm.defineXML.call_args[0][0] - self.assertIn("type=\"pflash\"", recovered_xml) - self.assertIn("secure=\"yes\"", recovered_xml) - def test_original_rom_loader_preserved(self): inst = wvmInstance.__new__(wvmInstance) + inst.instance = MagicMock(**{"snapshotListNames.return_value": []}) # no snapshots yet inst._XMLDesc = MagicMock(return_value="/usr/share/rom.bin") inst._defineXML = MagicMock() inst._snapshotCreateXML = MagicMock(return_value=None) inst.get_status = MagicMock(return_value=5) - # change_snapshot_xml should return False and not define XML - self.assertFalse(inst.change_snapshot_xml()) - inst._defineXML.assert_not_called() - inst.create_snapshot("snap_rom", "testing rom preservation") inst._defineXML.assert_not_called() - def test_malformed_xml_raises_in_change(self): - inst = wvmInstance.__new__(wvmInstance) - inst._XMLDesc = MagicMock(return_value="/path") - inst._defineXML = MagicMock(side_effect=RuntimeError("libvirt defineXML failed")) - with self.assertRaises(RuntimeError): - inst.recover_snapshot_xml() - - def test_snapshot_revert_rom_loader_preserved(self): inst = wvmInstance.__new__(wvmInstance) - inst.instance = MagicMock() + inst.instance = MagicMock(**{"snapshotListNames.return_value": ["snap_rom"]}) mock_snap = MagicMock() inst.instance.snapshotLookupByName = MagicMock(return_value=mock_snap) inst.instance.revertToSnapshot = MagicMock() @@ -173,6 +102,7 @@ class TestXmlSafety(unittest.TestCase): def test_internal_snapshot_never_rewrites_a_pflash_loader(self): # libvirt decides; the definition is not switched to rom. inst = wvmInstance.__new__(wvmInstance) + inst.instance = MagicMock(**{"snapshotListNames.return_value": []}) # no snapshots yet inst.get_status = MagicMock(return_value=5) inst._XMLDesc = MagicMock( return_value="/usr/share/OVMF/OVMF.fd"