From 6f810434382b00da25d44c10a8356ce453b16910 Mon Sep 17 00:00:00 2001 From: catborise Date: Thu, 15 Oct 2026 09:44:22 +0300 Subject: [PATCH] feat(instances): snapshot create runs as a job --- instances/jobs.py | 132 +++++++++++++++--- .../templates/instances/snapshots_tab.html | 10 +- instances/tests/test_error_handling.py | 10 +- instances/tests/test_page_controls.py | 9 +- instances/tests/test_snapshot_jobs.py | 71 ++++++++++ instances/tests/test_snapshot_views.py | 18 +-- instances/tests/test_views.py | 18 +-- instances/views.py | 72 +++------- 8 files changed, 230 insertions(+), 110 deletions(-) create mode 100644 instances/tests/test_snapshot_jobs.py diff --git a/instances/jobs.py b/instances/jobs.py index 9a4b6bb0..0ec8214b 100644 --- a/instances/jobs.py +++ b/instances/jobs.py @@ -1,12 +1,13 @@ """Jobs of the instances app (operations framework, spec section 5).""" +import math import re from django.core.exceptions import PermissionDenied from django.utils.translation import gettext_noop from django.utils.translation import gettext_noop as _ -from libvirt import (VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE, VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL, VIR_ERR_NO_DOMAIN, - libvirtError) +from libvirt import (VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE, VIR_DOMAIN_JOB_NONE, VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL, + VIR_ERR_NO_DOMAIN, libvirtError) from vrtManager import util from vrtManager.connection import wvmConnect from vrtManager.create import wvmCreate @@ -43,6 +44,63 @@ def _instance(pk): return Instance.objects.select_related("compute").get(pk=pk) +def _vm_plan(params): + """A job on one VM: the VM exclusive, its compute shared.""" + instance = _instance(params["instance"]) + return Plan( + claims={claims.compute_key(instance.compute_id): Claim.SHARED, + claims.instance_key(instance.uuid): Claim.EXCLUSIVE}, + resource=claims.instance_key(instance.uuid), + label=f"{instance.compute.name}/{instance.name}", + intent={"compute": instance.compute_id}, + ) + + +def _checked_proxy(op): + """In the worker: the user may still change the VM, and it did not move.""" + instance = _authorized(op.user, op.params["instance"]) + if instance.compute_id != op.intent["compute"]: + raise util.OperationError("The VM moved to another compute after the job was queued") + return _proxy(instance) + + +def _recovery_proxy(op): + """The VM for recovery; None when it left the compute the job ran on.""" + instance = Instance.objects.select_related("compute").filter(pk=op.params["instance"]).first() + if instance is None or instance.compute_id != op.intent.get("compute"): + return None + try: + return _proxy(instance) + except libvirtError as err: + if err.get_error_code() == VIR_ERR_NO_DOMAIN: + return None + raise # unreachable: recovery waits and retries + + +def _libvirt_busy(proxy): + """libvirt still runs a job on the VM that the worker no longer waits for.""" + return proxy.instance.isActive() and proxy.instance.jobInfo()[0] != VIR_DOMAIN_JOB_NONE + + +def _log(op, state, message): + compute, _sep, vm = op.resource_label.rpartition("/") # a VM name has no "/" + return compute, vm, message if state == Operation.SUCCEEDED else f"{message} ({state})" + + +def _snapshot_free_space(): + """The free space each storage pool keeps for a snapshot, from the + Settings page; an invalid value leaves the backend's default.""" + values = {} + for key, argument in (("SNAPSHOT_FREE_SPACE_PERCENT", "free_percent"), ("SNAPSHOT_FREE_SPACE_MIN_GIB", "free_min_gib")): + try: + value = float(getattr(app_settings, key)) + except (AttributeError, TypeError, ValueError): + continue + if math.isfinite(value) and value >= 0: + values[argument] = value + return values + + CLONE_POST_KEY_RE = re.compile(r"^(clone-net-mac-\d+|disk-[a-z0-9]+|meta-[a-z0-9]+)$") CLONE_FIELDS = ("name", "clone-title", "clone-description") @@ -180,20 +238,10 @@ class DeleteExternalSnapshot(Kind): raise util.OperationError(f"The VM has no external snapshot named {params['name']}") def plan(self, user, params): - instance = _instance(params["instance"]) - return Plan( - claims={claims.compute_key(instance.compute_id): Claim.SHARED, - claims.instance_key(instance.uuid): Claim.EXCLUSIVE}, - resource=claims.instance_key(instance.uuid), - label=f"{instance.compute.name}/{instance.name}", - intent={"compute": instance.compute_id}, - ) + return _vm_plan(params) def precheck(self, op, ctx): - instance = _authorized(op.user, op.params["instance"]) - if instance.compute_id != op.intent["compute"]: - raise util.OperationError("The VM moved to another compute after the job was queued") - proxy = _proxy(instance) + proxy = _checked_proxy(op) proxy._snapshot(op.params["name"], external=True) proxy._check_snapshot_state() @@ -234,9 +282,59 @@ class DeleteExternalSnapshot(Kind): "changed; check the VM before deleting the snapshot again or reverting to it.") def log_entry(self, op, state): - compute, _sep, vm = op.resource_label.rpartition("/") # a VM name has no "/" - message = f"Delete external snapshot: {op.params['name']}" - return compute, vm, message if state == Operation.SUCCEEDED else f"{message} ({state})" + return _log(op, state, f"Delete external snapshot: {op.params['name']}") + + +GONE = "The VM is no longer on the compute the job ran on" + + +@register +class CreateSnapshot(Kind): + name = "snapshot.create" + label = gettext_noop("Take snapshot") + time_limit = 6 * 3600 # a snapshot cannot be stopped: over time is only noted + + def normalize(self, raw): + return {"instance": int(raw["instance"]), "name": str(raw.get("name", "")), + "description": str(raw.get("description", "")), "external": bool(raw.get("external")), + "memory": bool(raw.get("memory")), "quiesce": bool(raw.get("quiesce"))} + + def validate(self, user, params): + # recovery counts a listed snapshot as taken by the job + if params["name"] in _proxy(_authorized(user, params["instance"])).instance.snapshotListNames(0): + raise util.OperationError(f"A snapshot named {params['name']} already exists") + + def plan(self, user, params): + return _vm_plan(params) + + def precheck(self, op, ctx): + _checked_proxy(op) + + def run(self, op, ctx): + p = op.params + proxy = _proxy(_instance(p["instance"])) + ctx.record_intent() + ctx.set_phase(gettext_noop("Taking the snapshot")) + if p["external"]: + proxy.create_external_snapshot(p["name"], p["description"], p["memory"], p["quiesce"], + **_snapshot_free_space()) + else: + proxy.create_snapshot(p["name"], p["description"]) + return {} + + def reconcile(self, op): + proxy = _recovery_proxy(op) + if proxy is None: + return Outcome(Operation.UNCERTAIN, error=GONE) + if _libvirt_busy(proxy): + return Outcome(Operation.RUNNING, phase=gettext_noop("libvirt is still taking the snapshot")) + if op.params["name"] in proxy.instance.snapshotListNames(0): + return Outcome(Operation.SUCCEEDED) + return Outcome(Operation.FAILED, error="The snapshot was not taken") + + def log_entry(self, op, state): + kind = "external snapshot" if op.params["external"] else "snapshot" + return _log(op, state, f"Create {kind}: {op.params['name']}") @register diff --git a/instances/templates/instances/snapshots_tab.html b/instances/templates/instances/snapshots_tab.html index 51707f6d..b77df035 100755 --- a/instances/templates/instances/snapshots_tab.html +++ b/instances/templates/instances/snapshots_tab.html @@ -31,12 +31,12 @@
{% 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 "Live snapshot could cause server timeout and instance might be paused!!!" %}

