fix(instances): the quota counts queued clones and pending values

A user's quota counted each VM's running vCPUs, memory and disks, so a
larger value set for the VM's next start was not counted, and a clone
counted only once its VM was recorded, so a user could queue clones
past the quota. Now:

- Each VM counts the larger of its running and its next-start values.
- A clone job counts as one VM with the source's persistent vCPUs,
  memory and disk size, which its plan stores, from its admission until
  its VM is recorded; a failed, cancelled or resolved job no longer
  counts. The clone's VM and its owner are recorded in one transaction,
  so the quota counts either the job or the VM, never neither.
- The quota reads each VM's definition once (cached_reads).

instances/jobs.py

@@ -5,6 +5,7 @@ import os
 import re
 import time
 
+from django.db import transaction
 from django.core.exceptions import PermissionDenied
 from django.shortcuts import get_object_or_404
 from django.template.loader import render_to_string
@@ -115,8 +116,9 @@ CLONE_FIELDS = ("name", "clone-title", "clone-description")
 
 
 def clone_data(user, instance, fields):
-    """The clone's settings from the form fields and the source as it is now;
-    OperationError with the reason when it cannot be made."""
+    """The clone's settings from the form fields and the source as it is now,
+    and what it adds to the user's quota; OperationError with the reason when
+    it cannot be made."""
     from instances.views import VOLUME_NAME_RE, quota_refused
 
     data = dict()
@@ -124,9 +126,10 @@ def clone_data(user, instance, fields):
     data["clone-title"] = fields.get("clone-title", "").strip()
     data["clone-description"] = fields.get("clone-description", "").strip()
 
-    # the clone copies the disks of the persistent definition
-    disk_sum = sum(int(disk.get("size") or 0) >> 30 for disk in instance.config_disks)
-    quota_msg = utils.check_user_quota(user, 1, instance.vcpu, instance.memory, disk_sum)
+    # the clone copies the persistent definition
+    quota = {"cpus": instance.proxy.get_vcpu(config=True), "memory": instance.proxy.get_memory(config=True),
+             "disk": utils.disk_gib(instance.config_disks)}
+    quota_msg = utils.check_user_quota(user, 1, quota["cpus"], quota["memory"], quota["disk"])
 
     data["disk_owner_uid"] = int(app_settings.INSTANCE_VOLUME_DEFAULT_OWNER_UID)
     data["disk_owner_gid"] = int(app_settings.INSTANCE_VOLUME_DEFAULT_OWNER_GID)
@@ -182,7 +185,7 @@ def clone_data(user, instance, fields):
     if invalid_disks:
         raise util.OperationError(
             _("Disk name '%(disk_name)s' contains invalid characters!") % {"disk_name": ", ".join(str(v) for v in invalid_disks)})
-    return data
+    return data, quota
 
 
 def vm_claims(compute, name):
@@ -192,12 +195,14 @@ def vm_claims(compute, name):
 
 
 def record_vm(op, owner):
-    """The panel's records of the new VM, as the synchronous views wrote them."""
-    vm = Instance.objects.get_or_create(compute_id=op.intent["compute"], uuid=op.intent["uuid"],
-                                        defaults={"name": op.intent["name"]})[0]
-    if owner and op.user_id:
-        UserInstance.objects.get_or_create(instance=vm, user_id=op.user_id,
-                                           defaults={"is_delete": True, "is_change": True, "is_vnc": True})
+    """The panel's records of the new VM, as the synchronous views wrote them;
+    together, as the quota then counts the VM instead of its clone job."""
+    with transaction.atomic():
+        vm = Instance.objects.get_or_create(compute_id=op.intent["compute"], uuid=op.intent["uuid"],
+                                            defaults={"name": op.intent["name"]})[0]
+        if owner and op.user_id:
+            UserInstance.objects.get_or_create(instance=vm, user_id=op.user_id,
+                                               defaults={"is_delete": True, "is_change": True, "is_vnc": True})
 
 
 def vm_outcome(op, owner, volumes):
@@ -458,7 +463,7 @@ class CloneInstance(Kind):
 
     def plan(self, user, params):
         instance = _clonable(user, params["instance"])
-        data = clone_data(user, instance, params["fields"])
+        data, quota = clone_data(user, instance, params["fields"])
         keys = vm_claims(instance.compute, data["name"])
         # the VM's definition is read again when the clone is defined
         keys[claims.instance_key(instance.uuid)] = Claim.EXCLUSIVE
@@ -469,7 +474,7 @@ class CloneInstance(Kind):
         return Plan(claims=keys, resource=claims.instance_key(instance.uuid),
                     label=f"{instance.compute.name}/{instance.name}",
                     intent={"compute": instance.compute_id, "uuid": util.randomUUID(), "name": data["name"],
-                            "clone": data})
+                            "clone": data, "quota": quota})
 
     def precheck(self, op, ctx):
         instance = _clonable(op.user, op.params["instance"])

instances/tests/test_clone_permission.py

@@ -71,7 +71,7 @@ class ClonePermissionTestCase(TestCase):
         with patch("instances.models.wvmInstance") as mock_wvm, patch(
             "instances.views.utils.check_user_quota", return_value=""
         ), patch("instances.utils.get_clone_free_names", return_value=["clone-vm", "clone-vm2"]):
-            proxy = mock_wvm.return_value
+            proxy = self.proxy = mock_wvm.return_value
             proxy.get_disk_devices.return_value = [SRC_DISK]
             proxy.get_net_devices.return_value = [{"mac": "52:54:00:aa:bb:cc"}]
             proxy.get_status.return_value = 5
@@ -263,10 +263,12 @@ class CloneNameTestCase(TestCase):
         with patch("instances.models.wvmInstance") as mock_wvm, patch(
             "instances.views.utils.check_user_quota", return_value=""
         ):
-            proxy = mock_wvm.return_value
+            proxy = self.proxy = mock_wvm.return_value
             proxy.get_disk_devices.return_value = [SRC_DISK]
             proxy.get_net_devices.return_value = [{"mac": "52:54:00:aa:bb:cc"}]
             proxy.get_status.return_value = 5
+            proxy.get_vcpu.return_value = 1
+            proxy.get_memory.return_value = 512
             self.client.post(reverse("instances:clone", args=[self.instance.id]),
                              {"name": name, "submission_key": f"key-names-{name}"})
         op = Operation.objects.first()
@@ -274,6 +276,11 @@ class CloneNameTestCase(TestCase):
 
     def test_a_free_name_from_the_list(self):
         self.assertEqual(self.clone("copy2"), "copy2")
+        # what the clone adds to the user's quota until its VM is recorded: the
+        # source's persistent definition, which it copies
+        self.assertEqual(Operation.objects.get().intent["quota"], {"cpus": 1, "memory": 512, "disk": 10})
+        self.proxy.get_vcpu.assert_called_with(config=True)
+        self.proxy.get_memory.assert_called_with(config=True)
 
     def test_any_other_name_is_refused(self):
         for name in ("copy1", "my-own-name", "", "copy12"):  # taken, not offered, empty, past the ten offered

instances/tests/test_quota.py

@@ -1,7 +1,7 @@
 """check_user_quota counts every VM of the user and refuses an increase it
 cannot verify (fail closed); requests that add nothing are not checked."""
 
-from types import SimpleNamespace
+from contextlib import nullcontext
 from unittest.mock import PropertyMock, patch
 
 from accounts.models import UserAttributes, UserInstance
@@ -14,16 +14,33 @@ from django.urls import reverse
 from instances.models import Instance
 from instances.utils import QUOTA_UNVERIFIED, check_user_quota
 from libvirt import VIR_DOMAIN_PAUSED, VIR_DOMAIN_RUNNING
+from operations.models import Operation
 from vrtManager.util import OperationError
 
 
-def vm_reader(vcpu=2, memory=1024, disk_gib=10):
+def vm_reader(vcpu=2, memory=1024, disk_gib=10, pending=None):
+    """pending: the persistent definition's values where they differ from the running VM's"""
+    live = {"vcpu": vcpu, "memory": memory, "disk_gib": disk_gib}
+    config = {**live, **(pending or {})}
+
+    class Reader:
+        def cached_reads(self):
+            return nullcontext(self)
+
+        def get_vcpu(self, config=False):
+            return values(config)["vcpu"]
+
+        def get_memory(self, config=False):
+            return values(config)["memory"]
+
+        def get_disk_devices(self, config=False):
+            return [{"size": values(config)["disk_gib"] << 30}]
+
+    def values(persistent):
+        return config if persistent else live
+
     def make(*args, **kwargs):
-        return SimpleNamespace(
-            get_vcpu=lambda: vcpu,
-            get_memory=lambda: memory,
-            get_disk_devices=lambda: [{"size": disk_gib << 30}],
-        )
+        return Reader()
 
     return make
 
@@ -82,6 +99,50 @@ class QuotaTests(TestCase):
         # one Compute object asked (its status is cached), not one per VM
         self.assertEqual(len(set(self.status_checks)), 1)
 
+    def test_values_pending_the_next_start_count_when_larger(self):
+        for field, pending, delta, refused in (("vcpu", 3, (0, 1, 0, 0), "cpu"), ("memory", 3000, (0, 0, 1, 0), "memory"),
+                                                ("disk_gib", 40, (0, 0, 0, 1), "disk")):
+            with self.subTest(field=field):
+                Instance.objects.all().delete()
+                for n in range(3):
+                    self.own(self.up, n)  # 6 vCPUs, 3072 MiB, 30 GiB running; quota 8, 8192, 100
+                self.assertEqual(self.check(*delta)[0], "")
+                self.assertEqual(self.check(*delta, reader=vm_reader(pending={field: pending}))[0], refused)
+        # a smaller pending value: the running VM still has the larger one
+        self.assertEqual(self.check(0, 2, 0, 0, reader=vm_reader(pending={"vcpu": 1}))[0], "")
+
+    def clone_job(self, state=Operation.QUEUED, uuid="abababab-0000-0000-0000-000000000001"):
+        return Operation.objects.create(kind="instance.clone", user=self.user, username=self.user.username, state=state,
+                                        submission_key=uuid, resource="instance:x", resource_label="up/vm",
+                                        intent={"uuid": uuid, "quota": {"cpus": 4, "memory": 1024, "disk": 10}})
+
+    def test_a_clone_counts_until_its_vm_is_recorded(self):
+        for n in range(2):
+            self.own(self.up, n)  # 4 vCPUs of 8
+        self.assertEqual(self.check(0, 1, 0, 0)[0], "")
+        op = self.clone_job()
+        self.assertEqual(self.check(0, 1, 0, 0)[0], "cpu")
+        for state in (Operation.RUNNING, Operation.UNCERTAIN):
+            with self.subTest(state=state):
+                Operation.objects.filter(pk=op.pk).update(state=state)
+                self.assertEqual(self.check(0, 1, 0, 0)[0], "cpu")
+        for state in (Operation.FAILED, Operation.CANCELLED, Operation.SUCCEEDED):
+            with self.subTest(state=state):
+                Operation.objects.filter(pk=op.pk).update(state=state)
+                self.assertEqual(self.check(0, 1, 0, 0)[0], "")
+        # its VM recorded while the job still runs: the VM counts (2 vCPUs here), not the job
+        Operation.objects.filter(pk=op.pk).update(state=Operation.RUNNING)
+        vm = Instance.objects.create(compute=self.up, name="clone", uuid=op.intent["uuid"])
+        UserInstance.objects.create(user=self.user, instance=vm)
+        self.assertEqual(self.check(0, 2, 0, 0)[0], "")
+        self.assertEqual(self.check(0, 3, 0, 0)[0], "cpu")
+
+    def test_queued_clones_count_as_instances(self):
+        UserAttributes.objects.filter(user=self.user).update(max_instances=2)
+        self.own(self.up, 1)
+        self.clone_job()
+        self.assertEqual(self.check(1, 0, 0, 0)[0], "instance")
+
     def test_a_user_over_quota_can_still_shrink(self):
         for n in range(5):
             self.own(self.up, n)  # 10 vCPUs used of 8

instances/utils.py

@@ -10,6 +10,7 @@ from django.db import transaction
 from django.utils.translation import gettext_lazy as _
 from libvirt import libvirtError
 from logs.audit import reason, write
+from operations.models import Operation
 from vrtManager.instance import wvmInstance, wvmInstances
 
 from .models import Instance
@@ -42,6 +43,11 @@ def get_clone_free_names(size=10):
     return free_names
 
 
+def disk_gib(disks):
+    """The size of the disks in whole GiB each, as the quota counts them."""
+    return sum(int(disk["size"] or 0) >> 30 for disk in disks)
+
+
 # check_user_quota could not read every VM of the user: an increase is refused
 QUOTA_UNVERIFIED = "unverified"
 
@@ -68,7 +74,13 @@ def check_user_quota(user, instance, cpu, memory, disk_size):
     user_instances = UserInstance.objects.filter(user=user, instance__is_template=False).select_related(
         "instance__compute"
     )
-    instance += user_instances.count()
+    # a clone counts from its job's admission until its VM is recorded
+    clones = [
+        op.intent["quota"] for op in Operation.objects.filter(
+            user=user, kind="instance.clone", state__in=Operation.ACTIVE + (Operation.UNCERTAIN,))
+        if "quota" in op.intent and not Instance.objects.filter(uuid=op.intent["uuid"]).exists()
+    ]
+    instance += user_instances.count() + len(clones)
     if ua.max_instances >= 0 and instance > ua.max_instances:
         msg = "instance"
         if quota_debug:
@@ -77,6 +89,10 @@ def check_user_quota(user, instance, cpu, memory, disk_size):
     if ua.max_cpus < 0 and ua.max_memory < 0 and ua.max_disk_size < 0:
         return msg
 
+    for clone in clones:
+        cpu += clone["cpus"]
+        memory += clone["memory"]
+        disk_size += clone["disk"]
     computes = {}  # status is cached per object: check each host once
     for usr_inst in user_instances:
         vm = usr_inst.instance
@@ -92,11 +108,11 @@ def check_user_quota(user, instance, cpu, memory, disk_size):
                 vm.name,
                 uuid=vm.uuid,
             )
-            cpu += int(conn.get_vcpu())
-            memory += int(conn.get_memory())
-            for disk in conn.get_disk_devices():
-                if disk["size"]:
-                    disk_size += int(disk["size"]) >> 30
+            # a running VM counts what it has, or what it gets at its next start if that is more
+            with conn.cached_reads():
+                cpu += max(conn.get_vcpu(), conn.get_vcpu(config=True))
+                memory += max(conn.get_memory(), conn.get_memory(config=True))
+                disk_size += max(disk_gib(conn.get_disk_devices()), disk_gib(conn.get_disk_devices(config=True)))
         except libvirtError:
             return QUOTA_UNVERIFIED
 

instances/views.py

@@ -2209,8 +2209,8 @@ def set_template(request, pk):
         messages.error(request, _("Shut the VM down to change whether it is a template."))
         return _back(request, pk, "options")
     if instance.is_template and not is_template and not request.user.is_superuser:
-        disk_sum = sum(int(disk.get("size") or 0) >> 30 for disk in instance.disks)
-        quota_msg = utils.check_user_quota(request.user, 1, instance.vcpu, instance.memory, disk_sum)
+        quota_msg = utils.check_user_quota(request.user, 1, instance.vcpu, instance.memory,
+                                           utils.disk_gib(instance.disks))
         if quota_msg:
             messages.error(request, quota_refused(quota_msg, _(
                 "User %(quota_msg)s quota reached, '%(instance_name)s' stays a template!"

vrtManager/instance.py

@@ -713,8 +713,8 @@ class wvmInstance(wvmConnect):
 
         return vcpus
 
-    def get_memory(self):
-        mem = util.get_xml_path(self._XMLDesc(0), "/domain/memory")
+    def get_memory(self, config=False):
+        mem = util.get_xml_path(self._XMLDesc(PERSISTENT_XML if config else 0), "/domain/memory")
         return int(mem) // 1024
 
     def get_cur_memory(self):