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" %}

+ + {% 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