feat(instances): external snapshot delete runs as a job

instances/jobs.py

@@ -0,0 +1,113 @@
+"""Jobs of the instances app (operations framework, spec section 5)."""
+
+from django.core.exceptions import PermissionDenied
+from django.utils.translation import gettext_noop
+from libvirt import VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL, VIR_ERR_NO_DOMAIN, libvirtError
+from vrtManager import util
+from vrtManager.instance import wvmInstance
+
+from instances.models import Instance
+from operations import claims
+from operations.jobs import Kind, Outcome, Plan, register
+from operations.models import Claim, Operation
+
+
+def _authorized(user, pk):
+    """The instance views' rule: change access and the snapshot permission."""
+    from instances.views import get_instance
+
+    if user is None or not user.is_active:
+        raise PermissionDenied
+    instance = get_instance(user, pk, perm_type="change")
+    if not user.has_perm("instances.snapshot_instances"):
+        raise PermissionDenied
+    return instance
+
+
+def _proxy(instance):
+    c = instance.compute
+    return wvmInstance(c.hostname, c.login, c.password, c.type, instance.name, uuid=instance.uuid)
+
+
+def _instance(pk):
+    return Instance.objects.select_related("compute").get(pk=pk)
+
+
+def _merged(jobs):
+    """(done, total) bytes of the snapshot's block jobs."""
+    return sum(j["cur"] for j in jobs), sum(j["end"] for j in jobs)
+
+
+@register
+class DeleteExternalSnapshot(Kind):
+    name = "snapshot.delete"
+    label = gettext_noop("Delete snapshot")
+    time_limit = 6 * 3600  # a merge cannot be stopped: over time is only noted
+    progress_bytes = True  # libvirt block job cur/end
+
+    def normalize(self, raw):
+        return {"instance": int(raw["instance"]), "name": str(raw.get("name", ""))}
+
+    def validate(self, user, params):
+        instance = _authorized(user, params["instance"])
+        if params["name"] not in _proxy(instance).instance.snapshotListNames(VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL):
+            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},
+        )
+
+    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._snapshot(op.params["name"], external=True)
+        proxy._check_snapshot_state()
+
+    def run(self, op, ctx):
+        proxy = _proxy(_instance(op.params["instance"]))
+        ctx.record_intent(before=proxy.snapshot_files_state())
+        ctx.set_phase(gettext_noop("Merging the snapshot"))
+        return {"left_behind": proxy.delete_external_snapshot(op.params["name"])}
+
+    def progress(self, op):
+        jobs = _proxy(_instance(op.params["instance"])).get_snapshot_jobs()
+        return _merged(jobs) if jobs else None
+
+    def reconcile(self, op):
+        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 Outcome(Operation.UNCERTAIN, error="The VM is no longer on the compute the job ran on")
+        try:
+            proxy = _proxy(instance)
+            listed = op.params["name"] in proxy.instance.snapshotListNames(VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL)
+            jobs = proxy.get_snapshot_jobs()
+        except util.ConnectionFailed:
+            raise  # unreachable: recovery waits and retries
+        except libvirtError as err:  # OperationError is a libvirtError too
+            if err.get_error_code() == VIR_ERR_NO_DOMAIN:
+                return Outcome(Operation.UNCERTAIN, error="The VM is no longer on its compute")
+            raise
+        if jobs:
+            return Outcome(Operation.RUNNING, phase=gettext_noop("libvirt is still merging the snapshot"),
+                           stalled=all(j["cur"] == j["end"] for j in jobs),
+                           progress=_merged(jobs))
+        if not listed:
+            before = op.intent.get("before")  # recorded with effects_started; absent only if run never began
+            left = proxy.left_behind_since(before) if before else []
+            return Outcome(Operation.SUCCEEDED, result={"left_behind": left})
+        return Outcome(Operation.UNCERTAIN,
+                       error="The snapshot is still listed and no merge runs. It cannot be told whether the disks "
+                             "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})"

instances/models.py

@@ -8,6 +8,8 @@ from libvirt import (
     VIR_DOMAIN_XML_SECURE,
     libvirtError,
 )
+from operations import claims
+from operations.models import Claim
 from vrtManager.instance import wvmInstance
 from webvirtcloud.settings import QEMU_CONSOLE_LISTENER_ADDRESSES
 
@@ -281,6 +283,11 @@ class Instance(models.Model):
         except libvirtError:
             return []
 
+    @cached_property
+    def held_by_job(self) -> bool:
+        # a queued job has no libvirt block job yet; a job's claim still holds the VM
+        return Claim.objects.filter(resource=claims.instance_key(self.uuid), operation__isnull=False).exists()
+
     @cached_property
     def inst_xml(self):
         return self.proxy._XMLDesc(VIR_DOMAIN_XML_SECURE)

instances/templates/instances/snapshots_tab.html

@@ -1,7 +1,8 @@
 {% load i18n %}
 {% load bootstrap_icons %}
+{% load operations %}
 <div class="tab-pane" id="snapshots" role="tabpanel">
-    {% with busy=instance.snapshot_jobs %}
+    {% with busy=instance.snapshot_jobs held=instance.held_by_job %}
     <!-- Nav tabs -->
     <ul class="nav nav-tabs" role="tablist" aria-label="Instance snapshot menu">
         <li class="nav-item" role="presentation">
@@ -42,7 +43,7 @@
                         <input type="text" class="form-control form-control-lg" name="name" placeholder="{% trans "Snapshot Name" %}" maxlength="14">
                         <span class="input-group-text">|</span>
                         <input type="text" class="form-control form-control-lg" name="description" placeholder="{% trans "Snapshot Description" %}" maxlength="45">
-                        {% if instance.external_snapshots or busy %}
+                        {% 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>
@@ -59,7 +60,7 @@
                     <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>
                     <input type="text" class="form-control form-control-lg" name="description" placeholder="{% trans "Snapshot Description" %}" maxlength="45">
-                    {% if instance.internal_snapshots or busy %}
+                    {% 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>
@@ -106,7 +107,7 @@
                                             {% csrf_token %}
                                             <input type="hidden" name="name" value="{{ snap.name }}">
                                             {% if instance.status == 5 %}
-                                                <button type="submit" class="btn btn-sm btn-primary" name="revert_snapshot" title="{% trans 'Revert to this Snapshot' %}" onclick="return confirm('{% trans "Are you sure?" %}')" {% if busy %}disabled{% endif %}>
+                                                <button type="submit" class="btn btn-sm btn-primary" name="revert_snapshot" title="{% trans 'Revert to this Snapshot' %}" onclick="return confirm('{% trans "Are you sure?" %}')" {% if busy or held %}disabled{% endif %}>
                                                         <span>{% bs_icon 'download' %} </span>
                                                 </button>
                                             {% else %}
@@ -120,7 +121,7 @@
                                     <td style="width:30px;">
                                         <form action="{% url 'instances:delete_snapshot' instance.id %}" method="post" role="form" aria-label="Delete snapshot form">{% csrf_token %}
                                             <input type="hidden" name="name" value="{{ snap.name }}">
-                                            <button type="submit" class="btn btn-sm btn-danger" title="{% trans 'Delete Snapshot' %}" onclick="return confirm('{% trans "Are you sure?" %}')" {% if busy %}disabled{% endif %}>
+                                            <button type="submit" class="btn btn-sm btn-danger" title="{% trans 'Delete Snapshot' %}" onclick="return confirm('{% trans "Are you sure?" %}')" {% if busy or held %}disabled{% endif %}>
                                                 {% bs_icon 'trash' %}
                                             </button>
                                         </form>
@@ -157,17 +158,17 @@
                                             <input type="hidden" name="name" value="{{ ext_snap.name }}">
                                             <button type="submit" class="btn btn-sm btn-primary" name="revert_external_snapshot" title="{% trans 'Revert to this Snapshot' %}"
                                                     data-confirm="{% trans 'Revert to this snapshot? Changes made since it are lost.' %}{% if not ext_snap.memory %}{% if instance.status == 1 or instance.status == 3 %} {% trans "The VM is powered off and starts again from the snapshot's disks, as after a power cut." %}{% endif %}{% endif %}{% if ext_snap.detaches %} {% trans 'These disks are detached; their writes since the current snapshot are discarded and their files are kept:' %} {% for d in ext_snap.detaches %}{{ d.dev }} ({{ d.path }}){% if not forloop.last %}, {% endif %}{% endfor %}.{% endif %}{% if instance.nvram %} {% trans 'Firmware variables (NVRAM) are not restored.' %}{% endif %}"
-                                                    onclick="return confirm(this.dataset.confirm)" {% if busy %}disabled{% endif %}>
+                                                    onclick="return confirm(this.dataset.confirm)" {% if busy or held %}disabled{% endif %}>
                                                 <span>{% bs_icon 'download' %} </span>
                                             </button>
                                         </form>
                                     </td>
                                     <td style="width:30px;">
-                                        <form action="{% url 'instances:delete_external_snapshot' instance.id %}" method="post" role="form" aria-label="Delete external snapshot form">{% csrf_token %}
+                                        <form action="{% url 'instances:delete_external_snapshot' instance.id %}" method="post" role="form" aria-label="Delete external snapshot form">{% csrf_token %}{% submission_key %}
                                             <input type="hidden" name="name" value="{{ ext_snap.name }}">
                                             <button type="submit" class="btn btn-sm btn-danger" name="delete_external_snapshot" title="{% trans 'Delete Snapshot' %}"
                                                     data-confirm="{% trans 'Delete this snapshot? Its changes are merged into the next one; nothing is lost.' %}"
-                                                    onclick="return confirm(this.dataset.confirm)" {% if busy %}disabled{% endif %}>
+                                                    onclick="return confirm(this.dataset.confirm)" {% if busy or held %}disabled{% endif %}>
                                                 {% bs_icon 'trash' %}
                                             </button>
                                         </form>

instances/tests/test_live_jobs.py

@@ -0,0 +1,92 @@
+"""Live checks for the instance jobs: the worker runs them against libvirt
+and recovery reads the same outcome back. Runs only with TEST_LIBVIRT_HOST
+set (see instances/tests/livetest.py)."""
+
+import os
+import time
+import unittest
+
+from django.contrib.auth import get_user_model
+from django.test import TransactionTestCase
+from vrtManager.connection import connection_manager
+from vrtManager.instance import wvmInstance
+
+from computes.models import Compute
+from instances.jobs import DeleteExternalSnapshot
+from instances.models import Instance
+from operations import jobs
+from operations.models import Operation
+from operations.tests import fakes
+from operations.worker import Worker
+
+from . import livetest
+
+P = livetest.PREFIX
+
+
+def wait_for(predicate, seconds):
+    end = time.monotonic() + seconds
+    while time.monotonic() < end:
+        if predicate():
+            return True
+        time.sleep(0.5)
+    return False
+
+
+class LiveJobsTestCase(TransactionTestCase):
+    @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 setUp(self):
+        host, login, password, conn_type = self.args
+        self.compute = Compute.objects.create(name=P + "compute", hostname=host, login=login, password=password,
+                                              type=conn_type)
+        self.admin = get_user_model().objects.create_superuser("job_live_admin", "a@example.com", "pw")
+
+    def tearDown(self):
+        livetest.cleanup(self.conn)
+
+    def vm(self, name, state="stopped"):
+        base = livetest.create_volume(self.conn, P + name)
+        dom = livetest.define_vm(self.conn, P + name, [base])
+        if state == "running":
+            dom.create()
+        return dom, wvmInstance(*self.args, None, uuid=dom.UUIDString())
+
+    def test_the_worker_deletes_a_snapshot_and_recovery_agrees(self):
+        dom, vm = self.vm("job-del", "running")
+        vm.create_external_snapshot("one")
+        instance = Instance.objects.create(compute=self.compute, name=dom.name(), uuid=dom.UUIDString())
+        op = jobs.submit("snapshot.delete", self.admin, {"instance": instance.pk, "name": "one"}, "key-live0001")
+        worker = Worker(concurrency=1, tick=0.1, heartbeat_every=1, progress_every=1, exit_on_lost_lease=False)
+        thread = fakes.start_worker(worker)
+        try:
+            self.assertTrue(wait_for(lambda: Operation.objects.get(pk=op.pk).state == Operation.SUCCEEDED, 180))
+        finally:
+            fakes.stop_worker(worker, thread)
+        self.assertEqual(dom.snapshotNum(0), 0)
+        Operation.objects.filter(pk=op.pk).update(state=Operation.RUNNING)
+        op.refresh_from_db()
+        self.assertEqual(DeleteExternalSnapshot().reconcile(op).state, Operation.SUCCEEDED)

instances/tests/test_live_snapshots.py

@@ -101,6 +101,11 @@ class LiveExternalSnapshotTestCase(SimpleTestCase):
         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_a_shut_off_vm_without_a_merge_has_no_job(self):
+        # libvirt reports a shut-off VM's merge as an offline block job; none runs here
+        _, vm = self.vm("snap-nojob")
+        self.assertEqual(vm.get_snapshot_jobs(), [])
+
     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")

instances/tests/test_snapshot_delete_job.py

@@ -0,0 +1,212 @@
+"""External snapshot delete is a job: the request queues it; the worker
+deletes through libvirt; recovery follows the reconcile table."""
+
+from datetime import timedelta
+from types import SimpleNamespace
+from unittest.mock import MagicMock, patch
+
+from django.contrib.auth import get_user_model
+from django.test import TransactionTestCase
+from django.urls import reverse
+from django.utils import timezone
+from libvirt import libvirtError
+
+from computes.models import Compute
+from instances.jobs import DeleteExternalSnapshot
+from instances.models import Instance
+from logs.models import Logs
+from operations import jobs
+from operations.jobs import Context, Outcome
+from operations.models import Claim, Operation, acquire
+from operations.worker import housekeeping
+from vrtManager.instance import wvmInstance
+
+
+def proxy(listed=("one",), jobs=(), before=None, left=()):
+    p = MagicMock()
+    p.instance.snapshotListNames.return_value = list(listed)
+    p.get_snapshot_jobs.return_value = list(jobs)
+    p.snapshot_files_state.return_value = before or {"disks": {}, "own": [], "points": []}
+    p.delete_external_snapshot.return_value = list(left)
+    p.left_behind_since.return_value = list(left)
+    return p
+
+
+class SnapshotDeleteJobTestCase(TransactionTestCase):
+    def setUp(self):
+        compute = Compute.objects.create(name="job-compute", hostname="192.0.2.10", login="root", password="", type=1)
+        self.instance = Instance.objects.create(compute=compute, name="job-vm",
+                                                uuid="11111111-2222-3333-4444-555555555555")
+        self.admin = get_user_model().objects.create_superuser("job_admin", "a@example.com", "pw")
+        self.client.force_login(self.admin)
+        patcher = patch("instances.jobs.wvmInstance", return_value=proxy())
+        self.wvm = patcher.start()
+        self.addCleanup(patcher.stop)
+
+    def post(self, key="key-00000001", name="one"):
+        return self.client.post(
+            reverse("instances:delete_external_snapshot", args=[self.instance.pk]),
+            {"name": name, "submission_key": key},
+            HTTP_REFERER=reverse("instances:instance", args=[self.instance.pk]),
+        )
+
+    def test_the_request_queues_one_job_holding_the_vm(self):
+        self.post()
+        self.post()
+        op = Operation.objects.get()
+        self.assertEqual((op.kind, op.state, op.params),
+                         ("snapshot.delete", Operation.QUEUED, {"instance": self.instance.pk, "name": "one"}))
+        self.assertEqual(set(Claim.objects.values_list("resource", "mode")),
+                         {(f"compute:{self.instance.compute_id}", "shared"),
+                          (f"instance:{self.instance.uuid}", "exclusive")})
+
+    def test_a_retry_after_the_job_ran_returns_it_without_libvirt(self):
+        self.post()
+        self.wvm.return_value = proxy(listed=())  # the snapshot is gone now
+        self.post()
+        self.assertEqual(Operation.objects.count(), 1)
+
+    def test_a_second_delete_is_refused_with_its_reason(self):
+        self.post(key="key-00000001")
+        response = self.post(key="key-00000002")
+        self.assertEqual(Operation.objects.count(), 1)
+        self.assertTrue(any("Delete snapshot is queued" in str(m) for m in response.wsgi_request._messages))
+
+    def test_a_long_name_is_kept_whole(self):
+        # its truncated prefix could name another snapshot
+        name = "s" * 100
+        self.assertEqual(DeleteExternalSnapshot().normalize({"instance": "1", "name": name})["name"], name)
+
+    def submitted(self):
+        return jobs.submit("snapshot.delete", self.admin, {"instance": self.instance.pk, "name": "one"},
+                              "key-00000009")
+
+    def test_run_records_the_files_before_the_delete(self):
+        op = self.submitted()
+        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()
+        self.wvm.return_value = proxy(before={"disks": {"vda": ["/p/a"]}, "own": ["/p/a.one"], "points": []},
+                                      left=["/p/a.one"])
+        ctx = Context(op, SimpleNamespace(identity="wvc-test:1:a"))
+        kind = DeleteExternalSnapshot()
+        kind.precheck(op, ctx)
+        result = kind.run(op, ctx)
+        op.refresh_from_db()
+        self.wvm.return_value.delete_external_snapshot.assert_called_once_with("one")
+        self.assertEqual(op.intent["before"]["own"], ["/p/a.one"])
+        self.assertEqual(result, {"left_behind": ["/p/a.one"]})
+        jobs.finish(op, Outcome(Operation.SUCCEEDED, result=result))
+        self.assertTrue(Logs.objects.filter(message="Delete external snapshot: one").exists())
+
+    def logged(self):
+        return list(Logs.objects.values_list("user", "host", "instance", "message"))
+
+    def test_every_end_is_logged_with_its_state(self):
+        for state in (Operation.FAILED, Operation.UNCERTAIN):
+            Logs.objects.all().delete()
+            Operation.objects.all().delete()
+            op = self.submitted()
+            jobs.finish(op, Outcome(state, error="x"))
+            self.assertEqual(self.logged(), [("job_admin", "job-compute", "job-vm",
+                                              f"Delete external snapshot: one ({state})")])
+        Logs.objects.all().delete()
+        Operation.objects.all().delete()
+        jobs.cancel(self.submitted().pk, self.admin)
+        self.assertEqual(self.logged(), [("job_admin", "job-compute", "job-vm", "Delete external snapshot: one (cancelled)")])
+
+    def test_a_compute_name_with_a_slash_is_logged_whole(self):
+        Compute.objects.filter(pk=self.instance.compute_id).update(name="region/job-compute")
+        op = self.submitted()
+        jobs.finish(op, Outcome(Operation.SUCCEEDED))
+        self.assertEqual(self.logged(), [("job_admin", "region/job-compute", "job-vm", "Delete external snapshot: one")])
+
+    def test_a_resolved_job_is_logged_on_its_vm(self):
+        op = self.submitted()
+        jobs.finish(op, Outcome(Operation.UNCERTAIN, error="x"))
+        Logs.objects.all().delete()
+        jobs.resolve(op.pk, self.admin, Operation.FAILED, "disks checked")
+        self.assertEqual(self.logged(), [("job_admin", "job-compute", "job-vm",
+                                          f"Job {op.pk} resolved as failed: disks checked")])
+
+    def test_retention_keeps_the_log(self):
+        op = self.submitted()
+        jobs.finish(op, Outcome(Operation.FAILED, error="x"))
+        Operation.objects.filter(pk=op.pk).update(finished_at=timezone.now() - timedelta(days=400))
+        housekeeping()
+        self.assertFalse(Operation.objects.filter(pk=op.pk).exists())
+        self.assertEqual([row[3] for row in self.logged()], ["Delete external snapshot: one (failed)"])
+
+    def test_precheck_refuses_a_moved_vm_and_an_inactive_user(self):
+        op = self.submitted()
+        other = Compute.objects.create(name="other", hostname="192.0.2.11", login="root", password="", type=1)
+        Instance.objects.filter(pk=self.instance.pk).update(compute=other)
+        with self.assertRaisesRegex(Exception, "moved"):
+            DeleteExternalSnapshot().precheck(op, Context(op, worker=None))
+        Instance.objects.filter(pk=self.instance.pk).update(compute=self.instance.compute)
+        get_user_model().objects.filter(pk=self.admin.pk).update(is_active=False)
+        op = Operation.objects.select_related("user").get(pk=op.pk)
+        with self.assertRaises(Exception):
+            DeleteExternalSnapshot().precheck(op, Context(op, worker=None))
+
+    def reconcile(self, **kw):
+        Operation.objects.all().delete()
+        self.wvm.return_value = proxy()  # the snapshot exists when the job is submitted
+        op = self.submitted()
+        Operation.objects.filter(pk=op.pk).update(state=Operation.RUNNING, effects_started=True,
+                                                  intent={"compute": self.instance.compute_id,
+                                                          "before": {"disks": {}, "own": [], "points": []}})
+        op.refresh_from_db()
+        self.wvm.return_value = proxy(**kw)
+        return DeleteExternalSnapshot().reconcile(op)
+
+    def test_reconcile_table(self):
+        merging = [{"dev": "vda", "cur": 1, "end": 2}]
+        self.assertEqual(self.reconcile(listed=()).state, Operation.SUCCEEDED)
+        self.assertEqual(self.reconcile(jobs=merging).state, Operation.RUNNING)
+        self.assertEqual(self.reconcile(listed=(), jobs=merging).state, Operation.RUNNING)
+        interrupted = self.reconcile()
+        self.assertEqual(interrupted.state, Operation.UNCERTAIN)
+        self.assertIn("It cannot be told whether the disks changed; check the VM before deleting the snapshot again",
+                      interrupted.error)
+
+    def test_reconcile_reports_a_merge_that_stays_at_its_end_as_stalled(self):
+        at_end = self.reconcile(jobs=[{"dev": "vda", "cur": 2, "end": 2}, {"dev": "vdb", "cur": 5, "end": 5}])
+        self.assertEqual((at_end.state, at_end.stalled), (Operation.RUNNING, True))
+        copying = self.reconcile(jobs=[{"dev": "vda", "cur": 2, "end": 2}, {"dev": "vdb", "cur": 1, "end": 5}])
+        self.assertEqual((copying.state, copying.stalled), (Operation.RUNNING, False))
+
+    def test_reconcile_reports_the_merge_progress(self):
+        merging = self.reconcile(jobs=[{"dev": "vda", "cur": 2, "end": 2}, {"dev": "vdb", "cur": 1, "end": 5}])
+        self.assertEqual(merging.progress, (3, 7))
+
+    def test_reconcile_watches_an_offline_merge_of_a_shut_off_vm(self):
+        # libvirt reports the merge of a shut-off VM's snapshot as an offline block job
+        p = proxy()
+        p._XMLDesc.return_value = ("<domain><devices><disk type='file' device='disk'>"
+                                   "<source file='/p/a.one'/><target dev='vda'/></disk></devices></domain>")
+        p._disk_sources = wvmInstance._disk_sources
+        p.instance.isActive.return_value = False
+        p.instance.blockJobInfo.side_effect = lambda dev, flags: {"cur": 1, "end": 2, "type": 4}
+        p.get_snapshot_jobs = lambda: wvmInstance.get_snapshot_jobs(p)
+        Operation.objects.all().delete()
+        self.wvm.return_value = proxy()
+        op = self.submitted()
+        Operation.objects.filter(pk=op.pk).update(state=Operation.RUNNING, effects_started=True,
+                                                  intent={"compute": self.instance.compute_id,
+                                                          "before": {"disks": {}, "own": [], "points": []}})
+        op.refresh_from_db()
+        self.wvm.return_value = p
+        self.assertEqual(DeleteExternalSnapshot().reconcile(op).state, Operation.RUNNING)
+
+    def test_reconcile_of_a_vm_gone_from_its_compute(self):
+        err = libvirtError("Domain not found")
+        err.err = (42,)  # VIR_ERR_NO_DOMAIN
+        Operation.objects.all().delete()
+        op = self.submitted()
+        Operation.objects.filter(pk=op.pk).update(state=Operation.RUNNING, intent={"compute": self.instance.compute_id})
+        op.refresh_from_db()
+        p = proxy()
+        p.instance.snapshotListNames.side_effect = err
+        self.wvm.return_value = p
+        self.assertEqual(DeleteExternalSnapshot().reconcile(op).state, Operation.UNCERTAIN)

instances/tests/test_snapshot_views.py

@@ -1,5 +1,6 @@
-"""The snapshot views call the backend once and show its refusals as
+"""The snapshot views call the backend once (an external snapshot delete
-messages; they no longer start, pause or stop the VM themselves."""
+queues a job instead) 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
@@ -15,6 +16,7 @@ from vrtManager import util
 from appsettings.models import AppSettings
 from computes.models import Compute
 from instances.models import Instance
+from operations.models import Claim, Operation
 
 
 class SnapshotViewsTestCase(TestCase):
@@ -32,7 +34,6 @@ class SnapshotViewsTestCase(TestCase):
             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,
@@ -66,26 +67,48 @@ class SnapshotViewsTestCase(TestCase):
                 _, method, _ = self.post("create_external_snapshot", {"name": "a"})
                 method.assert_called_once_with("a", "", False, False, **expected)
 
-    def test_revert_and_delete_call_the_backend_once_and_leave_power_alone(self):
+    def test_revert_calls_the_backend_once_and_leaves_power_alone(self):
-        for view in ("revert_external_snapshot", "delete_external_snapshot"):
+        proxy, method, _ = self.post("revert_external_snapshot", {"name": "a"})
-            with self.subTest(view):
+        method.assert_called_once_with("a")
-                proxy, method, _ = self.post(view, {"name": "a"})
+        for power in ("start", "force_shutdown", "shutdown"):
-                method.assert_called_once_with("a")
+            getattr(proxy, power).assert_not_called()
-                for power in ("start", "force_shutdown", "shutdown"):
-                    getattr(proxy, power).assert_not_called()
 
     def test_files_left_behind_are_named_in_a_warning(self):
-        for view in ("revert_external_snapshot", "delete_external_snapshot"):
+        _, _, messages = self.post("revert_external_snapshot", {"name": "a"}, left=["/pool/a.one", "/pool/vm.one.mem"])
-            with self.subTest(view):
+        warning = [m for m in messages if "/pool/a.one, /pool/vm.one.mem" in m]
-                _, _, messages = self.post(view, {"name": "a"}, left=["/pool/a.one", "/pool/vm.one.mem"])
+        self.assertEqual(len(warning), 1, messages)
-                warning = [m for m in messages if "/pool/a.one, /pool/vm.one.mem" in m]
+        self.assertIn("qemu user", warning[0])
-                self.assertEqual(len(warning), 1, messages)
+        _, _, messages = self.post("revert_external_snapshot", {"name": "a"})
-                self.assertIn("qemu user", warning[0])
+        self.assertFalse([m for m in messages if "not removed" in m], messages)
-                _, _, messages = self.post(view, {"name": "a"})
+
-                self.assertFalse([m for m in messages if "not removed" in m], messages)
+    def delete(self, listed):
+        with patch("instances.jobs.wvmInstance") as mock_wvm:
+            proxy = mock_wvm.return_value
+            proxy.instance.snapshotListNames.return_value = list(listed)
+            self.client.cookies.pop("messages", None)
+            response = self.client.post(
+                reverse("instances:delete_external_snapshot", args=[self.instance.id]),
+                {"name": "a", "submission_key": "key-00000001"},
+                HTTP_REFERER=reverse("instances:instance", args=[self.instance.id]),
+            )
+        self.assertEqual(response.status_code, 302)
+        return proxy, [str(m) for m in get_messages(response.wsgi_request)]
+
+    def test_delete_queues_a_job_and_leaves_libvirt_to_the_worker(self):
+        proxy, messages = self.delete(listed=["a"])
+        op = Operation.objects.get()
+        self.assertEqual((op.kind, op.state, op.params),
+                         ("snapshot.delete", Operation.QUEUED, {"instance": self.instance.id, "name": "a"}))
+        self.assertTrue(any("being deleted" in m for m in messages), messages)
+        proxy.delete_external_snapshot.assert_not_called()
+
+    def test_a_refused_delete_becomes_a_message_and_queues_nothing(self):
+        _, messages = self.delete(listed=[])
+        self.assertTrue(any("has no external snapshot named a" in m for m in messages), messages)
+        self.assertFalse(Operation.objects.exists())
 
     def test_refusals_become_messages(self):
-        for view in ("create_external_snapshot", "revert_external_snapshot", "delete_external_snapshot",
+        for view in ("create_external_snapshot", "revert_external_snapshot",
                      "revert_snapshot", "delete_snapshot", "snapshot"):
             with self.subTest(view):
                 _, _, messages = self.post(view, {"name": "a"}, error="refused for a reason")
@@ -174,6 +197,40 @@ class SnapshotTabTestCase(TestCase):
         self.assertTrue(all(b.get("disabled") is not None
                             for b in doc.xpath("//div[@id='snapshots']//button[@type='submit']")))
 
+    def hold(self, state):
+        key = f"instance:{self.instance.uuid}"
+        op = Operation.objects.create(kind="snapshot.delete", username="snap_admin", submission_key="k" * 12,
+                                      resource=key, resource_label="snap-vm", state=state)
+        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")))
+        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]
+
+        for name, lists, panes in cases:
+            with self.subTest(name):
+                doc = self.page(**lists, status=5)
+                self.assertTrue(all(enabled(doc, panes)), "enabled without a job")
+                claim = self.hold(Operation.QUEUED)
+                doc = self.page(**lists, status=5)
+                self.assertFalse(doc.xpath(buttons + "[not(@disabled)]"), "disabled while the job holds the VM")
+                self.assertNotIn("A disk job is running", doc.text_content())
+                claim.operation.delete()
+                self.assertTrue(all(enabled(self.page(**lists, status=5), panes)), "enabled after the job ends")
+
+    def test_an_uncertain_job_keeps_the_buttons_disabled(self):
+        self.hold(Operation.UNCERTAIN)
+        doc = self.page(internal=[self.snap("i")], status=5)
+        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"])

instances/views.py

@@ -38,7 +38,7 @@ from libvirt import (VIR_DOMAIN_UNDEFINE_KEEP_NVRAM,
                      VIR_DOMAIN_UNDEFINE_NVRAM,
                      libvirtError)
 from logs.views import addlogmsg
-from operations import claims
+from operations import claims, jobs
 from operations.utils import busy_message, summary
 from webvirtcloud.middleware import error_text
 from vrtManager import util
@@ -1459,20 +1459,23 @@ def revert_external_snapshot(request, pk):
 
 
 @require_POST
-@serialize_instance_mutation
 def delete_external_snapshot(request, pk):
-    instance = get_instance(request.user, pk, perm_type="change")
+    get_instance(request.user, pk, perm_type="change")
     if not request.user.has_perm("instances.snapshot_instances"):
         raise PermissionDenied
-    name = request.POST.get("name", "")
     try:
-        left = instance.proxy.delete_external_snapshot(name)
+        op = jobs.submit("snapshot.delete", request.user,
+                            {"instance": pk, "name": request.POST.get("name", "")},
+                            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)})
     else:
-        _warn_left_behind(request, left)
+        messages.info(request, _("The snapshot is being deleted; follow it in the job panel."))
-        msg = _("Delete external snapshot: %(snap)s") % {"snap": name}
+        jobs.remember_job(request, op)
-        addlogmsg(request.user.username, instance.compute.name, instance.name, msg)
     return _back(request, pk, "managesnapshot")
 
 

operations/jobs.py

@@ -248,3 +248,13 @@ def resolve(op_id, user, outcome_state, note):
         _log(user.username, host, instance, f"Job {op.pk} resolved as {outcome_state}: {note}")
     op.refresh_from_db()
     return op
+
+
+def remember_job(request, op):
+    """The resource page's panel shows this job even if it ends before the first poll."""
+    ids = request.session.get("operations_shown", [])
+    request.session["operations_shown"] = [*ids, str(op.pk)][-20:]
+
+
+def take_remembered(request):
+    return request.session.pop("operations_shown", [])

operations/tests/test_service.py

@@ -4,7 +4,7 @@ from unittest.mock import patch
 
 from django.contrib.auth import get_user_model
 from django.core.exceptions import PermissionDenied
-from django.test import TransactionTestCase
+from django.test import SimpleTestCase, TransactionTestCase
 from django.utils import timezone
 from vrtManager.util import OperationError
 
@@ -192,3 +192,13 @@ class CancelFinishResolveTestCase(TransactionTestCase):
         self.assertFalse(Claim.objects.exists())
         entry = Logs.objects.get(message__icontains="resolved")
         self.assertEqual((entry.user, entry.host, entry.instance), (self.admin.username, "c", "vm"))
+
+
+class RememberJobTestCase(SimpleTestCase):
+    def test_the_last_twenty_jobs_are_remembered_until_taken(self):
+        request = SimpleNamespace(session={})
+        for n in range(25):
+            jobs.remember_job(request, SimpleNamespace(pk=n))
+        self.assertEqual(request.session["operations_shown"], [str(n) for n in range(5, 25)])
+        self.assertEqual(jobs.take_remembered(request), [str(n) for n in range(5, 25)])
+        self.assertEqual(jobs.take_remembered(request), [])

vrtManager/instance.py

@@ -38,6 +38,7 @@ try:
         VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED,
         VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING,
         VIR_ERR_CONFIG_UNSUPPORTED,
+        VIR_ERR_OPERATION_INVALID,
         VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL,
         VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL,
         libvirtError,
@@ -1751,6 +1752,17 @@ class wvmInstance(wvmConnect):
                 points.update(self._disk_sources(saved).values())
         return disks, own, points | own
 
+    def snapshot_files_state(self):
+        """_snapshot_files as plain lists, recorded before a snapshot change."""
+        disks, own, points = self._snapshot_files()
+        return {"disks": {dev: sorted(files) for dev, files in disks.items()},
+                "own": sorted(own), "points": sorted(points)}
+
+    def left_behind_since(self, state):
+        """_left_behind from a state recorded by snapshot_files_state."""
+        before = ({dev: set(files) for dev, files in state["disks"].items()}, set(state["own"]), set(state["points"]))
+        return self._left_behind(before)
+
     def _refresh_pools_after_snapshot(self):
         """Refresh the pools once libvirt has changed the snapshot files. The
         snapshot operation is done by then, so an error is only logged;
@@ -1844,12 +1856,19 @@ class wvmInstance(wvmConnect):
         return snapshots
 
     def get_snapshot_jobs(self):
-        """Block jobs (snapshot merges) running on the VM's disks."""
+        """Block jobs (snapshot merges) running on the VM's disks. libvirt
-        if not self.instance.isActive():
+        merges a shut-off VM's snapshot as an offline block job; a libvirt
-            return []
+        without offline jobs refuses the query for an inactive VM, which
+        means no job."""
+        active = self.instance.isActive()
         jobs = []
-        for dev in self._disk_sources(self._XMLDesc(0)):
+        for dev in self._disk_sources(self._XMLDesc(0 if active else PERSISTENT_XML)):
-            info = self.instance.blockJobInfo(dev, 0)
+            try:
+                info = self.instance.blockJobInfo(dev, 0)
+            except libvirtError as err:
+                if active or err.get_error_code() != VIR_ERR_OPERATION_INVALID:
+                    raise
+                info = None
             if info:
                 jobs.append({"dev": dev, "cur": info["cur"], "end": info["end"]})
         return jobs

vrtManager/tests/test_external_snapshots.py

@@ -2,6 +2,7 @@
 checks preconditions and picks the flags. libvirt is mocked here; the live
 cases are in instances/tests/test_live_snapshots.py."""
 
+import json
 import unittest
 from datetime import datetime
 from unittest.mock import MagicMock, patch
@@ -167,10 +168,42 @@ class SnapshotJobsTestCase(unittest.TestCase):
         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):
+    def test_a_stopped_vm_without_a_job_has_none(self):
+        self.assertEqual(proxy(status=5).get_snapshot_jobs(), [])
+
+    def test_an_offline_merge_of_a_stopped_vm_is_reported(self):
+        # libvirt deletes an external snapshot of a shut-off VM as an offline block job
+        p = proxy(status=5, jobs={"vdb": {"cur": 3, "end": 8, "type": 4, "bandwidth": 0}})
+        p._XMLDesc = lambda flags: DOMAIN if flags == PERSISTENT_XML else "<domain><devices/></domain>"
+        self.assertEqual(p.get_snapshot_jobs(), [{"dev": "vdb", "cur": 3, "end": 8}])
+
+    def test_a_libvirt_without_offline_jobs_reports_none(self):
+        err = libvirtError("domain is not running")
+        err.err = (libvirt.VIR_ERR_OPERATION_INVALID,)
         p = proxy(status=5)
+        p.instance.blockJobInfo.side_effect = err
         self.assertEqual(p.get_snapshot_jobs(), [])
-        p.instance.blockJobInfo.assert_not_called()
+
+    def test_another_error_of_a_stopped_vm_propagates(self):
+        err = libvirtError("cannot reach the host")
+        err.err = (libvirt.VIR_ERR_SYSTEM_ERROR,)
+        p = proxy(status=5)
+        p.instance.blockJobInfo.side_effect = err
+        with self.assertRaises(libvirtError):
+            p.get_snapshot_jobs()
+
+    def test_a_running_vm_does_not_swallow_operation_invalid(self):
+        err = libvirtError("operation invalid")
+        err.err = (libvirt.VIR_ERR_OPERATION_INVALID,)
+        p = proxy()
+        p.instance.blockJobInfo.side_effect = err
+        with self.assertRaises(libvirtError):
+            p.get_snapshot_jobs()
+
+    def test_a_running_vm_reads_the_live_disks(self):
+        p = proxy(jobs={"vdb": {"cur": 1, "end": 2}})
+        p._XMLDesc = lambda flags: DOMAIN if flags == 0 else "<domain><devices/></domain>"
+        self.assertEqual(p.get_snapshot_jobs(), [{"dev": "vdb", "cur": 1, "end": 2}])
 
 
 class SnapshotLookupTestCase(unittest.TestCase):
@@ -192,6 +225,10 @@ class SnapshotStateTestCase(unittest.TestCase):
         with self.assertRaisesRegex(util.OperationError, "still running"):
             proxy(jobs={"vda": {"cur": 1, "end": 2}})._check_snapshot_state()
 
+    def test_an_offline_merge_of_a_stopped_vm_is_refused(self):
+        with self.assertRaisesRegex(util.OperationError, "still running"):
+            proxy(status=5, 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()
 
@@ -499,7 +536,7 @@ class LeftBehindTestCase(unittest.TestCase):
     discarded file (on NFS it removes them as the qemu user), so the delete
     and revert report the files nothing uses any more that still exist."""
 
-    def run_delete(self, still_there):
+    def delete_fixture(self, still_there):
         one = chain_snap("one", "/pool/a.one", memory="/pool/vm.one.mem")
         two = chain_snap("two", "/pool/a.two")
         p = proxy(external=[one, two])
@@ -515,7 +552,20 @@ class LeftBehindTestCase(unittest.TestCase):
             return MagicMock()
 
         p.get_volume_by_path = volume
-        return p.delete_external_snapshot("one")
+        return p
+
+    def run_delete(self, still_there):
+        return self.delete_fixture(still_there).delete_external_snapshot("one")
+
+    def test_a_recorded_state_gives_the_same_files(self):
+        # the delete job records the state before the merge and compares later
+        still_there = {"/pool/a.one", "/pool/vm.one.mem", "/pool/a.two", "/pool/a.qcow2"}
+        p = self.delete_fixture(still_there)
+        direct = p._left_behind(p._snapshot_files())
+        p = self.delete_fixture(still_there)
+        state = json.loads(json.dumps(p.snapshot_files_state()))  # as stored in the job
+        self.assertEqual(p.left_behind_since(state), direct)
+        self.assertEqual(direct, ["/pool/a.one", "/pool/vm.one.mem"])
 
     def test_files_libvirt_could_not_remove_are_reported(self):
         self.assertEqual(