From ffb2ce6bfabca7131f041eb3e0e0112fd5c4314a Mon Sep 17 00:00:00 2001 From: catborise Date: Sat, 3 Oct 2026 17:56:47 +0300 Subject: [PATCH] fix(instances): destroy and volume delete no longer lose disks Destroy: - Stops every active state, not only running; a paused VM used to keep running as a transient domain after the undefine. - Undefines with MANAGED_SAVE and SNAPSHOTS_METADATA, before any disk is deleted. A managed-save image used to make the undefine fail after the disks were already gone; now a failed undefine deletes nothing. - Keeps disks that another domain references, directly or as a backing file anywhere in its chain (running domains' backingStore, and the backingStore of the volumes in the active pools for stopped ones), and says so; they used to be deleted with the VM. Volume delete: - Detaches from the live domain in every active state, including paused. - Deletes the volume only after neither the live nor the persistent definition has the disk (bounded wait for the guest to release it), and refuses a volume another VM references. Otherwise the volume is kept and the user is told why. Both hold the compute exclusively for the usage scan and the deletion, so another VM cannot attach the disk in between; the destroy confirmation page (GET) takes no lock. Live tests reproduce each case on a RHEL qemu-kvm host and fail on the previous code; unit tests cover the destroy ordering and the locks. --- instances/test_destroy.py | 67 +++++++++++++++++++++ instances/test_live_dataloss.py | 102 ++++++++++++++++++++++++++++++++ instances/views.py | 71 ++++++++++++++++++---- vrtManager/instance.py | 59 ++++++++++++++++-- 4 files changed, 280 insertions(+), 19 deletions(-) create mode 100644 instances/test_destroy.py diff --git a/instances/test_destroy.py b/instances/test_destroy.py new file mode 100644 index 00000000..f50c3f3f --- /dev/null +++ b/instances/test_destroy.py @@ -0,0 +1,67 @@ +from contextlib import contextmanager +from unittest.mock import patch + +from computes.models import Compute +from django.contrib.auth import get_user_model +from django.test import TestCase +from django.urls import reverse +from instances.models import Instance +from libvirt import libvirtError + + +class DestroyOrderTestCase(TestCase): + def setUp(self): + admin = get_user_model().objects.create_superuser("destroy_admin", "d@example.com", "pw") + self.client.force_login(admin) + self.client.raise_request_exception = False + compute = Compute.objects.create(name="destroy-compute", hostname="127.0.0.1", login="", password="", type=4) + self.instance = Instance.objects.create( + compute=compute, name="destroy-vm", uuid="11111111-2222-3333-4444-555555555555" + ) + + def destroy(self, undefine_error=None): + with patch("instances.models.wvmInstance") as wvm: + proxy = wvm.return_value + proxy.instance.isActive.return_value = False + proxy.split_disk_paths_by_use.return_value = (["/pool/own.qcow2"], ["/pool/shared.qcow2"]) + proxy.delete.side_effect = undefine_error + self.client.post(reverse("instances:destroy", args=[self.instance.id]), {"delete_disk": "1"}) + return proxy + + def test_disks_are_deleted_after_the_undefine(self): + proxy = self.destroy() + proxy.get_volume_by_path.assert_called_once_with("/pool/own.qcow2") + self.assertFalse(Instance.objects.filter(pk=self.instance.pk).exists()) + + def test_failed_undefine_deletes_no_disk(self): + proxy = self.destroy(undefine_error=libvirtError("refused")) + proxy.get_volume_by_path.assert_not_called() + proxy.delete_all_disks.assert_not_called() + self.assertTrue(Instance.objects.filter(pk=self.instance.pk).exists()) + + def test_destroy_and_delete_vol_hold_the_compute_exclusively(self): + calls = [] + + @contextmanager + def record(compute, timeout=15.0, *, shared=False): + calls.append(shared) + yield + + with patch("instances.views.libvirt_compute_lock", record): + with patch("instances.models.wvmInstance"): + self.client.post(reverse("instances:delete_vol", args=[self.instance.id]), {"dev": "vda"}) + self.destroy() + self.assertEqual(calls, [False, False]) + + def test_confirmation_page_takes_no_lock(self): + calls = [] + + @contextmanager + def record(compute, timeout=15.0, *, shared=False): + calls.append(shared) + yield + + with patch("instances.views.libvirt_compute_lock", record), patch("instances.models.wvmInstance"): + response = self.client.get(reverse("instances:destroy", args=[self.instance.id])) + self.assertEqual(response.status_code, 200) + self.assertEqual(calls, []) diff --git a/instances/test_live_dataloss.py b/instances/test_live_dataloss.py index 92f30767..42965a65 100644 --- a/instances/test_live_dataloss.py +++ b/instances/test_live_dataloss.py @@ -82,6 +82,13 @@ class LiveDataLossTestCase(TestCase): for d in etree.fromstring(dom.XMLDesc(flags)).findall("devices/disk") } + def domain_exists(self, uuid): + try: + self.conn.lookupByUUIDString(uuid) + return True + except libvirt.libvirtError: + return False + # R-03: external snapshots @unittest.expectedFailure @@ -113,3 +120,98 @@ class LiveDataLossTestCase(TestCase): for path in sources: self.assertTrue(livetest.volume_exists(self.conn, path), f"{path} was deleted") self.assertIn("s1.newer", dom.snapshotListNames(0)) + + # R-14: destroy + + def test_r14_destroy_keeps_a_volume_another_vm_uses(self): + shared = livetest.create_volume(self.conn, P + "r14-shared") + _, inst = self.vm("r14-a", [shared]) + self.vm("r14-b", [shared]) + + response = self.post("destroy", inst, {"delete_disk": "1"}) + + self.assertEqual(response.status_code, 302) + self.assertTrue(livetest.volume_exists(self.conn, shared), "destroy deleted another VM's disk") + + def _destroy_base_of_another_vms_overlay(self, running): + base = livetest.create_volume(self.conn, P + "r14-base") + pool = self.conn.storagePoolLookupByName(livetest.POOL) + overlay = pool.createXML( + f"{P}r14-overlay.qcow264" + f"{base}" + "", + 0, + ).path() + _, inst = self.vm("r14-base", [base]) + dom_b, _ = self.vm("r14-overlay", [overlay]) + if running: + dom_b.create() + + response = self.post("destroy", inst, {"delete_disk": "1"}) + + self.assertEqual(response.status_code, 302) + self.assertTrue(livetest.volume_exists(self.conn, base), "deleted the backing file of another VM's disk") + + def test_r14_destroy_keeps_the_backing_file_of_a_stopped_vm(self): + self._destroy_base_of_another_vms_overlay(running=False) + + def test_r14_destroy_keeps_the_backing_file_of_a_running_vm(self): + self._destroy_base_of_another_vms_overlay(running=True) + + def test_r14_destroy_with_managed_save_removes_the_vm(self): + base = livetest.create_volume(self.conn, P + "r14-save") + dom, inst = self.vm("r14-save", [base]) + dom.create() + dom.managedSave(0) + + self.post("destroy", inst, {"delete_disk": "1"}) + + uuid = dom.UUIDString() + self.assertFalse(self.domain_exists(uuid), "VM is still defined") + self.assertFalse(Instance.objects.filter(uuid=uuid).exists()) + + def test_r14_destroy_of_a_paused_vm_removes_it(self): + base = livetest.create_volume(self.conn, P + "r14-paused") + dom, inst = self.vm("r14-paused", [base]) + dom.create() + dom.suspend() + + self.post("destroy", inst) + + self.assertFalse(self.domain_exists(dom.UUIDString()), "paused VM is still running") + + # S-02: delete a volume only after it is detached + + def _delete_attached_volume(self, pause): + base = livetest.create_volume(self.conn, P + "s02") + data = livetest.create_volume(self.conn, P + "s02-data") + dom, inst = self.vm("s02", [base, data]) + dom.create() + if pause: + dom.suspend() + + response = self.post("delete_vol", inst, {"dev": "vdb"}) + + self.assertEqual(response.status_code, 302) + + # The guest has no OS, so it never releases the disk: the live + # definition keeps vdb, and the volume must be kept with it. + self.assertIn("vdb", self.disk_sources(dom)) + self.assertTrue(livetest.volume_exists(self.conn, data), "volume deleted while the VM still uses it") + + def test_s02_delete_volume_of_a_paused_vm(self): + self._delete_attached_volume(pause=True) + + def test_s02_delete_volume_of_a_running_vm_without_guest_ack(self): + self._delete_attached_volume(pause=False) + + def test_s02_delete_volume_another_vm_uses_is_refused(self): + base = livetest.create_volume(self.conn, P + "s02-own") + shared = livetest.create_volume(self.conn, P + "s02-shared") + _, inst = self.vm("s02-a", [base, shared]) + self.vm("s02-b", [shared]) + + response = self.post("delete_vol", inst, {"dev": "vdb"}) + + self.assertEqual(response.status_code, 302) + self.assertTrue(livetest.volume_exists(self.conn, shared), "deleted another VM's disk") diff --git a/instances/views.py b/instances/views.py index 83b70457..3610c00a 100755 --- a/instances/views.py +++ b/instances/views.py @@ -30,6 +30,8 @@ from django.utils.http import url_has_allowed_host_and_scheme from django.utils.translation import gettext_noop as _ from django.views.decorators.http import require_POST from libvirt import (VIR_DOMAIN_UNDEFINE_KEEP_NVRAM, + VIR_DOMAIN_UNDEFINE_MANAGED_SAVE, + VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, VIR_DOMAIN_UNDEFINE_NVRAM, VIR_DOMAIN_START_PAUSED, libvirtError) @@ -420,6 +422,22 @@ def serialize_user_quota(func): return wrapper +def serialize_compute_mutation(func): + """Hold the whole compute: disk deletions must not race another VM attaching that disk.""" + @functools.wraps(func) + def wrapper(request, pk, *args, **kwargs): + inst = get_instance(request.user, pk) + if request.method in ("GET", "HEAD"): # e.g. the destroy confirmation page + return func(request, pk, *args, **kwargs) + try: + with libvirt_compute_lock(inst.compute): + return func(request, pk, *args, **kwargs) + except TimeoutError: + return _busy(request, pk) + + return wrapper + + DISK_FORMAT_RE = re.compile(r"^[a-z0-9]+$") DISK_SERIAL_RE = re.compile(r"^[A-Za-z0-9_.+-]*$") VOLUME_NAME_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9_.+-]*$") @@ -550,28 +568,39 @@ def force_off(request, pk): ) -@serialize_instance_mutation +@serialize_compute_mutation def destroy(request, pk): if request.method in ["POST", "DELETE"]: instance = get_instance(request.user, pk, perm_type="delete") with libvirt_instance_lock(instance): - if instance.proxy.get_status() == 1: - instance.proxy.force_shutdown() + proxy = instance.proxy + # Paused and other active states must stop too, or the VM keeps + # running as a transient domain after the undefine. + if proxy.instance.isActive(): + proxy.force_shutdown() + to_delete, shared = [], [] if request.POST.get("delete_disk", ""): - snapshots = sorted( - instance.proxy.get_snapshot(), reverse=True, key=lambda k: k["date"] - ) - for snapshot in snapshots: - instance.proxy.snapshot_delete(snapshot["name"]) - instance.proxy.delete_all_disks() + to_delete, shared = proxy.split_disk_paths_by_use() + # Undefine before deleting any disk: if it fails, nothing is lost. + # Snapshot metadata goes with the domain; internal snapshot data + # stays in the disk images that are kept. + flags = VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA if request.POST.get("delete_nvram", ""): - instance.proxy.delete(VIR_DOMAIN_UNDEFINE_NVRAM) + flags |= VIR_DOMAIN_UNDEFINE_NVRAM else: - instance.proxy.delete(VIR_DOMAIN_UNDEFINE_KEEP_NVRAM) - + flags |= VIR_DOMAIN_UNDEFINE_KEEP_NVRAM + proxy.delete(flags) instance.delete() + + for path in to_delete: + try: + proxy.get_volume_by_path(path).delete(0) + except libvirtError as err: + messages.error(request, _("Disk %(path)s was not deleted: %(err)s") % {"path": path, "err": err}) + for path in shared: + messages.warning(request, _("Disk %(path)s is used by another VM and was kept") % {"path": path}) addlogmsg( request.user.username, instance.compute.name, instance.name, _("Destroy") ) @@ -1058,7 +1087,7 @@ def edit_volume(request, pk): @require_POST @superuser_only -@serialize_instance_mutation +@serialize_compute_mutation def delete_vol(request, pk): instance = get_instance(request.user, pk, perm_type="change") dev = request.POST.get("dev", "") @@ -1081,8 +1110,24 @@ def delete_vol(request, pk): disk["storage"], ) + if disk["path"] in instance.proxy.paths_used_by_other_domains(): + messages.error( + request, + _("Volume %(vol)s is used by another VM; detach it instead") % {"vol": disk["image"]}, + ) + return redirect(request.META.get("HTTP_REFERER") + "#disks") + msg = _("Delete disk: %(dev)s") % {"dev": dev} instance.proxy.detach_disk(dev) + # Never delete a volume the guest may still be writing to. + if not instance.proxy.wait_disk_detached(dev): + messages.warning( + request, + _("The guest has not released disk %(dev)s yet; volume %(vol)s was kept. " + "Delete it from the storage pool once the VM is shut down.") + % {"dev": dev, "vol": disk["image"]}, + ) + return redirect(request.META.get("HTTP_REFERER") + "#disks") conn_delete.del_volume(disk["image"]) addlogmsg(request.user.username, instance.compute.name, instance.name, msg) diff --git a/vrtManager/instance.py b/vrtManager/instance.py index c82c4ad5..6e1b352d 100644 --- a/vrtManager/instance.py +++ b/vrtManager/instance.py @@ -838,14 +838,28 @@ class wvmInstance(wvmConnect): 0 ].getparent() xml_disk = etree.tostring(disk_el).decode() - devices = tree.find("devices") - devices.remove(disk_el) - if self.get_status() == 1: + # Paused and other active states need the live detach too. + if self.instance.isActive(): self.instance.detachDeviceFlags(xml_disk, VIR_DOMAIN_AFFECT_LIVE) - self.instance.detachDeviceFlags(xml_disk, VIR_DOMAIN_AFFECT_CONFIG) - if self.get_status() == 5: - self.instance.detachDeviceFlags(xml_disk, VIR_DOMAIN_AFFECT_CONFIG) + self.instance.detachDeviceFlags(xml_disk, VIR_DOMAIN_AFFECT_CONFIG) + + def has_disk(self, target_dev): + """True while the live or the persistent definition still has target_dev.""" + for flags in (0, VIR_DOMAIN_XML_INACTIVE): + tree = etree.fromstring(self._XMLDesc(flags)) + if tree.xpath("./devices/disk/target[@dev=$dev]", dev=target_dev): + return True + return False + + def wait_disk_detached(self, target_dev, timeout=10): + """A live detach completes only when the guest releases the disk.""" + deadline = time.monotonic() + timeout + while self.has_disk(target_dev): + if time.monotonic() >= deadline: + return False + time.sleep(0.5) + return True def edit_disk( self, @@ -1294,6 +1308,39 @@ class wvmInstance(wvmConnect): iso.append(img) return iso + def paths_used_by_other_domains(self): + """Paths another domain uses as a disk or as a backing file of one.""" + uuid = self.instance.UUIDString() + used = set() + for dom in self.wvm.listAllDomains(): + if dom.UUIDString() == uuid: + continue + for flags in (0, VIR_DOMAIN_XML_INACTIVE): + tree = etree.fromstring(dom.XMLDesc(flags)) + # "//" also reaches backingStore chains in a running domain's XML + used.update(tree.xpath("./devices/disk//source/@file|./devices/disk//source/@dev")) + # A stopped domain's XML has no backing chain; the volumes have it. + # Any volume backed by a path keeps that path, even an unattached one. + for pool in self.wvm.listAllStoragePools(): + if not pool.isActive(): + continue + with contextlib.suppress(libvirtError): + pool.refresh(0) + for vol in pool.listAllVolumes(): + with contextlib.suppress(libvirtError): + used.update(etree.fromstring(vol.XMLDesc(0)).xpath("./backingStore/path/text()")) + return used + + def split_disk_paths_by_use(self): + """This VM's disk paths, split into (only this VM, also used by another domain).""" + self.refresh_instance_pools() + own = [disk["path"] for disk in self.get_disk_devices() if disk["path"]] + used_elsewhere = self.paths_used_by_other_domains() + return ( + [path for path in own if path not in used_elsewhere], + [path for path in own if path in used_elsewhere], + ) + def delete_all_disks(self): self.refresh_instance_pools() disks = self.get_disk_devices()