From fbd780af021c7974db614b99c763a596fe11f994 Mon Sep 17 00:00:00 2001 From: catborise Date: Mon, 5 Oct 2026 10:47:35 +0300 Subject: [PATCH] fix(computes): instance sync asks libvirt outside the DB transaction refresh_instance_database opened the transaction first and then asked libvirt for the domain list. A host that goes silent on an established connection keeps that call waiting until keepalive gives up (30 s). On SQLite, the default database, the open transaction holds a read lock, so every other write of the app failed meanwhile with "database is locked", logins (session writes) included. Reproduced in the test setup: with a relay to a TCP compute frozen after connecting, three session writes during the 30 s refresh each failed after 5 s. - The connection check and listAllDomains run before the transaction, still under the compute lock, so create, delete and migration of the compute stay excluded as before. - The transaction only applies the changes. It checks the compute row first: a compute deleted while libvirt was asked gets no instances (this ended in a foreign key error before). A failed listing still leaves the database untouched. With the change, the same reproduction writes the sessions at once and keeps the instances. Both new tests fail on the previous code. --- computes/test_reconciliation.py | 47 +++++++++++++++++++++++++++++++-- computes/utils.py | 47 ++++++++++++++++++--------------- 2 files changed, 71 insertions(+), 23 deletions(-) diff --git a/computes/test_reconciliation.py b/computes/test_reconciliation.py index 082b575a..1ca9c039 100644 --- a/computes/test_reconciliation.py +++ b/computes/test_reconciliation.py @@ -1,6 +1,7 @@ -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, PropertyMock, patch from django.contrib.auth import get_user_model -from django.test import TestCase +from django.db import connection +from django.test import TestCase, TransactionTestCase from accounts.models import UserInstance from computes.models import Compute @@ -138,3 +139,45 @@ class InstanceReconciliationTestCase(TestCase): with patch("computes.utils.refresh_instance_database") as mock_refresh: refr(self.compute) mock_refresh.assert_called_once_with(self.compute) + + +class ReconciliationTransactionTestCase(TransactionTestCase): + """libvirt is asked outside the DB transaction: on SQLite an open transaction + makes every other write of the app fail while a silent host is waited on.""" + + def setUp(self): + self.compute = Compute.objects.create(name="tx-compute", hostname="127.0.0.1", login="root", password="", type=1) + + def proxy(self, list_domains): + mock_proxy = MagicMock() + mock_proxy.wvm.listAllDomains.side_effect = list_domains + self.compute.__dict__["proxy"] = mock_proxy + + def test_libvirt_is_called_outside_the_transaction(self): + seen = {} + + def status(): + seen["status"] = connection.in_atomic_block + return True + + def list_domains(): + seen["list"] = connection.in_atomic_block + return [MockDomain(name="vm-tx", uuid_str="77777777-7777-7777-7777-777777777777")] + + self.proxy(list_domains) + with patch.object(Compute, "status", new_callable=PropertyMock, side_effect=status): + refresh_instance_database(self.compute) + + self.assertEqual(seen, {"status": False, "list": False}) + self.assertTrue(Instance.objects.filter(compute=self.compute, name="vm-tx").exists()) + + def test_compute_deleted_while_listing_gets_no_instances(self): + def list_domains(): + Compute.objects.filter(pk=self.compute.pk).delete() + return [MockDomain(name="vm-orphan", uuid_str="88888888-8888-8888-8888-888888888888")] + + self.proxy(list_domains) + self.compute.__dict__["status"] = True + refresh_instance_database(self.compute) + + self.assertFalse(Instance.objects.filter(uuid="88888888-8888-8888-8888-888888888888").exists()) diff --git a/computes/utils.py b/computes/utils.py index c08722f2..557dccf1 100644 --- a/computes/utils.py +++ b/computes/utils.py @@ -220,37 +220,42 @@ def refresh_instance_database(compute): try: with libvirt_compute_lock(compute, timeout=15.0): - with transaction.atomic(): - comp_locked = Compute.objects.select_for_update().filter(pk=compute_pk).first() - if not comp_locked: - return + # 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. The compute lock still keeps + # create/delete/migration of this compute out until the end. + if hasattr(compute, "status") and compute.status is False: + return - if hasattr(compute, "status") and compute.status is False: - 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: - domains = compute.proxy.wvm.listAllDomains() + host_domains[d.UUIDString()] = d.name() except Exception as e: logger.warning( - "Failed to retrieve domains from compute %s (%s): %s", + "Error reading domain metadata from compute %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(): + # the compute may have been deleted while libvirt was asked + comp_locked = Compute.objects.select_for_update().filter(pk=compute_pk).first() + if not comp_locked: + return db_instances = list(Instance.objects.filter(compute=compute).select_for_update()) db_uuids = set(inst.uuid for inst in db_instances)