j3d1/webvirtcloud
1
0
Fork 0
mirror of https://github.com/retspen/webvirtcloud synced 2026-11-08 11:16:47 +00:00
Code Issues Projects Releases Wiki Activity

fix(instances): an operation on a VM a migration moved meanwhile is retried, not run on the new host

Browse source 
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.
This commit is contained in:
catborise 2026-10-07 22:57:16 +03:00
parent 5ee13f305d
commit e481b24240
2 changed files with 219 additions and 46 deletions
Download patch file Download diff file Expand all files Collapse all files

143
instances/test_moved_while_waiting.py Normal file
View file

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

122
instances/views.py
View file

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