feat(instances): snapshot revert runs as a job

instances/jobs.py

@@ -337,6 +337,59 @@ class CreateSnapshot(Kind):
         return _log(op, state, f"Create {kind}: {op.params['name']}")
 
 
+def _current(proxy):
+    dom = proxy.instance
+    return dom.snapshotCurrent(0).getName() if dom.hasCurrentSnapshot(0) else None
+
+
+@register
+class RevertSnapshot(Kind):
+    name = "snapshot.revert"
+    label = gettext_noop("Revert snapshot")
+    time_limit = 6 * 3600  # a revert cannot be stopped: over time is only noted
+
+    def normalize(self, raw):
+        return {"instance": int(raw["instance"]), "name": str(raw.get("name", "")),
+                "external": bool(raw.get("external"))}
+
+    def validate(self, user, params):
+        _proxy(_authorized(user, params["instance"]))._snapshot(params["name"], params["external"])
+
+    def plan(self, user, params):
+        return _vm_plan(params)
+
+    def precheck(self, op, ctx):
+        proxy = _checked_proxy(op)
+        if op.params["external"]:
+            proxy.check_external_revert(op.params["name"])
+        else:
+            proxy._snapshot(op.params["name"], False)
+
+    def run(self, op, ctx):
+        proxy = _proxy(_instance(op.params["instance"]))
+        ctx.record_intent(was_current=_current(proxy))
+        ctx.set_phase(gettext_noop("Reverting the VM"))
+        if op.params["external"]:
+            return {"left_behind": proxy.revert_external_snapshot(op.params["name"])}
+        proxy.snapshot_revert(op.params["name"])
+        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 reverting the VM"))
+        if _current(proxy) == op.params["name"] != op.intent.get("was_current"):
+            return Outcome(Operation.SUCCEEDED)
+        return Outcome(Operation.UNCERTAIN, error="It cannot be told whether the VM was reverted; "
+                                                  "check it, then revert again or resolve this job.")
+
+    def log_entry(self, op, state):
+        kind = "external snapshot" if op.params["external"] else "snapshot"
+        return _log(op, state, f"Revert {kind}: {op.params['name']}")
+
+
 @register
 class CloneInstance(Kind):
     name = "instance.clone"

instances/templates/instances/snapshots_tab.html

@@ -102,7 +102,7 @@
                                     <td>{{ snap.description }}</td>
                                     <td style="width:30px;">
                                         <form action="{% url 'instances:revert_snapshot' instance.id %}" method="post" role="form" aria-label="Restore snapshot form">
-                                            {% csrf_token %}
+                                            {% csrf_token %}{% submission_key %}
                                             <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 or held %}disabled{% endif %}>
@@ -152,7 +152,7 @@
                                     <td>{{ ext_snap.description }}</td>
                                     <td style="width:30px;">
                                         <form action="{% url 'instances:revert_external_snapshot' instance.id %}" method="post" role="form" aria-label="Restore external snapshot form">
-                                            {% csrf_token %}
+                                            {% csrf_token %}{% submission_key %}
                                             <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 %}"

instances/tests/test_snapshot_jobs.py

@@ -3,13 +3,14 @@
 from types import SimpleNamespace
 from unittest.mock import patch
 
+from vrtManager.util import OperationError
 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.jobs import CreateSnapshot, RevertSnapshot
 from instances.models import Instance
 from operations.jobs import Context, get_kind
 from operations.models import Claim, Operation, acquire
@@ -36,7 +37,8 @@ class SnapshotJobsTestCase(TransactionTestCase):
         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)
+        self.assertFalse(self.proxy.create_snapshot.called or self.proxy.create_external_snapshot.called
+                         or self.proxy.snapshot_revert.called or self.proxy.revert_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()
@@ -69,3 +71,28 @@ class SnapshotJobsTestCase(TransactionTestCase):
         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)
