From 37b87839c1e050272273578e621686dc0237b90b Mon Sep 17 00:00:00 2001 From: catborise Date: Wed, 4 Nov 2026 20:34:41 +0300 Subject: [PATCH] perf(computes): a VM list load writes nothing while the hosts match the database Every VM list load reconciles each compute with libvirt. Even when nothing had changed, that took the compute's claims and a transaction with SELECT FOR UPDATE (and the ownership advisory lock on PostgreSQL): about 11 queries a compute, 4 of them writes. On SQLite each load queued behind every other write of the app. The reconcile now first compares libvirt's VM list with the database, without a claim or a transaction, and stops when they match and no removed record has expired. Otherwise it runs as before, reading libvirt again under its claims, so two reconciles still cannot commit in reverse order. With two matching hosts a list load goes from 38 queries (8 writes) to 18 (none). --- computes/tests/test_ownership.py | 11 +++++ computes/tests/test_reconciliation.py | 10 +++++ computes/tests/test_views.py | 23 +++++++--- computes/utils.py | 63 +++++++++++++-------------- 4 files changed, 69 insertions(+), 38 deletions(-) diff --git a/computes/tests/test_ownership.py b/computes/tests/test_ownership.py index 78c5bc3d..56ba0890 100644 --- a/computes/tests/test_ownership.py +++ b/computes/tests/test_ownership.py @@ -121,6 +121,17 @@ class OwnershipTestCase(TestCase): self.assertFalse(InstanceTombstone.objects.exists()) self.assertIn(("-", "-", f"VM vm ({UUID}): removed record expired"), self.rows()) + @override_settings(INSTANCE_OWNERSHIP_RETENTION_DAYS=30) + def test_a_removed_record_expires_while_nothing_changes(self): + self.owned_vm(self.host_a) + refresh_instance_database(self.host_a) + InstanceTombstone.objects.update(removed=timezone.now() - timedelta(days=31)) + + refresh_instance_database(self.host_b) # its VMs match the database + + self.assertFalse(InstanceTombstone.objects.exists()) + self.assertIn(("-", "-", f"VM vm ({UUID}): removed record expired"), self.rows()) + def test_deleted_users_are_skipped(self): self.owned_vm(self.host_a) refresh_instance_database(self.host_a) diff --git a/computes/tests/test_reconciliation.py b/computes/tests/test_reconciliation.py index 1ca9c039..a2f39660 100644 --- a/computes/tests/test_reconciliation.py +++ b/computes/tests/test_reconciliation.py @@ -2,6 +2,7 @@ from unittest.mock import MagicMock, PropertyMock, patch from django.contrib.auth import get_user_model from django.db import connection from django.test import TestCase, TransactionTestCase +from django.test.utils import CaptureQueriesContext from accounts.models import UserInstance from computes.models import Compute @@ -171,6 +172,15 @@ class ReconciliationTransactionTestCase(TransactionTestCase): self.assertEqual(seen, {"status": False, "list": False}) self.assertTrue(Instance.objects.filter(compute=self.compute, name="vm-tx").exists()) + def test_a_compute_that_matches_the_database_writes_nothing(self): + Instance.objects.create(compute=self.compute, name="vm-same", uuid="99999999-9999-9999-9999-999999999999") + self.proxy(lambda: [MockDomain(name="vm-same", uuid_str="99999999-9999-9999-9999-999999999999")]) + self.compute.__dict__["status"] = True + with CaptureQueriesContext(connection) as queries: + refresh_instance_database(self.compute) + + self.assertEqual([q["sql"].split()[0] for q in queries.captured_queries], ["SELECT", "SELECT"]) + def test_compute_deleted_while_listing_gets_no_instances(self): def list_domains(): Compute.objects.filter(pk=self.compute.pk).delete() diff --git a/computes/tests/test_views.py b/computes/tests/test_views.py index 642d0a65..8b1b4ebc 100644 --- a/computes/tests/test_views.py +++ b/computes/tests/test_views.py @@ -268,13 +268,19 @@ class ComputeConcurrencyTestCase(TransactionTestCase): claims.admit({claims.compute_key(compute.id): Claim.EXCLUSIVE}, holder="other") refresh_instance_database(compute) self.assertEqual(Instance.objects.filter(compute=compute).count(), 1) - mock_proxy.wvm.listAllDomains.assert_not_called() + mock_proxy.wvm.listAllDomains.assert_called_once() # the pre-check's read; no reconcile def _reconcile_compute(self, name, domains): + """A compute whose database differs from libvirt (a VM libvirt no + longer lists), so a refresh reconciles it: the listing that domains + answers second is the one inside the reconcile, the first is the + pre-check's.""" from unittest.mock import MagicMock + from instances.models import Instance compute = Compute.objects.create(name=name, hostname="192.0.2.10", login="test", password="pwd", type=CONN_SOCKET) + Instance.objects.create(compute=compute, name="wvc-test-gone", uuid="wvc-test-gone-uuid") compute.proxy = MagicMock() compute.proxy.wvm.listAllDomains.side_effect = domains compute.status = True @@ -297,10 +303,14 @@ class ComputeConcurrencyTestCase(TransactionTestCase): finally: connection.close() + calls = [] + def list_domains(): - thread = threading.Thread(target=vm_operation) - thread.start() - thread.join(10) + calls.append("list") + if len(calls) == 2: # inside the reconcile + thread = threading.Thread(target=vm_operation) + thread.start() + thread.join(10) return [] compute = self._reconcile_compute("reconcile-shared-test", list_domains) @@ -324,7 +334,7 @@ class ComputeConcurrencyTestCase(TransactionTestCase): def list_domains(): calls.append("list") - if len(calls) == 1: + if len(calls) == 2: # inside the reconcile thread = threading.Thread(target=second_reconcile) thread.start() thread.join(10) @@ -332,7 +342,8 @@ class ComputeConcurrencyTestCase(TransactionTestCase): compute = self._reconcile_compute("reconcile-twice-test", list_domains) refresh_instance_database(compute) - self.assertEqual(calls, ["list"]) + # the second one's pre-check, then no listing of its own + self.assertEqual(calls, ["list", "list", "list"]) def test_a_reconcile_runs_while_a_job_holds_the_compute_shared(self): from computes.utils import refresh_instance_database diff --git a/computes/utils.py b/computes/utils.py index ff4b61d0..2efd7393 100644 --- a/computes/utils.py +++ b/computes/utils.py @@ -129,6 +129,16 @@ def _restore(inst): _log(inst, "owners restored") +def _host_domains(compute): + """{uuid: name} of the compute's VMs, or None if libvirt cannot list them.""" + try: + return {d.UUIDString(): d.name() for d in compute.proxy.wvm.listAllDomains()} + except Exception as e: + logger.warning("Failed to list the domains of compute %s (%s): %s", + getattr(compute, "name", compute), getattr(compute, "hostname", ""), e) + return None + + def refresh_instance_database(compute): """ Synchronizes the WebVirtCloud database with libvirt domain state. @@ -145,45 +155,35 @@ def refresh_instance_database(compute): compute_pk = getattr(compute, "pk", getattr(compute, "id", None)) if not compute_pk: return + # libvirt is asked before any transaction: a silent host can keep a call + # waiting for 30 s, and on SQLite an open transaction makes every other + # write of the app fail meanwhile. + if hasattr(compute, "status") and compute.status is False: + return + host_domains = _host_domains(compute) + if host_domains is None: + return + # What nearly every VM list load finds: the database matches libvirt and no + # removed record has expired. Nothing to write: no claim, no transaction. + retention = getattr(settings, "INSTANCE_OWNERSHIP_RETENTION_DAYS", 30) + cutoff = timezone.now() - timedelta(days=retention) + if (dict(Instance.objects.filter(compute_id=compute_pk).values_list("uuid", "name")) == host_domains + and not InstanceTombstone.objects.filter(removed__lt=cutoff).exists()): + return try: # The compute shared: VM operations and jobs on it go on; changes of # its VM set (create, clone, delete, migration, define) hold it # exclusive and stay out until the end. One reconcile of a compute at - # a time: two would read libvirt at different times and could commit - # in reverse order, an older listing undoing a newer one. + # a time, and libvirt read again inside it: two would read libvirt at + # different times and could commit in reverse order, an older listing + # undoing a newer one. with claims.hold({claims.compute_key(compute_pk): Claim.SHARED, claims.reconcile_key(compute_pk): Claim.EXCLUSIVE}): - # libvirt is asked before the transaction: a silent host can keep a - # call waiting for 30 s, and on SQLite an open transaction makes every - # other write of the app fail meanwhile. - if hasattr(compute, "status") and compute.status is False: + host_domains = _host_domains(compute) + if host_domains is None: return - try: - domains = compute.proxy.wvm.listAllDomains() - except Exception as e: - logger.warning( - "Failed to retrieve domains from compute %s (%s): %s", - getattr(compute, "name", compute), - getattr(compute, "hostname", ""), - e, - ) - return - - # Map libvirt UUIDs to current domain names - host_domains = {} - for d in domains: - try: - host_domains[d.UUIDString()] = d.name() - except Exception as e: - logger.warning( - "Error reading domain metadata from compute %s: %s", - getattr(compute, "name", compute), - e, - ) - return - with transaction.atomic(): lock_ownership() # the compute may have been deleted while libvirt was asked @@ -210,8 +210,7 @@ def refresh_instance_database(compute): # 3. Instances created out-of-band. A UUID that also exists on another # compute is a copy and gets no owners; otherwise a tombstone's # ownership is restored. - retention = getattr(settings, "INSTANCE_OWNERSHIP_RETENTION_DAYS", 30) - expired = InstanceTombstone.objects.filter(removed__lt=timezone.now() - timedelta(days=retention)) + expired = InstanceTombstone.objects.filter(removed__lt=cutoff) for t in expired: write("-", "-", "", f"VM {t.name} ({t.uuid}): removed record expired", True) expired.delete()