From b72e9b1442afc076ed40f411cc622efb5f75c34a Mon Sep 17 00:00:00 2001 From: catborise Date: Fri, 30 Oct 2026 22:19:31 +0300 Subject: [PATCH] feat(instances): a VM's destruction says what goes, and keeps what a kept file needs The Destroy tab was a sub-tab with one sentence and a link; its confirmation page said only "This action is irreversible!" over a "Remove Instance's data" switch. The tab now shows the state and "Review destruction". The owner chose the confirmation layout from three rendered options: the VM and its host, that its definition, saved state and snapshot records go and that a running VM is forced off, two plans for the files (delete the disk files, or keep them), the VM's disks, what is always kept, the NVRAM choice, and Cancel. Codex reviewed destroying; this is fixed: - A disk file that cannot be deleted keeps the files below it, which it needs: they were deleted after it, leaving it broken. The files are deleted overlays first (split_disk_paths_by_use orders them by how many files each grows from), and if the files a kept one needs cannot be read, none of the rest goes. A message names each file kept so, and a failed deletion says the VM is removed. --- .../instances/destroy_instance_form.html | 72 +++++++++++-------- .../templates/instances/destroy_tab.html | 27 +++---- instances/tests/test_destroy.py | 35 ++++++++- instances/views.py | 16 ++++- vrtManager/instance.py | 20 ++++-- vrtManager/tests/test_cloud_init_seed.py | 1 + vrtManager/tests/test_external_snapshots.py | 18 +++-- 7 files changed, 128 insertions(+), 61 deletions(-) diff --git a/instances/templates/instances/destroy_instance_form.html b/instances/templates/instances/destroy_instance_form.html index 0997a48c..bbf4a15e 100644 --- a/instances/templates/instances/destroy_instance_form.html +++ b/instances/templates/instances/destroy_instance_form.html @@ -1,38 +1,52 @@ {% extends 'base.html' %} {% load i18n %} -{% block title %} -{% trans "Confirm Destroy" %} -{% endblock title %} +{% block title %}{% blocktrans with name=instance.name %}Destroy {{ name }}{% endblocktrans %}{% endblock title %} {% block page_heading %} -{% trans "Destroy instance" %} {{ instance }} +{% blocktrans with name=instance.name %}Destroy {{ name }}{% endblocktrans %} {% blocktrans with host=instance.compute.name %}on {{ host }}{% endblocktrans %} {% endblock page_heading %} {% block content %} -{% if can_delete %} -
- {% trans "This action is irreversible!" %} -
-
{% csrf_token %} -
- - +

{% trans "The VM's definition, saved state and snapshot records are removed. A running VM is forced off first; what the guest has not written to disk is lost." %}

+{% csrf_token %} +
+ {% trans "What should happen to the files?" %} +
+
+ +
+
+ +
- {% if instance.nvram %} -
- - -
- {% endif %} - - -{% else %} -
+ {% if instance.config_disks %} +

{% trans "Disks" %}

+
    + {% for disk in instance.config_disks %} +
  • {{ disk.dev }} {{ disk.path|default:_("no file") }}
    • + {% endfor %} +
+ {% endif %} +

{% trans "A file another VM on this host uses, directly or under its own disk, is kept, and so is the template a disk was made from." %}

+ {% if instance.nvram %} +
+ +
-{% endif %} -{% endblock content %} \ No newline at end of file + {% endif %} +
+ {% trans "This cannot be undone." %} + {% trans "Cancel" %} + +
+ +{% endblock content %} diff --git a/instances/templates/instances/destroy_tab.html b/instances/templates/instances/destroy_tab.html index cd97ea26..5cf6d36c 100644 --- a/instances/templates/instances/destroy_tab.html +++ b/instances/templates/instances/destroy_tab.html @@ -1,27 +1,16 @@ {% load i18n %} +{% load vm_states %}
- + {% vm_state instance.status as state %}
-
- -
- -
-
-

{% trans 'This action starts remove instance process' %}

+
+

{% trans "Destroy" %} ยท {{ state.0 }}

{% if can_delete %} - {% trans "Destroy" %} + {% trans "Review destruction" %} +

{% trans "The next page shows what goes and lets you keep the disk files." %}

{% else %} - +

{% trans "Only the owners allowed to delete this VM and administrators can destroy it." %}

