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