+

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

{% else %}

{% trans "Create an internal snapshot" %}

{% endif %}
- {% csrf_token %} + {% csrf_token %}{% submission_key %}
| @@ -44,7 +44,7 @@ {% if instance.external_snapshots or busy or held %} {% else %} - + {% endif %}
@@ -53,7 +53,7 @@

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

- {% csrf_token %} + {% csrf_token %}{% submission_key %}
| @@ -61,7 +61,7 @@ {% if instance.internal_snapshots or busy or held %} {% else %} - + {% endif %}
diff --git a/instances/tests/test_error_handling.py b/instances/tests/test_error_handling.py index 13422339..3a167a65 100644 --- a/instances/tests/test_error_handling.py +++ b/instances/tests/test_error_handling.py @@ -18,13 +18,16 @@ EXTERNAL = "https://evil.example/instances/1/" def back_views(): - """URL names of the instance views that return with _back().""" + """URL names of the instance views that return with _back(), also through _snapshot_job().""" names = [] for pattern in get_resolver().url_patterns: if getattr(pattern, "namespace", None) != "instances": continue for sub in pattern.url_patterns: - if isinstance(sub, URLPattern) and "_back(request, pk" in inspect.getsource(inspect.unwrap(sub.callback)): + if isinstance(sub, URLPattern) and any( + call in inspect.getsource(inspect.unwrap(sub.callback)) + for call in ("_back(request, pk", "_snapshot_job(request, pk") + ): names.append(sub.name) return names @@ -57,7 +60,8 @@ class ErrorHandlingTestCase(TestCase): self.assertGreaterEqual(len(names), 30) for name in names: for referer in (None, EXTERNAL): - with self.subTest(view=name, referer=referer), patch("instances.models.wvmInstance"): + with self.subTest(view=name, referer=referer), patch("instances.models.wvmInstance"), \ + patch("instances.jobs.wvmInstance"): headers = {"HTTP_REFERER": referer} if referer else {} data = self.DATA[name](self) if name in self.DATA else {} response = self.client.post(self.url(name), data, **headers) diff --git a/instances/tests/test_page_controls.py b/instances/tests/test_page_controls.py index f4a63fd1..8deb410a 100644 --- a/instances/tests/test_page_controls.py +++ b/instances/tests/test_page_controls.py @@ -23,6 +23,7 @@ from accounts.models import UserInstance from appsettings.models import AppSettings from computes.models import Compute from instances.models import Instance +from operations.models import Operation CONSOLE = {"console", "vv file", "vdi"} POWER = {"power"} @@ -231,11 +232,13 @@ class TemplateSnapshotRuleTestCase(PageControls, TestCase): def snapshot(self, user): self.client.force_login(user) - with patch("instances.models.wvmInstance") as mock_wvm: + with patch("instances.models.wvmInstance"), patch("instances.jobs.wvmInstance") as mock_wvm: + mock_wvm.return_value.instance.snapshotListNames.return_value = [] self.client.post( - reverse("instances:snapshot", args=[self.instance.id]), {"name": "s1", "description": ""} + reverse("instances:snapshot", args=[self.instance.id]), + {"name": "s1", "description": "", "submission_key": "key-snap0001"}, ) - return mock_wvm.return_value.create_snapshot.called + return Operation.objects.filter(kind="snapshot.create").exists() def test_only_staff_owners_snapshot_a_template(self): perms = ("snapshot_instances",) diff --git a/instances/tests/test_snapshot_jobs.py b/instances/tests/test_snapshot_jobs.py new file mode 100644 index 00000000..51db43f2 --- /dev/null +++ b/instances/tests/test_snapshot_jobs.py @@ -0,0 +1,71 @@ +"""Snapshot create and revert run as jobs: the page queues them, the worker calls libvirt.""" + +from types import SimpleNamespace +from unittest.mock import patch + +from django.contrib.auth import get_user_model +from django.test import TransactionTestCase +from django.urls import reverse + +from appsettings.settings import get_settings +from computes.models import Compute +from instances.jobs import CreateSnapshot +from instances.models import Instance +from operations.jobs import Context, get_kind +from operations.models import Claim, Operation, acquire + + +class SnapshotJobsTestCase(TransactionTestCase): + def setUp(self): + get_settings() + compute = Compute.objects.create(name="snap-c", hostname="192.0.2.40", login="root", password="", type=1) + self.instance = Instance.objects.create(compute=compute, name="snap-vm", + uuid="21212121-3434-5656-7878-909090909090") + self.client.force_login(get_user_model().objects.create_superuser("snap_admin", "a@example.com", "pw")) + patcher = patch("instances.jobs.wvmInstance") + self.proxy = patcher.start().return_value + self.addCleanup(patcher.stop) + self.proxy.instance.snapshotListNames.return_value = [] + self.proxy.instance.isActive.return_value = False + self.proxy.instance.hasCurrentSnapshot.return_value = 0 + + def queue_and_run(self, view, data): + """Post the form; nothing runs in the request; then run the job as the worker does.""" + self.client.post(reverse(f"instances:{view}", args=[self.instance.pk]), + {**data, "submission_key": "key-snap0001"}) + op = Operation.objects.get() + self.assertIn((f"instance:{self.instance.uuid}", "exclusive"), + set(Claim.objects.values_list("resource", "mode"))) + self.assertFalse(self.proxy.create_snapshot.called or self.proxy.create_external_snapshot.called) + acquire("wvc-test:1:a", "wvc-test") + Operation.objects.filter(pk=op.pk).update(state=Operation.RUNNING, worker="wvc-test:1:a") + op.refresh_from_db() + ctx = Context(op, SimpleNamespace(identity="wvc-test:1:a")) + kind = get_kind(op.kind) + kind.precheck(op, ctx) + return op, kind.run(op, ctx) + + def test_an_external_snapshot_is_taken_by_the_worker(self): + op, _ = self.queue_and_run("create_external_snapshot", {"name": "a", "description": "d", "memory": "on"}) + self.assertEqual(op.kind, "snapshot.create") + self.assertEqual(self.proxy.create_external_snapshot.call_args.args, ("a", "d", True, False)) + + def test_an_internal_snapshot_is_taken_by_the_worker(self): + self.queue_and_run("snapshot", {"name": "b"}) + self.proxy.create_snapshot.assert_called_once_with("b", "") + + def test_a_taken_name_is_refused_on_the_page(self): + self.proxy.instance.snapshotListNames.return_value = ["a"] + self.client.post(reverse("instances:snapshot", args=[self.instance.pk]), + {"name": "a", "submission_key": "key-snap0001"}) + self.assertFalse(Operation.objects.exists()) + + def test_create_recovery(self): + op = Operation(params={"instance": self.instance.pk, "name": "a", "external": True}, + intent={"compute": self.instance.compute_id}) + self.assertEqual(CreateSnapshot().reconcile(op).state, Operation.FAILED) + self.proxy.instance.snapshotListNames.return_value = ["a"] + self.assertEqual(CreateSnapshot().reconcile(op).state, Operation.SUCCEEDED) + self.proxy.instance.isActive.return_value = True + self.proxy.instance.jobInfo.return_value = [2] + [0] * 11 # VIR_DOMAIN_JOB_UNBOUNDED + self.assertEqual(CreateSnapshot().reconcile(op).state, Operation.RUNNING) diff --git a/instances/tests/test_snapshot_views.py b/instances/tests/test_snapshot_views.py index fc70f243..b38a1537 100644 --- a/instances/tests/test_snapshot_views.py +++ b/instances/tests/test_snapshot_views.py @@ -14,7 +14,9 @@ from lxml import html as lxml_html from vrtManager import util from appsettings.models import AppSettings +from appsettings.settings import get_settings from computes.models import Compute +from instances.jobs import _snapshot_free_space from instances.models import Instance from operations.models import Claim, Operation @@ -32,11 +34,9 @@ class SnapshotViewsTestCase(TestCase): 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, "revert_snapshot": proxy.snapshot_revert, "delete_snapshot": proxy.snapshot_delete, - "snapshot": proxy.create_snapshot, }[view] method.return_value = list(left) # files revert and delete left behind if error: @@ -50,13 +50,6 @@ class SnapshotViewsTestCase(TestCase): 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_memory_quiesce_and_free_space(self): - free = {"free_percent": 10.0, "free_min_gib": 1.0} # the Settings page defaults - _, method, _ = self.post("create_external_snapshot", {"name": "a", "description": "d", "memory": "on"}) - method.assert_called_once_with("a", "d", True, False, **free) - _, method, _ = self.post("create_external_snapshot", {"name": "a", "description": "", "quiesce": "on"}) - method.assert_called_once_with("a", "", False, True, **free) - def test_the_free_space_comes_from_the_settings_page(self): for percent, minimum, expected in (("20", "0.5", {"free_percent": 20.0, "free_min_gib": 0.5}), ("0", "0", {"free_percent": 0.0, "free_min_gib": 0.0}), @@ -64,8 +57,8 @@ class SnapshotViewsTestCase(TestCase): with self.subTest(percent=percent, minimum=minimum): AppSettings.objects.filter(key="SNAPSHOT_FREE_SPACE_PERCENT").update(value=percent) AppSettings.objects.filter(key="SNAPSHOT_FREE_SPACE_MIN_GIB").update(value=minimum) - _, method, _ = self.post("create_external_snapshot", {"name": "a"}) - method.assert_called_once_with("a", "", False, False, **expected) + get_settings() + self.assertEqual(_snapshot_free_space(), expected) def test_revert_calls_the_backend_once_and_leaves_power_alone(self): proxy, method, _ = self.post("revert_external_snapshot", {"name": "a"}) @@ -108,8 +101,7 @@ class SnapshotViewsTestCase(TestCase): self.assertFalse(Operation.objects.exists()) def test_refusals_become_messages(self): - for view in ("create_external_snapshot", "revert_external_snapshot", - "revert_snapshot", "delete_snapshot", "snapshot"): + for view in ("revert_external_snapshot", "revert_snapshot", "delete_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) diff --git a/instances/tests/test_views.py b/instances/tests/test_views.py index d276ef96..d9b7fb7d 100644 --- a/instances/tests/test_views.py +++ b/instances/tests/test_views.py @@ -515,14 +515,7 @@ class InstancesTestCase(TestCase): def test_snapshots(self): self.assertEqual(len(self.instance.snapshots), 0) - response = self.client.post( - reverse("instances:snapshot", args=[self.instance.id]), - { - "name": "test-snapshot", - }, - HTTP_REFERER=reverse("index"), - ) - self.assertEqual(response.status_code, 302) + self.instance.proxy.create_snapshot("test-snapshot") for cached in ("snapshots", "internal_snapshots", "external_snapshots"): self.instance.__dict__.pop(cached, None) @@ -690,14 +683,7 @@ class InstancesTestCase(TestCase): self.assertEqual(clone.proxy.get_net_devices()[0]["mac"], "de:ad:be:ef:de:ad") - response = self.client.post( - reverse("instances:snapshot", args=[clone.id]), - { - "name": "test", - }, - HTTP_REFERER=reverse("index"), - ) - self.assertEqual(response.status_code, 302) + clone.proxy.create_snapshot("test") response = self.client.post( reverse("instances:destroy", args=[clone.id]), diff --git a/instances/views.py b/instances/views.py index 53ebb328..02b74099 100755 --- a/instances/views.py +++ b/instances/views.py @@ -4,7 +4,6 @@ except ImportError: import crypt import functools import json -import math import os import re import socket @@ -1337,22 +1336,10 @@ def mount_iso(request, pk): @require_POST -@serialize_instance_mutation def snapshot(request, pk): - instance = get_instance(request.user, pk, perm_type="change") - if not request.user.has_perm("instances.snapshot_instances"): - raise PermissionDenied - name = request.POST.get("name", "") - desc = request.POST.get("description", "") - try: - instance.proxy.create_snapshot(name, desc) - except libvirtError as err: - # e.g. UEFI firmware whose NVRAM format does not allow internal snapshots - messages.error(request, _("Snapshot was not created: %(err)s") % {"err": error_text(request, err)}) - else: - msg = _("Create snapshot: %(snap)s") % {"snap": name} - addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return _back(request, pk, "managesnapshot") + return _snapshot_job(request, pk, "snapshot.create", + {"name": request.POST.get("name", ""), "description": request.POST.get("description", "")}, + _("The snapshot is being taken; follow it in the job panel.")) @require_POST @@ -1406,38 +1393,13 @@ def _warn_left_behind(request, files): ) -def _snapshot_free_space(): - """The free space each storage pool keeps for a snapshot, from the - Settings page; an invalid value leaves the backend's default.""" - values = {} - for key, argument in (("SNAPSHOT_FREE_SPACE_PERCENT", "free_percent"), ("SNAPSHOT_FREE_SPACE_MIN_GIB", "free_min_gib")): - try: - value = float(getattr(app_settings, key)) - except (AttributeError, TypeError, ValueError): - continue - if math.isfinite(value) and value >= 0: - values[argument] = value - return values - - @require_POST -@serialize_instance_mutation def create_external_snapshot(request, pk): - instance = get_instance(request.user, pk, perm_type="change") - if not request.user.has_perm("instances.snapshot_instances"): - raise PermissionDenied - name = request.POST.get("name", "") - desc = request.POST.get("description", "") - memory = request.POST.get("memory") == "on" - quiesce = request.POST.get("quiesce") == "on" - try: - instance.proxy.create_external_snapshot(name, desc, memory, quiesce, **_snapshot_free_space()) - 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") + return _snapshot_job(request, pk, "snapshot.create", + {"name": request.POST.get("name", ""), "description": request.POST.get("description", ""), + "external": True, "memory": request.POST.get("memory") == "on", + "quiesce": request.POST.get("quiesce") == "on"}, + _("The snapshot is being taken; follow it in the job panel.")) @require_POST @@ -1459,27 +1421,31 @@ def revert_external_snapshot(request, pk): return _back(request, pk, "managesnapshot") -@require_POST -def delete_external_snapshot(request, pk): +def _snapshot_job(request, pk, kind, params, queued): + """Queue a snapshot job on the VM; refusals become messages.""" get_instance(request.user, pk, perm_type="change") if not request.user.has_perm("instances.snapshot_instances"): raise PermissionDenied try: - op = jobs.submit("snapshot.delete", request.user, - {"instance": pk, "name": request.POST.get("name", "")}, - request.POST.get("submission_key", "")) + op = jobs.submit(kind, request.user, {"instance": pk, **params}, request.POST.get("submission_key", "")) except claims.Busy as busy: messages.error(request, busy_message(busy, request.user)) except jobs.SubmissionConflict as err: messages.error(request, str(err)) except libvirtError as err: - messages.error(request, _("Snapshot was not deleted: %(err)s") % {"err": error_text(request, err)}) + messages.error(request, error_text(request, err)) else: - messages.info(request, _("The snapshot is being deleted; follow it in the job panel.")) + messages.info(request, queued) jobs.remember_job(request, op) return _back(request, pk, "managesnapshot") +@require_POST +def delete_external_snapshot(request, pk): + return _snapshot_job(request, pk, "snapshot.delete", {"name": request.POST.get("name", "")}, + _("The snapshot is being deleted; follow it in the job panel.")) + + @require_POST @superuser_only @serialize_instance_mutation