From e481b24240adbb9351b39fccc9ba1019858fa57f Mon Sep 17 00:00:00 2001 From: catborise Date: Wed, 7 Oct 2026 22:57:16 +0300 Subject: [PATCH] fix(instances): an operation on a VM a migration moved meanwhile is retried, not run on the new host A VM operation (or destroy and clone, which hold the whole compute) waits for its compute's lock while a migration holds it. When the migration moved the VM's record to the destination, the waiting operation went on there holding only the source compute's lock, unordered with a reconciliation, migration or disk attach on the destination. After taking the locks it now checks the VM is still on the locked compute and otherwise answers busy, so the retry locks the right one. Clone also checks again under the lock whether the user may clone the VM, and checks the quota and the clone's settings against the VM as it is then: its template mark, size or the user's rights may have changed meanwhile. --- instances/test_moved_while_waiting.py | 143 ++++++++++++++++++++++++++ instances/views.py | 122 +++++++++++++--------- 2 files changed, 219 insertions(+), 46 deletions(-) create mode 100644 instances/test_moved_while_waiting.py diff --git a/instances/test_moved_while_waiting.py b/instances/test_moved_while_waiting.py new file mode 100644 index 00000000..12151ca4 --- /dev/null +++ b/instances/test_moved_while_waiting.py @@ -0,0 +1,143 @@ +""" +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. +""" +from contextlib import contextmanager +from unittest.mock import PropertyMock, patch + +from django.contrib.auth import get_user_model +from django.test import TestCase +from django.urls import reverse + +from computes.models import Compute +from instances.models import Instance + + +class MovedWhileWaitingTestCase(TestCase): + def setUp(self): + self.admin = get_user_model().objects.create_superuser("moved_admin", "m@example.com", "x") + self.source = Compute.objects.create(name="src", hostname="192.0.2.1", login="root", password="", type=1) + self.target = Compute.objects.create(name="dst", hostname="192.0.2.2", login="root", password="", type=1) + self.vm = Instance.objects.create( + compute=self.source, name="moved-vm", uuid="eeeeeeee-ffff-0000-1111-222222222222" + ) + self.client.force_login(self.admin) + + def poweroff(self, lock): + with patch("instances.views.libvirt_instance_lock", lock), patch( + "instances.models.wvmInstance" + ) as wvm, patch.object(Compute, "status", new_callable=PropertyMock, return_value=True): + response = self.client.post(reverse("instances:poweroff", args=[self.vm.id])) + return response, wvm.return_value.shutdown.called + + def test_a_vm_moved_while_the_request_waited_is_not_touched(self): + @contextmanager + def lock_after_a_migration(instance, timeout=15.0): + # the migration that held the lock moved the VM before releasing it + Instance.objects.filter(pk=instance.pk).update(compute=self.target) + yield + + response, touched = self.poweroff(lock_after_a_migration) + 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) + + def test_a_vm_that_stayed_is_changed(self): + @contextmanager + def plain_lock(instance, timeout=15.0): + yield + + response, touched = self.poweroff(plain_lock) + self.assertTrue(touched) + + def test_destroy_of_a_vm_moved_while_it_waited_is_not_run(self): + # 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): + Instance.objects.filter(pk=self.vm.pk).update(compute=self.target) + yield + + with patch("instances.views.libvirt_compute_lock", compute_lock_after_a_migration), patch( + "instances.models.wvmInstance" + ) as wvm, patch.object(Compute, "status", new_callable=PropertyMock, return_value=True): + response = self.client.post(reverse("instances:destroy", args=[self.vm.id]), {"delete_disk": "on"}) + 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) + + def clone(self, user, change): + @contextmanager + def compute_lock_meanwhile(compute, timeout=15.0, *, shared=False): + change() + yield + + self.client.force_login(user) + with patch("instances.views.libvirt_compute_lock", compute_lock_meanwhile), patch( + "instances.models.wvmInstance" + ) as wvm, patch.object(Compute, "status", new_callable=PropertyMock, return_value=True), patch( + "instances.views.utils.check_user_quota", return_value="" # not what this tests + ): + wvm.return_value.get_disk_devices.return_value = [] + wvm.return_value.get_vcpu.return_value = 1 + wvm.return_value.get_memory.return_value = 512 + response = self.client.post(reverse("instances:clone", args=[self.vm.id]), {"name": "moved-clone"}) + return response, wvm.return_value.clone_instance.called + + def test_a_clone_of_a_vm_moved_while_it_waited_is_not_made(self): + from accounts.models import UserInstance + from django.contrib.auth.models import Permission + + # an owner who may clone it (a superuser would also choose MACs) + owner = get_user_model().objects.create_user("clone_owner", password="x") + owner.user_permissions.add(Permission.objects.get(codename="clone_instances")) + UserInstance.objects.create(instance=self.vm, user=owner, is_change=True) + response, cloned = self.clone( + 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]) + + def test_a_template_unmarked_while_the_request_waited_is_not_cloned_by_a_viewer(self): + from django.contrib.auth.models import Permission + + viewer = get_user_model().objects.create_user("clone_viewer", password="x") + for codename in ("view_instances", "clone_instances"): + viewer.user_permissions.add(Permission.objects.get(codename=codename)) + Instance.objects.filter(pk=self.vm.pk).update(is_template=True) + response, cloned = self.clone(viewer, lambda: Instance.objects.filter(pk=self.vm.pk).update(is_template=False)) + self.assertFalse(cloned) + self.assertEqual(response.status_code, 403) + + def test_a_clone_checks_the_quota_against_the_source_as_it_is_under_the_lock(self): + from accounts.models import UserInstance + from django.contrib.auth.models import Permission + + owner = get_user_model().objects.create_user("clone_quota", password="x") + owner.user_permissions.add(Permission.objects.get(codename="clone_instances")) + UserInstance.objects.create(instance=self.vm, user=owner, is_change=True) + self.client.force_login(owner) + with patch("instances.models.wvmInstance") as wvm: + wvm.return_value.get_disk_devices.return_value = [] + wvm.return_value.get_vcpu.return_value = 1 + wvm.return_value.get_memory.return_value = 512 + + @contextmanager + def compute_lock_after_a_resize(compute, timeout=15.0, *, shared=False): + wvm.return_value.get_vcpu.return_value = 8 # resized meanwhile + yield + + with patch("instances.views.libvirt_compute_lock", compute_lock_after_a_resize), patch.object( + Compute, "status", new_callable=PropertyMock, return_value=True + ), patch( + "instances.views.utils.check_user_quota", + side_effect=lambda user, count, cpu, memory, disk: "cpu" if cpu > 4 else "", + ): + response = self.client.post(reverse("instances:clone", args=[self.vm.id]), {"name": "grown-clone"}) + self.assertFalse(wvm.return_value.clone_instance.called) + self.assertTrue(any("quota" in str(m) for m in response.wsgi_request._messages)) diff --git a/instances/views.py b/instances/views.py index 1706cd78..db94a7ac 100755 --- a/instances/views.py +++ b/instances/views.py @@ -439,6 +439,13 @@ def _busy(request, pk): 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 + the new one (or finds the record merged away).""" + return not Instance.objects.filter(pk=pk, compute_id=inst.compute_id).exists() + + def serialize_instance_mutation(func): """Serialize one VM, after checking visibility, without blocking sibling VMs.""" @functools.wraps(func) @@ -446,6 +453,8 @@ def serialize_instance_mutation(func): inst = get_instance(request.user, pk) try: with libvirt_instance_lock(inst): + if _moved_meanwhile(pk, inst): + return _busy(request, pk) return func(request, pk, *args, **kwargs) except TimeoutError: return _busy(request, pk) @@ -486,6 +495,8 @@ def serialize_compute_mutation(func): return func(request, pk, *args, **kwargs) try: with libvirt_compute_lock(inst.compute): + if _moved_meanwhile(pk, inst): + return _busy(request, pk) return func(request, pk, *args, **kwargs) except TimeoutError: return _busy(request, pk) @@ -1753,16 +1764,18 @@ CDROM_BUSES = ("ide", "sata", "scsi") CLONE_POST_KEY_RE = re.compile(r"^(clone-net-mac-\d+|disk-[a-z0-9]+|meta-[a-z0-9]+)$") -@require_POST -@permission_required("instances.clone_instances", raise_exception=True) -@serialize_user_quota -def clone(request, pk): - # Cloning copies the source disks, so it needs change permission on the - # source VM. Templates are meant to be deployed from, so viewing is enough. - instance = get_instance(request.user, pk) +def _clone_source(user, pk): + """Cloning copies the source disks, so it needs change permission on the + source VM. Templates are meant to be deployed from, so viewing is enough.""" + instance = get_instance(user, pk) if not instance.is_template: - instance = get_instance(request.user, pk, perm_type="change") + instance = get_instance(user, pk, perm_type="change") + return instance + +def _clone_plan(request, instance): + """The clone's settings from the request and the source as it is now, + and why it cannot be made, or None.""" clone_data = dict() clone_data["name"] = request.POST.get("name", "").strip() clone_data["clone-title"] = request.POST.get("clone-title", "").strip() @@ -1826,57 +1839,74 @@ def clone(request, pk): "quota_msg": quota_msg, "clone_name": clone_data["name"], }) - messages.error(request, msg) - elif check_instance: + return clone_data, msg + if check_instance: msg = _("Instance '%(clone_name)s' already exists!") % { "clone_name": clone_data["name"] } - messages.error(request, msg) - elif not re.match(r"^[a-zA-Z0-9-]+$", clone_data["name"]): + return clone_data, msg + if not re.match(r"^[a-zA-Z0-9-]+$", clone_data["name"]): msg = _("Instance name '%(clone_name)s' contains invalid characters!") % { "clone_name": clone_data["name"] } - messages.error(request, msg) - elif "clone-net-mac-0" not in clone_data or invalid_macs: + return clone_data, msg + if "clone-net-mac-0" not in clone_data or invalid_macs: msg = _("Instance MAC '%(clone_mac)s' invalid format!") % { "clone_mac": ", ".join(invalid_macs) } - messages.error(request, msg) - elif invalid_disks: + return clone_data, msg + if invalid_disks: msg = _("Disk name '%(disk_name)s' contains invalid characters!") % { "disk_name": ", ".join(str(v) for v in invalid_disks) } - messages.error(request, msg) - else: - try: - # 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): - new_uuid = instance.proxy.clone_instance(clone_data) - new_instance = Instance.objects.get_or_create( - compute=instance.compute, - uuid=new_uuid, - defaults={"name": clone_data["name"]}, - )[0] - if new_instance.name != clone_data["name"]: - new_instance.name = clone_data["name"] - new_instance.save(update_fields=["name"]) - UserInstance.objects.get_or_create( - instance_id=new_instance.id, - user_id=request.user.id, - defaults={"is_delete": True, "is_change": True, "is_vnc": True}, - ) - msg = _("Create a clone of '%(instance_name)s'") % { - "instance_name": instance.name - } - messages.success(request, msg) - addlogmsg( - request.user.username, instance.compute.name, new_instance.name, msg - ) + return clone_data, msg + return clone_data, None - return redirect(reverse("instances:instance", args=[new_instance.id])) - except (libvirtError, ValueError, OSError) as e: # OSError includes a lock timeout - messages.error(request, error_text(request, e)) + +@require_POST +@permission_required("instances.clone_instances", raise_exception=True) +@serialize_user_quota +def clone(request, pk): + instance = _clone_source(request.user, pk) + try: + # 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 + # 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) + instance = _clone_source(request.user, pk) + clone_data, refusal = _clone_plan(request, instance) + if refusal: + messages.error(request, refusal) + return get_safe_redirect(request, default=reverse("instances:instance", args=[pk])) + new_uuid = instance.proxy.clone_instance(clone_data) + new_instance = Instance.objects.get_or_create( + compute=instance.compute, + uuid=new_uuid, + defaults={"name": clone_data["name"]}, + )[0] + if new_instance.name != clone_data["name"]: + new_instance.name = clone_data["name"] + new_instance.save(update_fields=["name"]) + UserInstance.objects.get_or_create( + instance_id=new_instance.id, + user_id=request.user.id, + defaults={"is_delete": True, "is_change": True, "is_vnc": True}, + ) + msg = _("Create a clone of '%(instance_name)s'") % { + "instance_name": instance.name + } + messages.success(request, msg) + addlogmsg( + request.user.username, instance.compute.name, new_instance.name, msg + ) + + return redirect(reverse("instances:instance", args=[new_instance.id])) + except (libvirtError, ValueError, OSError) as e: # OSError includes a lock timeout + messages.error(request, error_text(request, e)) return get_safe_redirect(request, default=reverse("instances:instance", args=[pk]))