fix(quota): a snapshot revert must fit the quota

A revert gives the VM the vCPUs and memory it had when the snapshot was
taken. An owner could take a snapshot, shrink the VM, give the freed share
to another VM and revert, ending over the quota. The revert now checks
what it adds to the VM (against the larger of its running and next-start
sizes) as a resize does: on the page when it is queued, and again in the
worker before it runs, as a resize may have used the quota meanwhile.

instances/jobs.py

@@ -434,6 +434,28 @@ def _snapshot_definitions(proxy, params):
     return any(definition.xpath(HOST_DEVICES) for definition in definitions), definitions
 
 
+def _revert_quota(user, instance, proxy, params):
+    """A revert gives the VM the vCPUs and memory it had then: what that adds
+    to what it has now (running or at its next start) must fit the user's
+    quota, as a resize must."""
+    if user.is_superuser or instance.is_template:
+        return
+    sizes = [(d.findtext("vcpu"), d.findtext("memory")) for d in _snapshot_definitions(proxy, params)[1]]
+    if not sizes or any(None in size for size in sizes):  # an old snapshot may record only the VM's UUID
+        raise util.OperationError(_("Snapshot %(name)s does not record the VM's vCPUs and memory: "
+                                    "the quota cannot be checked."), name=params["name"])
+    with proxy.cached_reads():
+        cpus = max(int(vcpu) for vcpu, _memory in sizes) - max(proxy.get_vcpu(), proxy.get_vcpu(config=True))
+        memory = (max(int(memory) // 1024 for _vcpu, memory in sizes)
+                  - max(proxy.get_memory(), proxy.get_memory(config=True)))
+    quota = utils.check_user_quota(user, 0, cpus, memory, 0)
+    if quota == utils.QUOTA_UNVERIFIED:
+        raise util.OperationError(utils.QUOTA_UNVERIFIED_TEXT)
+    if quota:
+        raise util.OperationError(_("%(quota)s: cannot revert to snapshot %(name)s."),
+                                  quota=utils.quota_message(quota), name=params["name"])
+
+
 @register
 class RevertSnapshot(Kind):
     name = "snapshot.revert"
@@ -445,13 +467,21 @@ class RevertSnapshot(Kind):
                 "external": bool(raw.get("external"))}
 
     def validate(self, user, params):
-        _proxy(_authorized(user, params["instance"]))._snapshot(params["name"], params["external"])
+        instance = _authorized(user, params["instance"])
+        proxy = _proxy(instance)
+        proxy._snapshot(params["name"], params["external"])
+        _revert_quota(user, instance, proxy, params)
 
     def plan(self, user, params):
         plan = _vm_plan(params)
+        instance = _instance(params["instance"])
         # a revert that gives the VM host devices holds the whole compute: no other VM takes them meanwhile
-        if _snapshot_definitions(_proxy(_instance(params["instance"])), params)[0]:
+        if _snapshot_definitions(_proxy(instance), params)[0]:
             plan.claims[claims.compute_key(plan.intent["compute"])] = Claim.EXCLUSIVE
+        # the user's quota until it ends: the worker's check and the revert are one step to other
+        # increases (a resize, another revert), as a resize's check and change are
+        if not user.is_superuser and not instance.is_template:
+            plan.claims[claims.quota_key(user.pk)] = Claim.EXCLUSIVE
         return plan
 
     def precheck(self, op, ctx):
@@ -460,6 +490,8 @@ class RevertSnapshot(Kind):
             proxy.check_external_revert(op.params["name"])
         else:
             proxy.check_internal_revert()
+        # again: a resize while it was queued may have used the quota
+        _revert_quota(op.user, _instance(op.params["instance"]), proxy, op.params)
         # a revert gives the VM the host devices it had then: not one given since to another VM
         has_hostdevs, definitions = _snapshot_definitions(proxy, op.params)
         if has_hostdevs:

instances/tests/test_host_devices.py

@@ -167,8 +167,8 @@ class RevertTests(TestCase):
     def test_the_compute_is_held_only_for_a_revert_with_host_devices(self):
         compute = f"compute:{self.instance.compute_id}"
         self.snapshot(by_model())
-        self.assertEqual(self.kind.plan(None, self.params).claims[compute], "exclusive")
+        self.assertEqual(self.kind.plan(MagicMock(is_superuser=True), self.params).claims[compute], "exclusive")
         self.snapshot("")
-        self.assertEqual(self.kind.plan(None, self.params).claims[compute], "shared")
+        self.assertEqual(self.kind.plan(MagicMock(is_superuser=True), self.params).claims[compute], "shared")
         self.precheck()  # no host device: the host's devices are not read
         self.proxy.wvm.listAllDevices.assert_not_called()

