From 7fc55537e5df1147876920423cff81810e9b6ae5 Mon Sep 17 00:00:00 2001 From: catborise Date: Fri, 30 Oct 2026 21:40:00 +0300 Subject: [PATCH] feat(instances): external snapshots first, internal ones a fallback The Snapshots tab had three sub-tabs: internal, external and the lists, with the two kinds side by side as equals. The owner set the direction: external snapshots are the way; internal ones pause the VM while its memory is saved and can take long, so they stay only as a fallback for a VM that cannot take an external one. The tab now has the external snapshot form on top, its name and description on one line, with its memory and freeze options; the internal snapshot under a folded "fallback" line with its warning; and one list of all snapshots, newest first, each on one line with its name, description, kind and date, and Revert and Delete. Codex reviewed snapshots end to end; these are fixed: - The pause and memory lines follow the VM's state and the kind. - An internal snapshot is reverted to on a shut-off VM only, on the server too; the tab alone held that rule. - A paused VM's revert says it comes back paused, not that it starts. - An external snapshot waits, with the reason, while snapshots newer than the one reverted to exist; the server refused it unexplained. - Deleting an external snapshot says its files are merged with the disks; "into the next one" was wrong for the last one. - The description has no 45-character limit only the browser kept. - Freezing is not shown ticked where it cannot be offered, and unticking memory gives back the earlier freeze choice. - Internal create, revert and delete refuse a saved state or a running disk job, as external ones do. --- instances/models.py | 10 +- .../templates/instances/snapshots_tab.html | 286 +++++++----------- instances/tests/test_snapshot_views.py | 41 ++- instances/views.py | 4 +- vrtManager/instance.py | 14 +- vrtManager/tests/test_external_snapshots.py | 21 ++ vrtManager/tests/test_xml_safety.py | 13 +- 7 files changed, 186 insertions(+), 203 deletions(-) diff --git a/instances/models.py b/instances/models.py index 2c8ae0ce..91370ce9 100644 --- a/instances/models.py +++ b/instances/models.py @@ -296,12 +296,20 @@ class Instance(models.Model): return self.proxy.get_video_models(self.arch, self.machine) def _snapshots(self, flag): - return sorted(self.proxy.get_snapshot(flag), reverse=True, key=lambda k: k["date"]) + external = flag == VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL + return sorted(({**snap, "external": external} for snap in self.proxy.get_snapshot(flag)), + reverse=True, key=lambda k: k["date"]) @cached_property def snapshots(self) -> list[dict]: + """Both kinds, the newest first; "external" tells them apart.""" return sorted(self.internal_snapshots + self.external_snapshots, reverse=True, key=lambda k: k["date"]) + @cached_property + def newer_than_current_snapshot(self) -> bool: + """Snapshots newer than the one reverted to: an external snapshot waits until they are deleted.""" + return any(snap["current"] and snap["children"] for snap in self.external_snapshots) + @cached_property def internal_snapshots(self) -> list[dict]: return self._snapshots(VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL) diff --git a/instances/templates/instances/snapshots_tab.html b/instances/templates/instances/snapshots_tab.html index 46e28f68..f9a22e1f 100755 --- a/instances/templates/instances/snapshots_tab.html +++ b/instances/templates/instances/snapshots_tab.html @@ -1,196 +1,124 @@ {% load i18n %} {% load tz %} {% get_current_timezone as TIME_ZONE %} -{% load bootstrap_icons %} {% load operations %} +{% load vm_states %}
{% with busy=instance.snapshot_jobs held=instance.held_by_job %} - + {% vm_state instance.status as state %}
-
-
    - - - -
-
- -
- {% if busy %} -
- {% trans "A disk job is running on this VM; snapshot actions are unavailable until it ends." %} -
- {% 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." %}

-

{% trans "The VM is paused while its memory is saved." %}

- {% else %} -

{% trans "Create an internal snapshot" %}

+
+

{% trans "Snapshots" %} · {{ state.0 }}

+ {% if busy %} +
{% trans "A disk job is running on this VM; snapshot actions are unavailable until it ends." %}
{% endif %} -
- {% csrf_token %}{% submission_key %} -
- - | - - {% if instance.external_snapshots or busy or held %} - - {% else %} - - {% endif %} + {# an external snapshot is the way: each disk goes on in a new file, the VM keeps running #} + {% csrf_token %}{% submission_key %} +
+
+ + +
+
+ + +
+
+
- -
-
-
-

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

-
- {% csrf_token %}{% submission_key %} -
- - | - - {% if instance.internal_snapshots or busy or held %} - - {% else %} - - {% endif %}
-
- - -
-
- {% if instance.status == 1 and instance.guest_agent_ready %} +
+
+ + +
+
+ {% if instance.status == 1 and instance.guest_agent_ready %} - {% else %} - - {% endif %} - + {% else %} + + {% endif %} + +
-

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

+

+ {% if instance.internal_snapshots %}{% trans "The VM has internal snapshots: delete them before taking an external one." %} + {% elif instance.newer_than_current_snapshot %}{% trans "Snapshots newer than the one reverted to exist: delete them before taking a new one." %} + {% else %}{% trans "Each disk goes on in a new file next to it; the VM keeps running." %} + {% if instance.status == 1 %}{% trans "With memory, the VM pauses while its memory is saved." %}{% endif %} + {% if instance.status != 1 %}{% trans "Memory and frozen file systems are for a running VM." %}{% elif not instance.guest_agent_ready %}{% trans "Freezing needs the guest agent, which is not connected." %}{% endif %}{% endif %} +

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

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

- {% if instance.internal_snapshots %} -
{% trans "Internal" %}
-
- - - - - - - - - - - {% for snap in instance.internal_snapshots %} - - - - - - - - {% endfor %} - -
{% blocktrans %}Date ({{ TIME_ZONE }}){% endblocktrans %}{% trans "Name" %}{% trans "Description" %}{% trans "Action" %}
{{ snap.date|date:"Y-m-d H:i:s" }}{{ snap.name }}{{ snap.description }} -
- {% csrf_token %}{% submission_key %} - - {% if instance.status == 5 %} - - {% else %} - - {% endif %} -
- 		-
{% csrf_token %} - - -
-
-
- {% endif %} - {% if instance.external_snapshots %} -
{% trans "External" %}
-
- - - - - - - - - - - {% for ext_snap in instance.external_snapshots %} - - - - - - - - {% endfor %} - -
{% blocktrans %}Date ({{ TIME_ZONE }}){% endblocktrans %}{% trans "Name" %}{% trans "Description" %}{% trans "Action" %}
{{ ext_snap.date|date:"Y-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 %}{% submission_key %} - - -
- 		-
{% csrf_token %}{% submission_key %} - - -
-
-
- {% endif %} +
+ {% trans "Internal snapshot: a fallback for a VM that cannot take an external one" %} +
{% csrf_token %}{% submission_key %} +

+ {% if instance.status == 1 %}{% trans "The disks and memory go into the disk files. The VM does not respond while its memory is saved, which can take long: avoid it in production." %} + {% elif instance.status == 3 %}{% trans "The disks and memory go into the disk files; the paused VM stays paused." %} + {% else %}{% trans "The disks go into their files." %}{% endif %} +

+
+
+ + +
+
+ + +
+
+ +
+
+ {% if instance.external_snapshots %}

{% trans "The VM has external snapshots: the two kinds do not mix." %}

{% endif %} +
+
+ {% if instance.snapshots %} +
    + {% for snap in instance.snapshots %} +
  • +
    + {{ snap.name }}{% if snap.description %} · {{ snap.description }}{% endif %} + · {% if snap.external %}{% trans "external" %}{% if snap.memory %}, {% trans "with memory" %}{% endif %}{% else %}{% trans "internal" %}{% endif %}{% if snap.current %}, {% trans "current" %}{% endif %} · {{ snap.date|date:"Y-m-d H:i" }} +
    + {% if snap.external %} +
    {% csrf_token %}{% submission_key %} + + +
    +
    {% csrf_token %}{% submission_key %} + + +
    + {% else %} +
    {% csrf_token %}{% submission_key %} + + +
    +
    {% csrf_token %} + + +
    + {% endif %} +
    • + {% endfor %} +
+ {% if instance.internal_snapshots and instance.status != 5 %}

{% trans "Shut the VM down to revert to an internal snapshot." %}

{% endif %} {% else %} -

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

+

{% trans "The VM has no snapshot." %}

{% endif %} -
-
+

{% blocktrans %}Times in {{ TIME_ZONE }}. A VM with snapshots cannot be migrated.{% endblocktrans %}

+
{% endwith %} -
+
diff --git a/instances/tests/test_snapshot_views.py b/instances/tests/test_snapshot_views.py index c71460d7..e514795d 100644 --- a/instances/tests/test_snapshot_views.py +++ b/instances/tests/test_snapshot_views.py @@ -126,7 +126,7 @@ class SnapshotTabTestCase(TestCase): def snap(self, name, **kw): return {"name": name, "date": datetime(2026, 1, 1), "description": "", "memory": False, - "current": False, "detaches": [], **kw} + "current": False, "children": 0, "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"}])], @@ -158,11 +158,11 @@ class SnapshotTabTestCase(TestCase): self.assertEqual("powered off" in button.get("data-confirm"), restarts) def test_quiesce_is_checked_and_offered_with_a_running_vm_and_its_agent(self): + # where it cannot be offered it is not shown ticked either for status, agent, offered in ((1, True, True), (1, False, False), (3, True, False), (5, True, False)): with self.subTest(status=status, agent=agent): [box] = self.page(status=status, agent=agent).xpath("//input[@name='quiesce']") - self.assertIsNotNone(box.get("checked")) - self.assertEqual(box.get("disabled") is None, offered) + self.assertEqual((box.get("checked") is not None, box.get("disabled") is None), (offered, offered)) def test_memory_checkbox_only_for_a_running_vm(self): checkbox = "//div[@id='snapshots']//input[@name='memory']" # the Resize tab has a memory field too @@ -186,15 +186,19 @@ class SnapshotTabTestCase(TestCase): return Claim.objects.create(operation=op, resource=key, mode=Claim.EXCLUSIVE) def test_an_active_job_disables_actions_before_libvirt_has_a_block_job(self): - # the button each case leaves enabled, so only the job can disable it - cases = (("none", {}, ("takesnapshot", "takeextsnapshot")), - ("internal only", {"internal": [self.snap("i")]}, ("takesnapshot", "managesnapshot")), - ("external only", {"external": [self.snap("e")]}, ("takeextsnapshot", "managesnapshot"))) + # the buttons each case leaves enabled, so only the job can disable them + def form(name): + return reverse(f"instances:{name}", args=[self.instance.id]) + + cases = (("none", {}, ("create_external_snapshot", "snapshot")), + ("internal only", {"internal": [self.snap("i")]}, ("snapshot", "revert_snapshot", "delete_snapshot")), + ("external only", {"external": [self.snap("e")]}, + ("create_external_snapshot", "revert_external_snapshot", "delete_external_snapshot"))) buttons = "//div[@id='snapshots']//button[@type='submit']" - def enabled(doc, panes): - return [bool(doc.xpath(f"//div[@id='{pane}']//button[@type='submit' and not(@disabled)]")) - for pane in panes] + def enabled(doc, forms): + return [bool(doc.xpath("//form[@action=$a]//button[@type='submit' and not(@disabled)]", a=form(name))) + for name in forms] for name, lists, panes in cases: with self.subTest(name): @@ -213,7 +217,16 @@ class SnapshotTabTestCase(TestCase): self.assertTrue(doc.xpath("//button[@name='revert_snapshot']")) self.assertFalse(doc.xpath("//div[@id='snapshots']//button[@type='submit'][not(@disabled)]")) - 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"]) + def test_one_list_newest_first_each_with_its_kind(self): + doc = self.page(internal=[self.snap("i")], external=[self.snap("e", date=datetime(2026, 2, 1), memory=True)]) + rows = [" ".join(li.text_content().split()) for li in doc.xpath("//ul[@id='snapshot-list']/li")] + self.assertEqual([row.split(" ")[0] for row in rows], ["e", "i"]) + self.assertIn("external, with memory", rows[0]) + self.assertIn("internal", rows[1]) + + def test_an_external_snapshot_waits_for_newer_ones_after_a_revert(self): + create = "//form[@action=$a]//button[@type='submit']" + url = reverse("instances:create_external_snapshot", args=[self.instance.id]) + doc = self.page(external=[self.snap("old", current=True, children=1), self.snap("new")]) + self.assertIsNotNone(doc.xpath(create, a=url)[0].get("disabled")) + self.assertIn("Snapshots newer than the one reverted to exist", doc.text_content()) diff --git a/instances/views.py b/instances/views.py index 7bf69181..d3fa90ff 100755 --- a/instances/views.py +++ b/instances/views.py @@ -1545,7 +1545,7 @@ def delete_snapshot(request, pk): 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") + return _back(request, pk, "snapshots") @require_POST @@ -1581,7 +1581,7 @@ def _snapshot_job(request, pk, kind, params): messages.error(request, error_text(request, err)) else: jobs.remember_job(request, op) - return _back(request, pk, "managesnapshot") + return _back(request, pk, "snapshots") @require_POST diff --git a/vrtManager/instance.py b/vrtManager/instance.py index 381efc59..0fce043b 100644 --- a/vrtManager/instance.py +++ b/vrtManager/instance.py @@ -1771,6 +1771,7 @@ class wvmInstance(wvmConnect): self.instance.snapshotCreateXML(xml, flag) def create_snapshot(self, name, desc=None): + self._check_snapshot_state() 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" @@ -2071,6 +2072,7 @@ class wvmInstance(wvmConnect): "description": tree.findtext("description") or "", "memory": tree.find("memory[@snapshot='external']") is not None, "current": bool(snap.isCurrent(0)), + "children": snap.numChildren(0), "detaches": [{"dev": dev, "path": path} for dev, path in detaches.items()], } ) @@ -2222,18 +2224,24 @@ class wvmInstance(wvmConnect): 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.""" + """Refusals shared by snapshot create, revert and delete, internal and + external. 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): + self._check_snapshot_state() self._snapshot(snapshot, external=False).delete(0) def snapshot_revert(self, snapshot): + """An internal snapshot is reverted to on a shut-off VM; one of a + running VM starts it again as it was then.""" + if self.get_status() != 5: + raise util.OperationError("Shut the VM down to revert to an internal snapshot") + self._check_snapshot_state() self.instance.revertToSnapshot(self._snapshot(snapshot, external=False), 0) def get_managed_save_image(self): diff --git a/vrtManager/tests/test_external_snapshots.py b/vrtManager/tests/test_external_snapshots.py index 9881f6f8..0e5c2d72 100644 --- a/vrtManager/tests/test_external_snapshots.py +++ b/vrtManager/tests/test_external_snapshots.py @@ -236,6 +236,27 @@ class SnapshotStateTestCase(unittest.TestCase): def test_a_clean_vm_passes(self): proxy()._check_snapshot_state() + def test_internal_snapshots_are_refused_alike(self): + # a saved state or a running job refuses internal create, revert and delete too + for state in ({"managed_save": True}, {"jobs": {"vda": {"cur": 1, "end": 2}}}): + for name, act in (("create", lambda p: p.create_snapshot("s")), + ("revert", lambda p: p.snapshot_revert("i")), + ("delete", lambda p: p.snapshot_delete("i"))): + with self.subTest(name, **{key: True for key in state}): + p = proxy(status=5, internal=[fake_snap("i")], **state) + with self.assertRaises(util.OperationError): + act(p) + p.instance.revertToSnapshot.assert_not_called() + p._snapshotCreateXML.assert_not_called() + + def test_an_internal_snapshot_is_reverted_to_on_a_shut_off_vm(self): + for status in (1, 3): + with self.subTest(status=status), self.assertRaisesRegex(util.OperationError, "Shut the VM down"): + proxy(status=status, internal=[fake_snap("i")]).snapshot_revert("i") + p = proxy(status=5, internal=[fake_snap("i")]) + p.snapshot_revert("i") + p.instance.revertToSnapshot.assert_called_once() + DISK_ONLY_ATOMIC = libvirt.VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY | libvirt.VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC diff --git a/vrtManager/tests/test_xml_safety.py b/vrtManager/tests/test_xml_safety.py index 9cf7a40b..b41b4d06 100644 --- a/vrtManager/tests/test_xml_safety.py +++ b/vrtManager/tests/test_xml_safety.py @@ -57,7 +57,8 @@ class TestXmlSafety(unittest.TestCase): def test_snapshot_xml_payload_is_escaped(self): inst = wvmInstance.__new__(wvmInstance) - inst.instance = MagicMock(**{"snapshotListNames.return_value": []}) # no snapshots yet + inst.instance = MagicMock(**{"snapshotListNames.return_value": [], "hasManagedSaveImage.return_value": 0}) # no snapshots yet + inst.get_snapshot_jobs = MagicMock(return_value=[]) inst.get_status = MagicMock(return_value=5) # shutoff inst._XMLDesc = MagicMock(return_value="") inst._snapshotCreateXML = MagicMock(return_value=None) @@ -76,7 +77,8 @@ class TestXmlSafety(unittest.TestCase): def test_original_rom_loader_preserved(self): inst = wvmInstance.__new__(wvmInstance) - inst.instance = MagicMock(**{"snapshotListNames.return_value": []}) # no snapshots yet + inst.instance = MagicMock(**{"snapshotListNames.return_value": [], "hasManagedSaveImage.return_value": 0}) # no snapshots yet + inst.get_snapshot_jobs = MagicMock(return_value=[]) inst._XMLDesc = MagicMock(return_value="/usr/share/rom.bin") inst._defineXML = MagicMock() inst._snapshotCreateXML = MagicMock(return_value=None) @@ -87,7 +89,9 @@ class TestXmlSafety(unittest.TestCase): def test_snapshot_revert_rom_loader_preserved(self): inst = wvmInstance.__new__(wvmInstance) - inst.instance = MagicMock(**{"snapshotListNames.return_value": ["snap_rom"]}) + inst.instance = MagicMock(**{"snapshotListNames.return_value": ["snap_rom"], "hasManagedSaveImage.return_value": 0}) + inst.get_snapshot_jobs = MagicMock(return_value=[]) + inst.get_status = MagicMock(return_value=5) # an internal snapshot is reverted to on a shut-off VM mock_snap = MagicMock() inst.instance.snapshotLookupByName = MagicMock(return_value=mock_snap) inst.instance.revertToSnapshot = MagicMock() @@ -102,7 +106,8 @@ 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.instance = MagicMock(**{"snapshotListNames.return_value": [], "hasManagedSaveImage.return_value": 0}) # no snapshots yet + inst.get_snapshot_jobs = MagicMock(return_value=[]) inst.get_status = MagicMock(return_value=5) inst._XMLDesc = MagicMock( return_value="/usr/share/OVMF/OVMF.fd"