+
+    def test_an_external_revert_is_made_by_the_worker(self):
+        self.proxy.revert_external_snapshot.return_value = ["/pool/a.one"]
+        op, result = self.queue_and_run("revert_external_snapshot", {"name": "a"})
+        self.assertEqual((op.kind, result), ("snapshot.revert", {"left_behind": ["/pool/a.one"]}))
+        self.proxy.revert_external_snapshot.assert_called_once_with("a")
+
+    def test_a_revert_libvirt_cannot_do_is_refused_before_effects(self):
+        self.proxy.check_external_revert.side_effect = OperationError("taken by an older version")
+        with self.assertRaises(OperationError):
+            self.queue_and_run("revert_external_snapshot", {"name": "a"})
+        self.assertFalse(Operation.objects.get().effects_started)
+
+    def test_an_internal_revert_is_made_by_the_worker(self):
+        self.queue_and_run("revert_snapshot", {"name": "b"})
+        self.proxy.snapshot_revert.assert_called_once_with("b")
+
+    def test_revert_recovery(self):
+        op = Operation(params={"instance": self.instance.pk, "name": "a", "external": False},
+                       intent={"compute": self.instance.compute_id, "was_current": "z"})
+        self.proxy.instance.hasCurrentSnapshot.return_value = 1
+        self.proxy.instance.snapshotCurrent.return_value.getName.return_value = "a"
+        self.assertEqual(RevertSnapshot().reconcile(op).state, Operation.SUCCEEDED)
+        op.intent["was_current"] = "a"  # it was current before: nothing tells
+        self.assertEqual(RevertSnapshot().reconcile(op).state, Operation.UNCERTAIN)

instances/tests/test_snapshot_views.py

@@ -30,15 +30,12 @@ class SnapshotViewsTestCase(TestCase):
         admin = get_user_model().objects.create_superuser("snap_admin", "s@example.com", "pw")
         self.client.force_login(admin)
 
-    def post(self, view, data, error=None, left=()):
+    def post(self, view, data, error=None):
         with patch("instances.models.wvmInstance") as mock_wvm:
             proxy = mock_wvm.return_value
             method = {
-                "revert_external_snapshot": proxy.revert_external_snapshot,
-                "revert_snapshot": proxy.snapshot_revert,
                 "delete_snapshot": proxy.snapshot_delete,
             }[view]
-            method.return_value = list(left)  # files revert and delete left behind
             if error:
                 method.side_effect = util.OperationError(error)
             self.client.cookies.pop("messages", None)  # only this request's messages
@@ -60,20 +57,6 @@ class SnapshotViewsTestCase(TestCase):
                 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"})