instances/tests/test_snapshot_jobs.py

@@ -1,14 +1,17 @@
 """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 unittest.mock import PropertyMock, 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 accounts.models import UserAttributes, UserInstance
 from appsettings.settings import get_settings
+from django.contrib.auth.models import Permission
+from django.contrib.messages import get_messages
 from computes.models import Compute
 from instances.jobs import CreateSnapshot, RevertSnapshot
 from instances.models import Instance
@@ -30,6 +33,10 @@ class SnapshotJobsTestCase(TransactionTestCase):
         self.proxy._snapshot.return_value.getXMLDesc.return_value = "<domainsnapshot><domain/></domainsnapshot>"
         self.proxy.instance.isActive.return_value = False
         self.proxy.instance.hasCurrentSnapshot.return_value = 0
+        # the quota's reads of the user's VMs
+        reads = patch("instances.utils.wvmInstance")
+        self.vm_reads = reads.start().return_value
+        self.addCleanup(reads.stop)
 
     def queue_and_run(self, view, data):
         """Post the form; nothing runs in the request; then run the job as the worker does."""
@@ -95,6 +102,49 @@ class SnapshotJobsTestCase(TransactionTestCase):
         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_that_would_exceed_the_owner_s_quota_is_refused(self):
+        # snapshot at 4 vCPUs, shrunk to 2, the 2 given to another VM: reverting gives 6 of 4
+        owner = get_user_model().objects.create_user("snap_owner", password="pw")
+        owner.user_permissions.add(Permission.objects.get(codename="snapshot_instances"))
+        UserAttributes.objects.update_or_create(user=owner, defaults={"max_cpus": 4, "max_memory": 2048,
+                                                                      "max_instances": -1, "max_disk_size": -1})
+        UserInstance.objects.create(user=owner, instance=self.instance, is_change=True)
+        for proxy in (self.proxy, self.vm_reads):  # the VM now: 2 vCPUs, 1 GiB, running and at its next start
+            proxy.get_vcpu.return_value, proxy.get_memory.return_value = 2, 1024
+            proxy.get_disk_devices.return_value = []
+        other = Instance.objects.create(compute=self.instance.compute, name="snap-other", uuid="21212121-3434-5656-7878-909090909091")
+        UserInstance.objects.create(user=owner, instance=other, is_change=True)
+        patch.object(Compute, "status", new_callable=PropertyMock, return_value=True).start()
+        self.addCleanup(patch.stopall)
+        for vcpu, memory, admitted in ((4, 1024, False), (2, 2048, False), (2, 1024, True)):
+            with self.subTest(vcpu=vcpu, memory=memory):
+                self.client.force_login(owner)  # a new session: no messages of the last one
+                self.proxy._snapshot.return_value.getXMLDesc.return_value = (
+                    f"<domainsnapshot><domain><vcpu>{vcpu}</vcpu><memory>{memory * 1024}</memory></domain></domainsnapshot>")
+                response = self.client.post(reverse("instances:revert_snapshot", args=[self.instance.pk]),
+                                            {"name": "a", "submission_key": f"key-snap{vcpu}{memory}"})
+                self.assertEqual(Operation.objects.exists(), admitted)
+                shown = " ".join(str(m) for m in get_messages(response.wsgi_request))
+                self.assertEqual("cannot revert to snapshot a" in shown, not admitted, shown)
+                self.client.logout()
+        # the admitted revert holds the owner's quota until it ends: no resize or other revert
+        # passes the check meanwhile (its own check in the worker and its revert are one step)
+        op = Operation.objects.get()
+        self.assertIn((f"quota:user:{owner.pk}", "exclusive"), set(Claim.objects.filter(operation=op).values_list("resource", "mode")))
+        # the worker checks again: the VM may have been shrunk while it was queued (2 -> 1 vCPU, used elsewhere)
+        self.proxy.get_vcpu.return_value = 1
+        with self.assertRaisesMessage(OperationError, "cannot revert to snapshot a"):
+            RevertSnapshot().precheck(op, None)
+        # an old snapshot may record only the VM's UUID: refused, not a server error
+        Operation.objects.all().delete()
+        Claim.objects.all().delete()
+        self.proxy._snapshot.return_value.getXMLDesc.return_value = "<domainsnapshot><domain><uuid>x</uuid></domain></domainsnapshot>"
+        self.client.force_login(owner)
+        response = self.client.post(reverse("instances:revert_snapshot", args=[self.instance.pk]),
+                                    {"name": "a", "submission_key": "key-snapold1"})
+        self.assertFalse(Operation.objects.exists())
+        self.assertIn("does not record the VM's vCPUs and memory", " ".join(str(m) for m in get_messages(response.wsgi_request)))
+
     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):