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

@@ -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)

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()

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

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()