-        method.assert_called_once_with("a")
-        for power in ("start", "force_shutdown", "shutdown"):
-            getattr(proxy, power).assert_not_called()
-
-    def test_files_left_behind_are_named_in_a_warning(self):
-        _, _, messages = self.post("revert_external_snapshot", {"name": "a"}, left=["/pool/a.one", "/pool/vm.one.mem"])
-        warning = [m for m in messages if "/pool/a.one, /pool/vm.one.mem" in m]
-        self.assertEqual(len(warning), 1, messages)
-        self.assertIn("qemu user", warning[0])
-        _, _, messages = self.post("revert_external_snapshot", {"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
@@ -101,7 +84,7 @@ class SnapshotViewsTestCase(TestCase):
         self.assertFalse(Operation.objects.exists())
 
     def test_refusals_become_messages(self):
-        for view in ("revert_external_snapshot", "revert_snapshot", "delete_snapshot"):
+        for view in ("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

@@ -521,15 +521,6 @@ class InstancesTestCase(TestCase):
             self.instance.__dict__.pop(cached, None)
         self.assertEqual(len(self.instance.snapshots), 1)
 
-        response = self.client.post(
-            reverse("instances:revert_snapshot", args=[self.instance.id]),
-            {
-                "name": "test-snapshot",
-            },
-            HTTP_REFERER=reverse("index"),
-        )
-        self.assertEqual(response.status_code, 302)
-
         response = self.client.post(
             reverse("instances:delete_snapshot", args=[self.instance.id]),
             {

instances/views.py

@@ -1360,37 +1360,9 @@ def delete_snapshot(request, pk):
 
 
 @require_POST
-@serialize_instance_mutation
 def revert_snapshot(request, pk):
-    instance = get_instance(request.user, pk, perm_type="change")
+    return _snapshot_job(request, pk, "snapshot.revert", {"name": request.POST.get("name", "")},
-    if not request.user.has_perm("instances.snapshot_instances"):
+                         _("The VM is being reverted; follow it in the job panel."))
-        raise PermissionDenied
-    snap_name = request.POST.get("name", "")
-    try:
-        instance.proxy.snapshot_revert(snap_name)
-    except libvirtError as err:
-        messages.error(request, _("Snapshot was not reverted: %(err)s") % {"err": error_text(request, err)})
-        return _back(request, pk, "managesnapshot")
-    msg = _("Successful revert snapshot: ")
-    msg += snap_name
-    messages.success(request, msg)
-    msg = _("Revert snapshot: %(snap)s") % {"snap": snap_name}
-    addlogmsg(request.user.username, instance.compute.name, instance.name, msg)
-    return _back(request, pk, "managesnapshot")
-
-
-def _warn_left_behind(request, files):
-    """libvirt only logs it when it cannot remove a snapshot file."""
-    if files:
-        messages.warning(
-            request,
-            _(
-                "These snapshot files were not removed, although no VM on this host uses them any more: %(files)s. "
-                "On NFS libvirt removes them as the qemu user, who needs write access to their directory. "
-                "Remove them by hand."
-            )
-            % {"files": ", ".join(files)},
-        )
 
 
 @require_POST
@@ -1403,22 +1375,9 @@ def create_external_snapshot(request, pk):
 
 
 @require_POST
-@serialize_instance_mutation
 def revert_external_snapshot(request, pk):
-    instance = get_instance(request.user, pk, perm_type="change")
+    return _snapshot_job(request, pk, "snapshot.revert", {"name": request.POST.get("name", ""), "external": True},
-    if not request.user.has_perm("instances.snapshot_instances"):
+                         _("The VM is being reverted; follow it in the job panel."))
-        raise PermissionDenied
-    name = request.POST.get("name", "")
-    try:
-        left = instance.proxy.revert_external_snapshot(name)
-    except libvirtError as err:
-        messages.error(request, _("Snapshot was not reverted: %(err)s") % {"err": error_text(request, err)})
-    else:
-        messages.success(request, _("Successful revert snapshot: ") + name)
-        _warn_left_behind(request, left)
-        msg = _("Revert external snapshot: %(snap)s") % {"snap": name}
-        addlogmsg(request.user.username, instance.compute.name, instance.name, msg)
-    return _back(request, pk, "managesnapshot")
 
 
 def _snapshot_job(request, pk, kind, params, queued):

vrtManager/instance.py

@@ -1683,11 +1683,9 @@ class wvmInstance(wvmConnect):
                     "and the memory if it is saved)"
                 )
 
-    def revert_external_snapshot(self, name):
+    def check_external_revert(self, name):
-        """libvirt returns the disks and the definition to the snapshot's. A
+        """Refuses a revert that cannot work, before anything changes; returns
-        disk-only snapshot keeps the VM running, paused or shut off as it is;
+        the snapshot."""
-        a snapshot with memory resumes the VM as it was saved. Returns the
-        files libvirt left behind (see _left_behind)."""
         snap = self._snapshot(name, external=True)
         self._check_snapshot_state()
         tree = etree.fromstring(snap.getXMLDesc(0))
@@ -1701,6 +1699,15 @@ class wvmInstance(wvmConnect):
             raise util.OperationError(
                 "This snapshot was taken by an older version on a UEFI VM and cannot be reverted; delete it instead"
             )
+        return snap
+
+    def revert_external_snapshot(self, name):
+        """libvirt returns the disks and the definition to the snapshot's. A
+        disk-only snapshot keeps the VM running, paused or shut off as it is;
+        a snapshot with memory resumes the VM as it was saved. Returns the
+        files libvirt left behind (see _left_behind)."""
+        snap = self.check_external_revert(name)
+        tree = etree.fromstring(snap.getXMLDesc(0))
         if tree.find("memory[@snapshot='external']") is not None:
             flags = 0
         else: