diff --git a/instances/jobs.py b/instances/jobs.py index 80f4d9c2..ca5df964 100644 --- a/instances/jobs.py +++ b/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"]) diff --git a/instances/tests/test_clone_permission.py b/instances/tests/test_clone_permission.py index 3f189b8e..574f3de2 100644 --- a/instances/tests/test_clone_permission.py +++ b/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 diff --git a/instances/tests/test_quota.py b/instances/tests/test_quota.py index bd7093ef..5d18ef3b 100644 --- a/instances/tests/test_quota.py +++ b/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 diff --git a/instances/utils.py b/instances/utils.py index 6abbf570..293506bb 100644 --- a/instances/utils.py +++ b/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 diff --git a/instances/views.py b/instances/views.py index 1b28ee6f..a4b40a1c 100755 --- a/instances/views.py +++ b/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!" diff --git a/vrtManager/instance.py b/vrtManager/instance.py index 0b579612..4ecd4c34 100644 --- a/vrtManager/instance.py +++ b/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):