feat(instances): snapshot create runs as a job

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

instances/templates/instances/snapshots_tab.html

@@ -31,12 +31,12 @@
         <div role="tabpanel" class="tab-pane tab-pane-bordered active" id="takesnapshot">
             {% if instance.status != 5 %}
             <p>{% 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." %} </p>
-                <p class="text-danger">{% trans "Live snapshot could cause server timeout and instance might be paused!!!" %}</p>
+                <p class="text-danger">{% trans "The VM is paused while its memory is saved." %}</p>
             {% else %}
                 <p>{% trans "Create an internal snapshot" %}</p>
             {% endif %}
             <form action="{% url 'instances:snapshot' instance.id %}" method="post" role="form" aria-label="Create snapshot form">
-                {% csrf_token %}
+                {% csrf_token %}{% submission_key %}
                     <div class="input-group mb-3">
                         <input type="text" class="form-control form-control-lg" name="name" placeholder="{% trans "Snapshot Name" %}" maxlength="14">
                         <span class="input-group-text">|</span>
@@ -44,7 +44,7 @@
                         {% if instance.external_snapshots or busy or held %}
                             <button type="submit" class="btn btn-lg btn-success float-end" name="snapshot" disabled>{% trans "Take Snapshot" %}</button>
                         {% else %}
-                            <button type="submit" class="btn btn-lg btn-success float-end" name="snapshot" onclick="showPleaseWaitDialog();">{% trans "Take Snapshot" %}</button>
+                            <button type="submit" class="btn btn-lg btn-success float-end" name="snapshot">{% trans "Take Snapshot" %}</button>
                         {% endif %}
                     </div>
             </form>
@@ -53,7 +53,7 @@
         <div role="tabpanel" class="tab-pane tab-pane-bordered" id="takeextsnapshot">
             <p>{% trans "Create an external snapshot. Each disk keeps its state in a new file next to it." %}</p>
             <form action="{% url 'instances:create_external_snapshot' instance.id %}" method="post" role="form" aria-label="Create external snapshot form">
-                {% csrf_token %}
+                {% csrf_token %}{% submission_key %}
                 <div class="input-group mb-3">
                     <input type="text" class="form-control form-control-lg" name="name" placeholder="{% trans "Snapshot Name" %}" maxlength="64" pattern="[A-Za-z0-9_\-][A-Za-z0-9._\-]{0,63}" title="{% trans "Letters, digits, '.', '_' or '-'; not starting with '.'" %}" required>
                     <span class="input-group-text">|</span>
@@ -61,7 +61,7 @@
                     {% if instance.internal_snapshots or busy or held %}
                         <button type="submit" class="btn btn-lg btn-success float-end" name="snapshot" disabled>{% trans "Take Snapshot" %}</button>
                     {% else %}
-                        <button type="submit" class="btn btn-lg btn-success float-end" name="snapshot" onclick="if (this.form.checkValidity()) showPleaseWaitDialog();">{% trans "Take Snapshot" %}</button>
+                        <button type="submit" class="btn btn-lg btn-success float-end" name="snapshot">{% trans "Take Snapshot" %}</button>
                     {% endif %}
                 </div>
                 <div class="form-check mb-3">

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)

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

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)

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)

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]),

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