{% endif %} -
-
-
\ No newline at end of file +
diff --git a/instances/tests/test_destroy.py b/instances/tests/test_destroy.py index 4dd7948e..045f6b3d 100644 --- a/instances/tests/test_destroy.py +++ b/instances/tests/test_destroy.py @@ -1,5 +1,5 @@ from contextlib import contextmanager -from unittest.mock import patch +from unittest.mock import MagicMock, patch from computes.models import Compute from django.contrib.auth import get_user_model @@ -64,6 +64,39 @@ class DestroyOrderTestCase(TestCase): self.assertEqual(response.status_code, 200) self.assertEqual(proxy.get_volume_by_path.called, deleted) + def test_a_disk_file_that_stays_keeps_the_files_it_needs(self): + # the overlay is not deleted: the file below it would leave it broken + volumes = {} + + def volume(path): + vol = volumes.setdefault(path, MagicMock()) + if path == "/pool/vm.snap1": + vol.delete.side_effect = libvirtError("Permission denied") + return vol + + 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/vm.snap1", "/pool/vm.qcow2", "/pool/vm-data.qcow2"], []) + proxy.get_volume_by_path.side_effect = volume + proxy.backing_path.side_effect = {"/pool/vm.snap1": "/pool/vm.qcow2"}.get + response = self.client.post(reverse("instances:destroy", args=[self.instance.id]), {"delete_disk": "true"}) + self.assertNotIn("/pool/vm.qcow2", volumes) + volumes["/pool/vm-data.qcow2"].delete.assert_called_once_with(0) + self.assertEqual([str(m) for m in get_messages(response.wsgi_request)][-1], + "Disk /pool/vm.qcow2 was kept: a disk file that stays needs it") + + def test_when_the_files_a_kept_one_needs_are_unknown_none_of_the_rest_goes(self): + 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/a.qcow2", "/pool/b.qcow2"], []) + proxy.get_volume_by_path.return_value.delete.side_effect = libvirtError("Permission denied") + proxy.backing_path.side_effect = libvirtError("pool unreachable") + response = self.client.post(reverse("instances:destroy", args=[self.instance.id]), {"delete_disk": "true"}) + self.assertEqual(response.status_code, 302) + proxy.get_volume_by_path.assert_called_once_with("/pool/a.qcow2") + def test_disks_are_deleted_after_the_undefine(self): proxy = self.destroy() proxy.get_volume_by_path.assert_called_once_with("/pool/own.qcow2") diff --git a/instances/views.py b/instances/views.py index d3fa90ff..a443e94c 100755 --- a/instances/views.py +++ b/instances/views.py @@ -822,13 +822,25 @@ def destroy(request, pk): proxy.delete(flags) instance.delete() - # each disk has its own row: the VM is gone even if one is kept + # each disk has its own row: the VM is gone even if one is kept; a + # file that stays keeps the files below it, which it needs + kept = set() for path in to_delete: + if path in kept: + messages.warning(request, _("Disk %(path)s was kept: a disk file that stays needs it") % {"path": path}) + continue try: with audit_vm(request, instance, f"delete volume {path}"): 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": error_text(request, err)}) + messages.error(request, _("The VM is removed, but disk %(path)s was not deleted: %(err)s") % { + "path": path, "err": error_text(request, err)}) + try: + while (path := proxy.backing_path(path)) and path not in kept: + kept.add(path) + except libvirtError: + # the files it needs are unknown: none of the rest goes + kept.update(to_delete) for path in shared: messages.warning(request, _("Disk %(path)s is used by another VM and was kept") % {"path": path}) return redirect(reverse("instances:index")) diff --git a/vrtManager/instance.py b/vrtManager/instance.py index 0fce043b..24ffadc2 100644 --- a/vrtManager/instance.py +++ b/vrtManager/instance.py @@ -1705,10 +1705,18 @@ class wvmInstance(wvmConnect): used_elsewhere = self.paths_used_by_other_domains(own) # a kept file keeps the files below it for path in [path for path in own if path in used_elsewhere]: - while (path := self._backing_path(path)) and path not in used_elsewhere: + while (path := self.backing_path(path)) and path not in used_elsewhere: used_elsewhere.add(path) + def below(path): + """How many files the path grows from: an overlay is deleted before + the files it needs, so one that cannot be deleted keeps them.""" + seen = set() + while (path := self.backing_path(path)) and path not in seen: + seen.add(path) + return len(seen) + return ( - [path for path in own if path not in used_elsewhere], + sorted((path for path in own if path not in used_elsewhere), key=below, reverse=True), [path for path in own if path in used_elsewhere], ) @@ -1739,7 +1747,7 @@ class wvmInstance(wvmConnect): points.update(self._disk_sources(saved).values()) files = list(tops) for path in tops: - while (path := self._backing_path(path)) in points and path not in files: + while (path := self.backing_path(path)) in points and path not in files: files.append(path) grew = True while grew: @@ -1748,14 +1756,14 @@ class wvmInstance(wvmConnect): if top in files: # reached by a branch in this pass continue branch, path = [top], top - while (path := self._backing_path(path)) in points | overlays and path not in files: + while (path := self.backing_path(path)) in points | overlays and path not in files: branch.append(path) if path in files: files += branch grew = True return files[len(tops):] + memory - def _backing_path(self, path): + def backing_path(self, path): """The backing file of a volume; None if it has none or is in no pool. Any other error is raised: a chain cut short would let a file a kept one needs count as deletable.""" @@ -2026,7 +2034,7 @@ class wvmInstance(wvmConnect): # do not keep each other, but a file in use keeps the files below it in_use = self.paths_in_use(own=left) for path in [path for path in left if path in in_use]: - while (path := self._backing_path(path)) and path not in in_use: + while (path := self.backing_path(path)) and path not in in_use: in_use.add(path) left = [path for path in left if path not in in_use] return left diff --git a/vrtManager/tests/test_cloud_init_seed.py b/vrtManager/tests/test_cloud_init_seed.py index c610c19f..75b5fb17 100644 --- a/vrtManager/tests/test_cloud_init_seed.py +++ b/vrtManager/tests/test_cloud_init_seed.py @@ -91,6 +91,7 @@ class DeleteWithDisksTestCase(unittest.TestCase): vm.get_disk_devices = lambda: [{"path": "/pool/web-1.qcow2"}] vm._snapshot_chain_files = lambda own: [] vm.paths_used_by_other_domains = lambda own: set() + vm.backing_path = lambda path: None pool = MagicMock() if seed_in_pool: pool.storageVolLookupByName.return_value.path.return_value = f"/pool/cidata-{UUID}.iso" diff --git a/vrtManager/tests/test_external_snapshots.py b/vrtManager/tests/test_external_snapshots.py index 0e5c2d72..c985f6b0 100644 --- a/vrtManager/tests/test_external_snapshots.py +++ b/vrtManager/tests/test_external_snapshots.py @@ -611,7 +611,7 @@ class LeftBehindTestCase(unittest.TestCase): def test_a_file_another_vm_uses_is_not_reported(self): # on NFS libvirt could not remove it, but another VM uses it now with patch.object(wvmInstance, "paths_in_use", return_value={"/pool/a.one"}) as in_use, \ - patch.object(wvmInstance, "_backing_path", return_value=None): + patch.object(wvmInstance, "backing_path", return_value=None): left = self.run_delete({"/pool/a.one", "/pool/vm.one.mem", "/pool/a.two", "/pool/a.qcow2"}) self.assertEqual(left, ["/pool/vm.one.mem"]) # their own backing files do not keep each other @@ -622,7 +622,7 @@ class LeftBehindTestCase(unittest.TestCase): # file, and the in-use top's own backing reference does not count backing = {"/pool/a.one": "/pool/vm.one.mem"} with patch.object(wvmInstance, "paths_in_use", return_value={"/pool/a.one"}), \ - patch.object(wvmInstance, "_backing_path", side_effect=backing.get): + patch.object(wvmInstance, "backing_path", side_effect=backing.get): left = self.run_delete({"/pool/a.one", "/pool/vm.one.mem", "/pool/a.two", "/pool/a.qcow2"}) self.assertEqual(left, []) @@ -774,6 +774,9 @@ class DestroyFilesTestCase(unittest.TestCase): backing[top] = "/pool/a.one" if extra: backing[extra[0]] = extra[1] + # the template's own chain, longer than the VM's files + template = ["/templates/base.qcow2"] + [f"/templates/base{n}.qcow2" for n in range(1, 10)] + backing.update(zip(template, template[1:])) if other_overlay_on: backing["/pool/other.qcow2"] = other_overlay_on @@ -798,10 +801,10 @@ class DestroyFilesTestCase(unittest.TestCase): missing = libvirtError("no volume") missing.err = (libvirt.VIR_ERR_NO_STORAGE_VOL,) p.get_volume_by_path = MagicMock(side_effect=missing) - self.assertIsNone(p._backing_path("/elsewhere/base.qcow2")) # in no pool + self.assertIsNone(p.backing_path("/elsewhere/base.qcow2")) # in no pool p.get_volume_by_path = MagicMock(side_effect=libvirtError("connection lost")) with self.assertRaises(libvirtError): - p._backing_path("/pool/a.one") + p.backing_path("/pool/a.one") def test_the_snapshot_chain_and_memory_go_but_not_the_template(self): delete, keep = self.split() @@ -822,6 +825,13 @@ class DestroyFilesTestCase(unittest.TestCase): ) self.assertEqual(keep, []) + def test_an_overlay_is_deleted_before_the_files_it_needs(self): + # after a revert the branch a.two comes before a.one: if it cannot be deleted, a.one is still there + delete, _ = self.split(top="/pool/a.reverted") + for overlay, below in (("/pool/a.two", "/pool/a.one"), ("/pool/a.reverted", "/pool/a.one"), + ("/pool/a.one", "/pool/a.qcow2")): + self.assertLess(delete.index(overlay), delete.index(below), (overlay, below)) + def test_a_snapshot_file_off_the_disks_chain_stays(self): # a disk detached since: its snapshot overlay grows from a file the # VM's disks do not