From addc7ccfcb6d77d28587175aa8f6384f6ca268ba Mon Sep 17 00:00:00 2001 From: catborise Date: Fri, 23 Oct 2026 15:07:45 +0300 Subject: [PATCH] fix(computes): a VM moved between computes keeps its owners on PostgreSQL Two computes decide the owners of the same UUID: the one the VM left buries them in a tombstone (or a compute delete does), the one it went to restores them. On PostgreSQL the two ran at once: the restore still saw the old row of the uncommitted bury, skipped, and the VM ended up without owners while the tombstone stayed. SQLite was never affected: its IMMEDIATE transactions take the write lock at the start. The reconcile and the compute delete now take one transaction-level advisory lock (pg_advisory_xact_lock) when their database step starts, so ownership decisions run one after another on PostgreSQL as they do on SQLite. Reading libvirt stays outside the lock. --- computes/models.py | 3 +- computes/tests/test_ownership_race.py | 82 +++++++++++++++++++++++++++ computes/utils.py | 16 +++++- 3 files changed, 99 insertions(+), 2 deletions(-) create mode 100644 computes/tests/test_ownership_race.py diff --git a/computes/models.py b/computes/models.py index 1bd4fa6e..a8c13433 100644 --- a/computes/models.py +++ b/computes/models.py @@ -96,9 +96,10 @@ class Compute(Model): exclusive, so no reconcile, VM operation or job on it runs meanwhile (Busy if one does); its row is locked before its VMs', as the reconcile does.""" - from computes.utils import forget, libvirt_compute_lock + from computes.utils import forget, libvirt_compute_lock, lock_ownership with libvirt_compute_lock(self), transaction.atomic(): + lock_ownership() Compute.objects.select_for_update().filter(pk=self.pk).first() for inst in self.instance_set.select_for_update(): forget(inst) diff --git a/computes/tests/test_ownership_race.py b/computes/tests/test_ownership_race.py new file mode 100644 index 00000000..fdc31b98 --- /dev/null +++ b/computes/tests/test_ownership_race.py @@ -0,0 +1,82 @@ +"""Two reconciles of different computes decide the owners of the same UUID +one after the other: the move's restore sees the bury committed. SQLite's +IMMEDIATE transactions serialize them; PostgreSQL takes the ownership lock.""" + +import threading +import time +from unittest.mock import MagicMock, patch + +from accounts.models import UserInstance +from computes import utils +from computes.models import Compute +from django.contrib.auth import get_user_model +from django.db import connection +from django.test import TransactionTestCase +from instances.models import Instance, InstanceTombstone + +UUID = "44444444-4444-4444-4444-444444444444" + + +class Domain: + def __init__(self, name, uuid): + self._name, self._uuid = name, uuid + + def name(self): + return self._name + + def UUIDString(self): + return self._uuid + + +class OwnershipRaceTests(TransactionTestCase): + def compute(self, name, domains): + c = Compute.objects.create(name=name, hostname=f"{name}.example", login="", password="", type=1) + c.__dict__["proxy"] = MagicMock() + c.__dict__["status"] = True + c.proxy.wvm.listAllDomains.return_value = [Domain(n, u) for n, u in domains] + return c + + def test_move_seen_by_both_reconciles_at_once(self): + a = self.compute("host-a", []) # the VM left A (virsh migrate) + self.race(a, lambda: utils.refresh_instance_database(a)) + + def test_a_compute_delete_and_a_reconcile_at_once(self): + a = self.compute("host-a", [("vm", UUID)]) # deleted while B sees the VM + self.race(a, a.delete) + + def race(self, a, decide_a): + """decide_a buries the VM's owners on A and waits with its transaction + open while B's reconcile finds the VM; B must restore them.""" + owner = get_user_model().objects.create_user("owner", password="pw") + b = self.compute("host-b", [("vm", UUID)]) + vm = Instance.objects.create(compute=a, name="vm", uuid=UUID) + UserInstance.objects.create(instance=vm, user=owner, is_change=True) + + buried, go = threading.Event(), threading.Event() + real_forget = utils.forget + + def slow_forget(inst): + real_forget(inst) # B's row does not exist yet: owners go to a tombstone + buried.set() + go.wait(10) # A's transaction stays open here + + def run(decide): + try: + decide() + finally: + connection.close() + + with patch.object(utils, "forget", slow_forget): + ta = threading.Thread(target=run, args=(decide_a,)) + ta.start() + self.assertTrue(buried.wait(10)) + tb = threading.Thread(target=run, args=(lambda: utils.refresh_instance_database(b),)) + tb.start() + time.sleep(1) # B would decide now if nothing held it + go.set() + ta.join(30) + tb.join(30) + + on_b = Instance.objects.get(compute=b, uuid=UUID) + self.assertEqual(list(on_b.userinstance_set.values_list("user__username", flat=True)), ["owner"]) + self.assertFalse(InstanceTombstone.objects.exists()) diff --git a/computes/utils.py b/computes/utils.py index 76c7638d..7a95657e 100644 --- a/computes/utils.py +++ b/computes/utils.py @@ -5,7 +5,7 @@ from datetime import timedelta from accounts.models import UserInstance from django.conf import settings from django.contrib.auth import get_user_model -from django.db import transaction +from django.db import connection, transaction from django.utils import timezone from computes.models import Compute from instances.models import Instance, InstanceTombstone @@ -46,6 +46,19 @@ def libvirt_instance_lock(instance): yield +# pg_advisory_xact_lock key of the ownership step ("wvc_own") +OWNERSHIP_LOCK = 0x7776635F6F776E + + +def lock_ownership(): + """Hold the ownership step of every compute until this transaction ends: + a bury and a restore of the same UUID on two computes must not see each + other's work half done. SQLite's IMMEDIATE transactions already do this.""" + if connection.vendor == "postgresql": + with connection.cursor() as cursor: + cursor.execute("SELECT pg_advisory_xact_lock(%s)", [OWNERSHIP_LOCK]) + + def _log(inst, what): write("-", inst.compute.name, "", f"VM {inst.name} ({inst.uuid}): {what}", True) @@ -170,6 +183,7 @@ def refresh_instance_database(compute): return with transaction.atomic(): + lock_ownership() # 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: