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

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

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

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: