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