refactor(computes): the lock helpers take claims instead of file locks

instances/api/viewsets.py

@@ -311,8 +311,9 @@ class CreateInstanceViewSet(viewsets.ViewSet):
                     raise
                 return Response({"status": str(err)}, status=status.HTTP_400_BAD_REQUEST)
             # After the definition: the VM uses the volume, so no rollback here.
-            with utils.libvirt_compute_lock(compute):
-                utils.refresh_instance_database(compute)
+            # The refresh claims the compute itself and skips it when busy; the
+            # VM exists either way, so a busy compute must not refuse the request.
+            utils.refresh_instance_database(compute)
             msg = f"Instance {serializer.validated_data['name']} is created"
             return Response({"status": msg})
         else:

instances/tests/test_clone_permission.py

@@ -176,7 +176,7 @@ class ClonePermissionTestCase(TestCase):
         calls = []
 
         @contextmanager
-        def record(compute, timeout=15.0, *, shared=False):
+        def record(compute, *, shared=False):
             calls.append(shared)
             yield
 

instances/tests/test_destroy.py

@@ -58,7 +58,7 @@ class DestroyOrderTestCase(TestCase):
         calls = []
 
         @contextmanager
-        def record(compute, timeout=15.0, *, shared=False):
+        def record(compute, *, shared=False):
             calls.append(shared)
             yield
 
@@ -86,7 +86,7 @@ class DestroyOrderTestCase(TestCase):
         calls = []
 
         @contextmanager
-        def record(compute, timeout=15.0, *, shared=False):
+        def record(compute, *, shared=False):
             calls.append(shared)
             yield
 

instances/tests/test_moved_while_waiting.py

@@ -1,9 +1,8 @@
 """
-A VM operation waits for the VM's compute lock while a migration holds it.
-When the migration moves the VM to another compute meanwhile, the waiting
-operation holds the old compute's lock, not the new one's: it must not run
-on the new host but answer "busy, retry", and the retry locks the right
-compute.
+A migration may move a VM between a request reading it and taking its
+compute's claim: the request then holds the old compute's claim, not the
+new one's. It must not run on the new host but answer "moved, try again",
+and the retry claims the right compute.
 """
 from contextlib import contextmanager
 from unittest.mock import PropertyMock, patch
@@ -35,7 +34,7 @@ class MovedWhileWaitingTestCase(TestCase):
 
     def test_a_vm_moved_while_the_request_waited_is_not_touched(self):
         @contextmanager
-        def lock_after_a_migration(instance, timeout=15.0):
+        def lock_after_a_migration(instance):
             # the migration that held the lock moved the VM before releasing it
             Instance.objects.filter(pk=instance.pk).update(compute=self.target)
             yield
@@ -44,11 +43,11 @@ class MovedWhileWaitingTestCase(TestCase):
         self.assertFalse(touched)
         self.assertEqual(response.status_code, 302)
         messages = [str(m) for m in response.wsgi_request._messages]
-        self.assertIn("Instance operation is busy. Please retry.", messages)
+        self.assertIn("The VM was moved meanwhile; try again.", messages)
 
     def test_a_vm_that_stayed_is_changed(self):
         @contextmanager
-        def plain_lock(instance, timeout=15.0):
+        def plain_lock(instance):
             yield
 
         response, touched = self.poweroff(plain_lock)
@@ -58,7 +57,7 @@ class MovedWhileWaitingTestCase(TestCase):
         # destroy holds the whole compute; the migration moved the VM before
         # releasing it
         @contextmanager
-        def compute_lock_after_a_migration(compute, timeout=15.0, *, shared=False):
+        def compute_lock_after_a_migration(compute, *, shared=False):
             Instance.objects.filter(pk=self.vm.pk).update(compute=self.target)
             yield
 
@@ -69,11 +68,11 @@ class MovedWhileWaitingTestCase(TestCase):
         self.assertFalse(wvm.return_value.delete.called)
         self.assertTrue(Instance.objects.filter(pk=self.vm.pk).exists())
         messages = [str(m) for m in response.wsgi_request._messages]
-        self.assertIn("Instance operation is busy. Please retry.", messages)
+        self.assertIn("The VM was moved meanwhile; try again.", messages)
 
     def clone(self, user, change):
         @contextmanager
-        def compute_lock_meanwhile(compute, timeout=15.0, *, shared=False):
+        def compute_lock_meanwhile(compute, *, shared=False):
             change()
             yield
 
@@ -101,7 +100,7 @@ class MovedWhileWaitingTestCase(TestCase):
             owner, lambda: Instance.objects.filter(pk=self.vm.pk).update(compute=self.target)
         )
         self.assertFalse(cloned)
-        self.assertIn("Instance operation is busy. Please retry.", [str(m) for m in response.wsgi_request._messages])
+        self.assertIn("The VM was moved meanwhile; try again.", [str(m) for m in response.wsgi_request._messages])
 
     def test_a_template_unmarked_while_the_request_waited_is_not_cloned_by_a_viewer(self):
         from django.contrib.auth.models import Permission
@@ -128,7 +127,7 @@ class MovedWhileWaitingTestCase(TestCase):
             wvm.return_value.get_memory.return_value = 512
 
             @contextmanager
-            def compute_lock_after_a_resize(compute, timeout=15.0, *, shared=False):
+            def compute_lock_after_a_resize(compute, *, shared=False):
                 wvm.return_value.get_vcpu.return_value = 8  # resized meanwhile
                 yield
 

instances/tests/test_security_mutations.py

@@ -3,12 +3,14 @@ from django.core.exceptions import PermissionDenied
 from django.contrib.auth import get_user_model
 from django.contrib.auth.models import Permission
 from django.http import Http404
-from django.test import TestCase
+from django.db import connection
+from django.test import TestCase, TransactionTestCase
 from django.urls import reverse
 
 from accounts.models import UserInstance
 from computes.models import Compute
 from instances.models import Instance
+from operations import claims
 from instances.views import get_instance
 
 
@@ -214,89 +216,6 @@ class InstanceSecurityMutationsTestCase(TestCase):
             res = self.client.get(reverse(f"instances:{ep}", args=[self.instance.id]))
             self.assertEqual(res.status_code, 405, f"{ep} allowed GET request instead of 405")
 
-    def test_opposite_direction_migrations_no_deadlock(self):
-        import threading
-        import time
-        from instances.utils import migrate_instance
-
-        c1 = MagicMock()
-        c1.id = 101
-        c1.pk = 101
-        c1.hostname = "127.0.0.1"
-        c1.type = 1
-        c1.login = "root"
-        c1.password = ""
-
-        c2 = MagicMock()
-        c2.id = 102
-        c2.pk = 102
-        c2.hostname = "127.0.0.2"
-        c2.type = 1
-        c2.login = "root"
-        c2.password = ""
-
-        inst1 = MagicMock()
-        inst1.id = "inst1"
-        inst1.compute = c1
-        inst1.name = "inst1"
-        inst1.autostart = False
-        inst1.proxy = MagicMock()
-        inst1.uuid = "u1"
-
-        inst2 = MagicMock()
-        inst2.id = "inst2"
-        inst2.compute = c2
-        inst2.name = "inst2"
-        inst2.autostart = False
-        inst2.proxy = MagicMock()
-        inst2.uuid = "u2"
-
-        barrier = threading.Barrier(2)
-        errors = []
-
-        def worker_1_to_2():
-            try:
-                barrier.wait(timeout=5)
-                migrate_instance(c2, inst1, self.superuser)
-            except Exception as e:
-                errors.append(("worker_1_to_2", e))
-
-        def worker_2_to_1():
-            try:
-                barrier.wait(timeout=5)
-                migrate_instance(c1, inst2, self.superuser)
-            except Exception as e:
-                errors.append(("worker_2_to_1", e))
-
-        def slow_moveto(*args, **kwargs):
-            time.sleep(0.05)
-
-        with patch("instances.utils.wvmInstances") as mock_instances_cls, \
-             patch("instances.utils.wvmInstance"), \
-             patch("instances.utils.transaction.atomic"), \
-             patch("instances.utils.addlogmsg"), \
-             patch("instances.utils.Instance") as mock_instance_model:
-
-            mock_instances_cls.return_value.moveto.side_effect = slow_moveto
-            # under the locks each VM is still on its compute; the
-            # destination has no record of it
-            def rows(**query):
-                found = MagicMock(compute_id={"inst1": 101, "inst2": 102}[query["id"]]) if "id" in query else None
-                return MagicMock(**{"first.return_value": found})
-
-            mock_instance_model.objects.filter.side_effect = rows
-
-            t1 = threading.Thread(target=worker_1_to_2)
-            t2 = threading.Thread(target=worker_2_to_1)
-            t1.start()
-            t2.start()
-            t1.join(timeout=10)
-            t2.join(timeout=10)
-
-            self.assertFalse(t1.is_alive(), "Worker 1 to 2 timed out / deadlocked")
-            self.assertFalse(t2.is_alive(), "Worker 2 to 1 timed out / deadlocked")
-            self.assertEqual(errors, [], f"Unexpected errors during concurrent migrations: {errors}")
-
     def test_resize_disk_empty_selection_warns_cleanly(self):
         self.client.force_login(self.change_user)
         with patch("instances.models.wvmInstance") as mock_wvm_cls:
@@ -411,9 +330,112 @@ class InstanceSecurityMutationsTestCase(TestCase):
 
     def test_busy_instance_redirects_without_executing_the_operation(self):
         self.client.force_login(self.owner)
-        with patch("instances.views.libvirt_instance_lock", side_effect=TimeoutError("busy")), patch(
+        with patch("instances.views.libvirt_instance_lock", side_effect=claims.Busy("instance:wvc-test")), patch(
             "instances.models.wvmInstance"
         ) as proxy:
             response = self.client.post(reverse("instances:poweron", args=[self.instance.pk]), {})
         self.assertRedirects(response, reverse("instances:instance", args=[self.instance.pk]), fetch_redirect_response=False)
         proxy.assert_not_called()
+
+
+class OppositeMigrationsTestCase(TransactionTestCase):
+    """Two migrations between the same computes in opposite directions: a
+    claim is refused at once instead of waited for, so one may be refused but
+    none deadlocks."""
+
+    def setUp(self):
+        self.superuser = get_user_model().objects.create_superuser("migrate_admin", "a@example.com", "x")
+
+    def test_opposite_direction_migrations_never_deadlock(self):
+        import threading
+        import time
+        from instances.utils import migrate_instance
+
+        c1 = MagicMock()
+        c1.id = 101
+        c1.pk = 101
+        c1.hostname = "192.0.2.1"
+        c1.type = 1
+        c1.login = "root"
+        c1.password = ""
+
+        c2 = MagicMock()
+        c2.id = 102
+        c2.pk = 102
+        c2.hostname = "192.0.2.2"
+        c2.type = 1
+        c2.login = "root"
+        c2.password = ""
+
+        inst1 = MagicMock()
+        inst1.id = "inst1"
+        inst1.compute = c1
+        inst1.name = "inst1"
+        inst1.autostart = False
+        inst1.proxy = MagicMock()
+        inst1.uuid = "u1"
+
+        inst2 = MagicMock()
+        inst2.id = "inst2"
+        inst2.compute = c2
+        inst2.name = "inst2"
+        inst2.autostart = False
+        inst2.proxy = MagicMock()
+        inst2.uuid = "u2"
+
+        barrier = threading.Barrier(2)
+        errors, refused = [], []
+
+        def worker_1_to_2():
+            try:
+                barrier.wait(timeout=5)
+                migrate_instance(c2, inst1, self.superuser)
+            except claims.Busy:
+                refused.append("worker_1_to_2")
+            except Exception as e:
+                errors.append(("worker_1_to_2", e))
+            finally:
+                connection.close()
+
+        def worker_2_to_1():
+            try:
+                barrier.wait(timeout=5)
+                migrate_instance(c1, inst2, self.superuser)
+            except claims.Busy:
+                refused.append("worker_2_to_1")
+            except Exception as e:
+                errors.append(("worker_2_to_1", e))
+            finally:
+                connection.close()
+
+        def slow_moveto(*args, **kwargs):
+            time.sleep(0.05)
+
+        with patch("instances.utils.wvmInstances") as mock_instances_cls, \
+             patch("instances.utils.wvmInstance"), \
+             patch("instances.utils.record_migration"), \
+             patch("instances.utils.addlogmsg"), \
+             patch("instances.utils.Instance") as mock_instance_model:
+
+            mock_instances_cls.return_value.moveto.side_effect = slow_moveto
+            # under the locks each VM is still on its compute; the
+            # destination has no record of it
+            def rows(**query):
+                found = MagicMock(compute_id={"inst1": 101, "inst2": 102}[query["id"]]) if "id" in query else None
+                return MagicMock(**{"first.return_value": found})
+
+            mock_instance_model.objects.filter.side_effect = rows
+
+            t1 = threading.Thread(target=worker_1_to_2)
+            t2 = threading.Thread(target=worker_2_to_1)
+            t1.start()
+            t2.start()
+            t1.join(timeout=10)
+            t2.join(timeout=10)
+
+            self.assertFalse(t1.is_alive(), "Worker 1 to 2 timed out / deadlocked")
+            self.assertFalse(t2.is_alive(), "Worker 2 to 1 timed out / deadlocked")
+            self.assertEqual(errors, [], f"Unexpected errors during concurrent migrations: {errors}")
+            # a refusal is immediate; at least one of them migrates
+            self.assertLess(len(refused), 2)
+            self.assertEqual(mock_instances_cls.return_value.moveto.call_count, 2 - len(refused))

instances/views.py

@@ -38,6 +38,8 @@ from libvirt import (VIR_DOMAIN_UNDEFINE_KEEP_NVRAM,
                      VIR_DOMAIN_UNDEFINE_NVRAM,
                      libvirtError)
 from logs.views import addlogmsg
+from operations import claims
+from operations.utils import busy_message, summary
 from webvirtcloud.middleware import error_text
 from vrtManager import util
 from vrtManager.create import wvmCreate
@@ -446,16 +448,25 @@ def get_instance(user, pk, perm_type="view"):
     return instance
 
 
-def _busy(request, pk):
+def _busy(request, pk, busy):
     if hasattr(request, "accepted_renderer"):
-        return JsonResponse({"detail": "Instance operation is busy. Please retry."}, status=409)
-    messages.error(request, _("Instance operation is busy. Please retry."))
+        blocker = busy.blocker
+        return JsonResponse({"detail": busy_message(busy, request.user),
+                             "operation": summary(blocker, request.user) if blocker else None}, status=409)
+    messages.error(request, busy_message(busy, request.user))
+    return get_safe_redirect(request, default=reverse("instances:instance", args=[pk]))
+
+
+def _moved(request, pk):
+    if hasattr(request, "accepted_renderer"):
+        return JsonResponse({"detail": "The VM was moved meanwhile; try again."}, status=409)
+    messages.error(request, _("The VM was moved meanwhile; try again."))
     return get_safe_redirect(request, default=reverse("instances:instance", args=[pk]))
 
 
 def _moved_meanwhile(pk, inst):
-    """A migration may have moved the VM while the request waited for its
-    compute's lock: the lock held is then the old compute's. The retry locks
+    """A migration may have moved the VM before the request took its
+    compute's claim: the claim held is then the old compute's. The retry locks
     the new one (or finds the record merged away)."""
     return not Instance.objects.filter(pk=pk, compute_id=inst.compute_id).exists()
 
@@ -468,10 +479,10 @@ def serialize_instance_mutation(func):
         try:
             with libvirt_instance_lock(inst):
                 if _moved_meanwhile(pk, inst):
-                    return _busy(request, pk)
+                    return _moved(request, pk)
                 return func(request, pk, *args, **kwargs)
-        except TimeoutError:
-            return _busy(request, pk)
+        except claims.Busy as busy:
+            return _busy(request, pk, busy)
 
     return wrapper
 
@@ -494,8 +505,8 @@ def serialize_user_quota(func):
         try:
             with user_quota_lock(request.user):
                 return func(request, pk, *args, **kwargs)
-        except TimeoutError:
-            return _busy(request, pk)
+        except claims.Busy as busy:
+            return _busy(request, pk, busy)
 
     return wrapper
 
@@ -510,10 +521,10 @@ def serialize_compute_mutation(func):
         try:
             with libvirt_compute_lock(inst.compute):
                 if _moved_meanwhile(pk, inst):
-                    return _busy(request, pk)
+                    return _moved(request, pk)
                 return func(request, pk, *args, **kwargs)
-        except TimeoutError:
-            return _busy(request, pk)
+        except claims.Busy as busy:
+            return _busy(request, pk, busy)
 
     return wrapper
 
@@ -728,7 +739,9 @@ def migrate(request, pk):
     try:
         # migrate_instance logs the outcome
         utils.migrate_instance(target_host, instance, request.user, live, unsafe, offline, autoconverge, compress)
-    except (libvirtError, OSError) as err:  # OSError includes a lock timeout
+    except claims.Busy as busy:
+        return _busy(request, instance.id, busy)
+    except (libvirtError, OSError) as err:
         messages.error(request, err)
 
     return get_safe_redirect(
@@ -1945,11 +1958,11 @@ def clone(request, pk):
         # The whole compute: the destination name and disk names must not be
         # taken by a concurrent clone of another VM while volumes are copied.
         with libvirt_compute_lock(instance.compute):
-            # While this request waited, a migration may have moved the
+            # Before this request took the claim, a migration may have moved the
             # source, or its template mark, size or the user's rights changed:
             # the clone is checked against the source as it is now.
             if _moved_meanwhile(pk, instance):
-                return _busy(request, pk)
+                return _moved(request, pk)
             instance = _clone_source(request.user, pk)
             clone_data, refusal = _clone_plan(request, instance)
             if refusal:
@@ -1978,7 +1991,9 @@ def clone(request, pk):
         )
 
         return redirect(reverse("instances:instance", args=[new_instance.id]))
-    except (libvirtError, ValueError, OSError) as e:  # OSError includes a lock timeout
+    except claims.Busy as busy:
+        return _busy(request, pk, busy)
+    except (libvirtError, ValueError, OSError) as e:
         messages.error(request, error_text(request, e))
 
     return get_safe_redirect(request, default=reverse("instances:instance", args=[pk]))