From fb3f6865324ec90d4549624d6ed4b034b12cffd5 Mon Sep 17 00:00:00 2001 From: catborise Date: Mon, 5 Oct 2026 12:26:57 +0300 Subject: [PATCH] fix(instances): the quota counts every VM and refuses what it cannot verify check_user_quota added up the vCPUs, memory and disks of the user's VMs, but skipped a VM whose host failed a TCP probe. A host that was down therefore let users exceed their quota, and the probe always failed for ssh computes named by an ssh_config alias or reached through ProxyJump, so their VMs were never counted. A VM that could not be read turned the page into the error page instead. - Hosts are checked with Compute.status, the connection WebVirtCloud uses, once per host. A VM that cannot be read (host down, VM gone) makes the usage unknown: clone and resize up are refused with their own message (fail closed). - A request that adds nothing is not checked, so a user over quota can still shrink a VM. Users without limits, and an exceeded instance count (from the database), need no host either. - Memory resize of a running VM changes only the current memory, but the quota was charged with the submitted maximum. It is now charged with no change, and resize_mem gets the current maximum, so a VM that shuts down meanwhile cannot take an unchecked one. - The VM page no longer computes the quota on every load (it read every VM of the user on every host for a "quota reached" banner). The limit is reported when a change is refused. The test that expected a down host to pass the quota now expects it to be refused. Checked against the test hosts: with a VM behind an ssh_config alias the previous code counted 29 vCPUs, the new one 31; with a VM on an unreachable host the previous code allowed +1 vCPU, the new one refuses it after 5.1 s and allows -1 at once. The new tests fail on the previous code, except the ones guarding unchanged behaviour. --- computes/test_quota_down_host.py | 7 +- instances/templates/instance.html | 5 - instances/test_quota.py | 179 ++++++++++++++++++++++++++++++ instances/utils.py | 54 ++++++--- instances/views.py | 35 ++++-- 5 files changed, 245 insertions(+), 35 deletions(-) create mode 100644 instances/test_quota.py diff --git a/computes/test_quota_down_host.py b/computes/test_quota_down_host.py index 933388f4..db6be4be 100644 --- a/computes/test_quota_down_host.py +++ b/computes/test_quota_down_host.py @@ -6,11 +6,12 @@ from computes.models import Compute from django.contrib.auth import get_user_model from django.test import TestCase from instances.models import Instance -from instances.utils import check_user_quota +from instances.utils import QUOTA_UNVERIFIED, check_user_quota class QuotaWithDownHostTestCase(TestCase): - def test_vm_on_an_unreachable_host_does_not_break_the_quota_check(self): + def test_vm_on_an_unreachable_host_makes_the_quota_unverified(self): + # its usage is unknown, so an increase is refused (fail closed), without an error page sock = socket.socket() sock.bind(("127.0.0.1", 0)) port = sock.getsockname()[1] @@ -22,4 +23,4 @@ class QuotaWithDownHostTestCase(TestCase): UserInstance.objects.create(user=user, instance=vm) get_settings() # app settings are normally loaded by middleware - self.assertEqual(check_user_quota(user, 1, 1, 128, 1), "") + self.assertEqual(check_user_quota(user, 1, 1, 128, 1), QUOTA_UNVERIFIED) diff --git a/instances/templates/instance.html b/instances/templates/instance.html index 21625131..03ed0baf 100644 --- a/instances/templates/instance.html +++ b/instances/templates/instance.html @@ -67,11 +67,6 @@ {% bs_icon 'repeat'%} - {% if user_quota_msg %} -
- {{ user_quota_msg|capfirst }} {% trans "quota reached" %}. -
- {% endif %}
diff --git a/instances/test_quota.py b/instances/test_quota.py new file mode 100644 index 00000000..28d81d6c --- /dev/null +++ b/instances/test_quota.py @@ -0,0 +1,179 @@ +"""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 unittest.mock import patch + +from accounts.models import UserAttributes, UserInstance +from appsettings.settings import get_settings +from computes.models import Compute +from django.contrib.auth import get_user_model +from django.contrib.messages import get_messages +from django.test import TestCase +from django.urls import reverse +from instances.models import Instance +from instances.utils import QUOTA_UNVERIFIED, check_user_quota +from libvirt import VIR_DOMAIN_RUNNING +from vrtManager.util import OperationError + + +def vm_reader(vcpu=2, memory=1024, disk_gib=10): + def make(*args, **kwargs): + return SimpleNamespace( + get_vcpu=lambda: vcpu, + get_memory=lambda: memory, + get_disk_devices=lambda: [{"size": disk_gib << 30}], + ) + + return make + + +class QuotaTests(TestCase): + def setUp(self): + get_settings() # app settings are normally loaded by middleware + self.user = get_user_model().objects.create_user("quota-user", password="pw") + UserAttributes.objects.create(user=self.user, max_instances=5, max_cpus=8, max_memory=8192, max_disk_size=100) + self.up = Compute.objects.create(name="up", hostname="wvc-alias", login="root", password="", type=2) + self.down = Compute.objects.create(name="down", hostname="192.0.2.1", login="u", password="p", type=1) + self.status_checks = [] + + def own(self, compute, n): + vm = Instance.objects.create(compute=compute, name=f"vm-{compute.name}-{n}", uuid=f"{n:08d}-0000-0000-0000-{compute.pk:012d}") + UserInstance.objects.create(user=self.user, instance=vm) + return vm + + def check(self, *delta, reader=None): + test = self + + def status(compute): + test.status_checks.append(id(compute)) + return compute.pk != test.down.pk + + reader = reader or vm_reader() + with patch.object(Compute, "status", property(status)), patch( + "instances.utils.wvmInstance", side_effect=reader + ) as wvm: + result = check_user_quota(self.user, *delta) + return result.split(" ")[0], wvm.call_count # without the QUOTA_DEBUG detail + + def test_vms_on_a_host_without_a_tcp_route_are_counted(self): + # an ssh_config alias: the old TCP probe failed and skipped these VMs + for n in range(4): + self.own(self.up, n) + self.assertEqual(self.check(0, 1, 0, 0), ("cpu", 4)) + + def test_a_host_that_is_down_refuses_an_increase(self): + self.own(self.up, 1) + self.own(self.down, 2) + self.assertEqual(self.check(0, 1, 0, 0)[0], QUOTA_UNVERIFIED) + + def test_a_vm_that_cannot_be_read_refuses_an_increase(self): + self.own(self.up, 1) + + def gone(*args, **kwargs): + raise OperationError("Domain not found") + + self.assertEqual(self.check(0, 0, 512, 0, reader=gone)[0], QUOTA_UNVERIFIED) + + def test_each_host_is_checked_once(self): + for n in range(3): + self.own(self.up, n) + self.check(0, 1, 0, 0) + # one Compute object asked (its status is cached), not one per VM + self.assertEqual(len(set(self.status_checks)), 1) + + 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 + # (the TCP probe of the previous code would skip these VMs and hide the case) + with patch("instances.utils.connection_manager.host_is_up", return_value=True): + self.assertEqual(self.check(0, -1, 0, 0), ("", 0)) + self.assertEqual(self.check(0, 0, -512, 0), ("", 0)) + + def test_unlimited_users_are_not_checked(self): + UserAttributes.objects.filter(user=self.user).update(max_instances=-1, max_cpus=-1, max_memory=0, max_disk_size=-1) + self.own(self.down, 1) + self.assertEqual(self.check(1, 4, 4096, 50), ("", 0)) + + def test_only_an_instance_limit_needs_no_host(self): + UserAttributes.objects.filter(user=self.user).update(max_instances=2, max_cpus=-1, max_memory=-1, max_disk_size=-1) + self.own(self.down, 1) + self.assertEqual(self.check(1, 4, 4096, 50), ("", 0)) + self.own(self.up, 2) + self.assertEqual(self.check(1, 4, 4096, 50), ("instance", 0)) + + def test_instance_limit_is_checked_before_any_host(self): + for n in range(5): + self.own(self.down, n) + self.assertEqual(self.check(1, 0, 0, 0), ("instance", 0)) + + def test_templates_are_not_counted(self): + for n in range(4): + vm = self.own(self.up, n) + Instance.objects.filter(pk=vm.pk).update(is_template=True) + self.assertEqual(self.check(0, 2, 0, 0), ("", 3)) + + +class QuotaViewTests(TestCase): + """The resize views turn the result into a message and charge only what changes.""" + + def setUp(self): + get_settings() + self.user = get_user_model().objects.create_user("quota-view-user", password="pw") + self.client.force_login(self.user) + compute = Compute.objects.create(name="qv", hostname="10.0.0.1", login="u", password="p", type=1) + self.vm = Instance.objects.create(compute=compute, name="qv-vm", uuid="12121212-0000-0000-0000-000000000000") + UserInstance.objects.create(user=self.user, instance=self.vm, is_change=True) + + def post(self, view, data, quota="", status=VIR_DOMAIN_RUNNING): + with patch("instances.views.utils.check_user_quota", return_value=quota) as check, patch( + "instances.models.wvmInstance" + ) as wvm: + wvm.return_value.get_vcpu.return_value = 1 + wvm.return_value.get_memory.return_value = 2048 + wvm.return_value.get_cur_memory.return_value = 2048 + wvm.return_value.get_status.return_value = status + response = self.client.post(reverse(view, args=[self.vm.id]), data) + messages = [str(m) for m in get_messages(response.wsgi_request)] + return messages, check, wvm.return_value + + def test_unverified_quota_has_its_own_message(self): + messages, _, proxy = self.post("instances:resizevm_cpu", {"vcpu": 2, "cur_vcpu": 2}, quota=QUOTA_UNVERIFIED) + self.assertEqual(len(messages), 1) + self.assertIn("a host of your virtual machines cannot be reached", messages[0]) + self.assertNotIn("quota reached", messages[0]) + proxy.resize_cpu.assert_not_called() + + def test_running_vm_memory_charges_no_maximum_change(self): + # a running VM keeps its maximum: resize_mem changes the current memory only + _, check, proxy = self.post("instances:resize_memory", {"memory": 8192, "cur_memory": 1024}) + self.assertEqual(check.call_args.args[1:], (0, 0, 0, 0)) + # the maximum passed is the current one: if the VM shut down meanwhile, + # resize_mem would write it, and it must not be the unchecked 8192 + proxy.resize_mem.assert_called_once_with("1024", 2048) + + def test_shut_off_vm_memory_charges_the_maximum_change(self): + _, check, _ = self.post("instances:resize_memory", {"memory": 4096, "cur_memory": 1024}, status=5) + self.assertEqual(check.call_args.args[1:], (0, 0, 2048, 0)) + + +class DetailPageQuotaTests(TestCase): + def test_the_detail_page_does_not_read_every_vm_for_the_quota(self): + get_settings() + user = get_user_model().objects.create_user("quota-detail-user", password="pw") + compute = Compute.objects.create(name="qd", hostname="10.0.0.1", login="u", password="p", type=1) + vm = Instance.objects.create(compute=compute, name="qd-vm", uuid="34343434-0000-0000-0000-000000000000") + UserInstance.objects.create(user=user, instance=vm) + self.client.force_login(user) + with patch("instances.models.wvmInstance") as wvm, patch( + "instances.views.utils.check_user_quota", side_effect=AssertionError("quota checked on GET") + ): + proxy = wvm.return_value + proxy.get_memory.return_value = 1024 + proxy.get_cur_memory.return_value = 1024 + proxy.get_max_memory.return_value = 4096 + for name in ("get_networks", "get_ifaces", "get_storages", "get_disk_devices", "get_media_devices", "get_net_devices"): + getattr(proxy, name).return_value = [] + proxy.get_status.return_value = 5 + response = self.client.get(reverse("instances:instance", args=[vm.id])) + self.assertEqual(response.status_code, 200) diff --git a/instances/utils.py b/instances/utils.py index 1e84dfe9..4f846530 100644 --- a/instances/utils.py +++ b/instances/utils.py @@ -9,6 +9,7 @@ from computes.utils import libvirt_compute_lock from django.conf import settings from django.db import transaction from django.utils.translation import gettext_lazy as _ +from libvirt import libvirtError from vrtManager import util from vrtManager.connection import connection_manager from vrtManager.instance import wvmInstance, wvmInstances @@ -43,40 +44,63 @@ def get_clone_free_names(size=10): return free_names +# check_user_quota could not read every VM of the user: an increase is refused +QUOTA_UNVERIFIED = "unverified" + + def check_user_quota(user, instance, cpu, memory, disk_size): + """ + Which quota the user would exceed by adding these amounts (instances, + vCPUs, memory MiB, disk GiB) to their VMs, or "". QUOTA_UNVERIFIED if a + VM that counts cannot be read (its host is down, the VM is gone): the + usage is unknown, so an increase is refused. + """ ua, attributes_created = UserAttributes.objects.get_or_create(user=user) msg = "" if user.is_superuser: return msg + # adding nothing (a shrink) cannot exceed a quota + if instance <= 0 and cpu <= 0 and memory <= 0 and disk_size <= 0: + return msg quota_debug = app_settings.QUOTA_DEBUG - user_instances = UserInstance.objects.filter(user=user, instance__is_template=False) + user_instances = UserInstance.objects.filter(user=user, instance__is_template=False).select_related( + "instance__compute" + ) instance += user_instances.count() + if ua.max_instances > 0 and instance > ua.max_instances: + msg = "instance" + if quota_debug: + msg += f" ({instance} > {ua.max_instances})" + return msg + if ua.max_cpus <= 0 and ua.max_memory <= 0 and ua.max_disk_size <= 0: + return msg + + computes = {} # status is cached per object: check each host once for usr_inst in user_instances: - if connection_manager.host_is_up( - usr_inst.instance.compute.type, - usr_inst.instance.compute.hostname, - ): + vm = usr_inst.instance + compute = computes.setdefault(vm.compute_id, vm.compute) + if compute.status is not True: + return QUOTA_UNVERIFIED + try: conn = wvmInstance( - usr_inst.instance.compute.hostname, - usr_inst.instance.compute.login, - usr_inst.instance.compute.password, - usr_inst.instance.compute.type, - usr_inst.instance.name, - uuid=usr_inst.instance.uuid, + compute.hostname, + compute.login, + compute.password, + compute.type, + 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 + except libvirtError: + return QUOTA_UNVERIFIED - if ua.max_instances > 0 and instance > ua.max_instances: - msg = "instance" - if quota_debug: - msg += f" ({instance} > {ua.max_instances})" if ua.max_cpus > 0 and cpu > ua.max_cpus: msg = "cpu" if quota_debug: diff --git a/instances/views.py b/instances/views.py index c8750db8..cd7dad54 100755 --- a/instances/views.py +++ b/instances/views.py @@ -34,6 +34,7 @@ from libvirt import (VIR_DOMAIN_UNDEFINE_KEEP_NVRAM, VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, VIR_DOMAIN_UNDEFINE_NVRAM, VIR_DOMAIN_START_PAUSED, + VIR_DOMAIN_RUNNING, libvirtError) from logs.views import addlogmsg from vrtManager import util @@ -118,7 +119,6 @@ def instance(request, pk): if instance.cur_memory not in memory_range: insort(memory_range, instance.cur_memory) clone_free_names = utils.get_clone_free_names() - user_quota_msg = utils.check_user_quota(request.user, 0, 0, 0, 0) default_bus = app_settings.INSTANCE_VOLUME_DEFAULT_BUS default_io = app_settings.INSTANCE_VOLUME_DEFAULT_IO @@ -403,6 +403,14 @@ def serialize_instance_mutation(func): return wrapper +def quota_refused(quota_msg, refused): + """The message for a refused change: refused is the action's own + 'User %(quota_msg)s quota reached, ...' text, already formatted.""" + if quota_msg == utils.QUOTA_UNVERIFIED: + return _("The quota cannot be checked because a host of your virtual machines cannot be reached. Nothing was changed.") + return refused + + def serialize_user_quota(func): """Outermost lock for quota-checked changes: concurrent requests of one user (also on different computes) must not pass the check together.""" @@ -766,12 +774,12 @@ def resizevm_cpu(request, pk): request.user, 0, int(new_vcpu) - vcpu, 0, 0 ) if not request.user.is_superuser and quota_msg: - msg = _( + msg = quota_refused(quota_msg, _( "User %(quota_msg)s quota reached, cannot resize CPU of '%(instance_name)s'!" ) % { "quota_msg": quota_msg, "instance_name": instance.name, - } + }) messages.error(request, msg) else: cur_vcpu = new_cur_vcpu @@ -804,16 +812,19 @@ def resize_memory(request, pk): new_cur_memory_custom = request.POST.get("cur_memory_custom", "") if new_cur_memory_custom: new_cur_memory = new_cur_memory_custom - quota_msg = utils.check_user_quota( - request.user, 0, 0, int(new_memory) - memory, 0 - ) + # The quota charges the maximum. A running VM keeps it (resize_mem changes + # only the current memory there); pass the current maximum, so a VM that + # shuts down meanwhile does not get an unchecked one. + if instance.proxy.get_status() == VIR_DOMAIN_RUNNING: + new_memory = memory + quota_msg = utils.check_user_quota(request.user, 0, 0, int(new_memory) - memory, 0) if not request.user.is_superuser and quota_msg: - msg = _( + msg = quota_refused(quota_msg, _( "User %(quota_msg)s quota reached, cannot resize memory of '%(instance_name)s'!" ) % { "quota_msg": quota_msg, "instance_name": instance.name, - } + }) messages.error(request, msg) else: instance.proxy.resize_mem(new_cur_memory, new_memory) @@ -859,12 +870,12 @@ def resize_disk(request, pk): request.user, 0, 0, 0, disk_new_sum - disk_sum ) if not request.user.is_superuser and quota_msg: - msg = _( + msg = quota_refused(quota_msg, _( "User %(quota_msg)s quota reached, cannot resize disks of '%(instance_name)s'!" ) % { "quota_msg": quota_msg, "instance_name": instance.name, - } + }) messages.error(request, msg) else: instance.proxy.resize_disk(disks_new) @@ -1789,12 +1800,12 @@ def clone(request, pk): ] if not request.user.is_superuser and quota_msg: - msg = _( + msg = quota_refused(quota_msg, _( "User '%(quota_msg)s' quota reached, cannot create '%(clone_name)s'!" ) % { "quota_msg": quota_msg, "clone_name": clone_data["name"], - } + }) messages.error(request, msg) elif check_instance: msg = _("Instance '%(clone_name)s' already exists!") % {