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