fix(instances): a VM's destruction checks its files on every host first

Destroying a VM with its disk files forced it off first and then
worked out which files to delete, so a check that failed left the VM
stopped, and only this host's VMs and volumes were checked: a file on
shared storage that a VM on another host uses was deleted.

The files are now worked out before the VM is stopped: the disks it
runs with and those of its next start, as before with their snapshot
files and seed. A file a VM or volume of another of the panel's hosts
uses at the same path (shared storage is mounted at one path on every
host, as migration needs) is kept like one used here; another record
of the VM's own host is skipped, as there the VM itself uses them. A
host that cannot be read stops the destruction with its name, before
anything is stopped or deleted.

instances/templates/instances/destroy_instance_form.html

@@ -36,7 +36,7 @@
         {% endfor %}
     </ul>
     {% endif %}
-    <p class="small text-body-secondary">{% 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." %}</p>
+    <p class="small text-body-secondary">{% trans "A file another VM uses on any of the panel's hosts, directly or under its own disk, is kept, and so is the template a disk was made from. If a host cannot be checked, the VM is not destroyed." %}</p>
     {% if instance.nvram %}
     <div class="form-check mb-3">
         <input class="form-check-input" type="checkbox" name="delete_nvram" value="true" id="delete_nvram" checked>

instances/tests/test_destroy.py

@@ -7,6 +7,7 @@ from django.contrib.messages import get_messages
 from django.test import TestCase
 from django.urls import reverse
 from instances.models import Instance
+from instances.utils import paths_used_on_other_computes
 from libvirt import VIR_DOMAIN_UNDEFINE_KEEP_NVRAM, VIR_DOMAIN_UNDEFINE_NVRAM, libvirtError
 from vrtManager.util import OperationError
 
@@ -97,6 +98,32 @@ class DestroyOrderTestCase(TestCase):
         self.assertEqual(response.status_code, 302)
         proxy.get_volume_by_path.assert_called_once_with("/pool/a.qcow2")
 
+    def test_the_files_are_checked_before_the_vm_is_stopped(self):
+        with patch("instances.models.wvmInstance") as wvm:
+            proxy = wvm.return_value
+            proxy.instance.isActive.return_value = True
+            proxy.split_disk_paths_by_use.return_value = (["/pool/own.qcow2"], [])
+            self.client.post(reverse("instances:destroy", args=[self.instance.id]), {"delete_disk": "true"})
+        calls = [name for name, *_ in proxy.mock_calls if name in ("split_disk_paths_by_use", "force_shutdown", "delete")]
+        self.assertEqual(calls, ["split_disk_paths_by_use", "force_shutdown", "delete"])
+
+    def test_a_host_that_cannot_be_checked_stops_nothing(self):
+        Compute.objects.create(name="other-host", hostname="192.0.2.50", login="u", password="p", type=1)
+        def connect(host, *args):
+            if host == "192.0.2.50":
+                raise libvirtError("unreachable")
+            return MagicMock()
+
+        with patch("instances.models.wvmInstance") as wvm, patch("instances.utils.wvmConnect", side_effect=connect):
+            proxy = wvm.return_value
+            proxy.instance.isActive.return_value = True
+            proxy.split_disk_paths_by_use.side_effect = lambda elsewhere: elsewhere(["/pool/own.qcow2"])
+            response = self.client.post(reverse("instances:destroy", args=[self.instance.id]), {"delete_disk": "true"})
+        proxy.force_shutdown.assert_not_called()
+        proxy.delete.assert_not_called()
+        self.assertTrue(Instance.objects.filter(pk=self.instance.pk).exists())
+        self.assertIn("The disk files cannot be checked on other-host", str(list(get_messages(response.wsgi_request))[-1]))
+
     def test_disks_are_deleted_after_the_undefine(self):
         proxy = self.destroy()
         proxy.get_volume_by_path.assert_called_once_with("/pool/own.qcow2")
@@ -153,3 +180,46 @@ class DestroyOrderTestCase(TestCase):
             wvm.return_value.get_status.return_value = 3  # paused
             response = self.client.get(reverse("instances:destroy", args=[self.instance.id]))
         self.assertIn('id="delete_form"', response.content.decode())
+
+
+class OtherComputesTestCase(TestCase):
+    """The files of a VM another host's VM or volume uses: shared storage
+    holds them at the same path on every host that mounts it."""
+
+    def setUp(self):
+        self.source = Compute.objects.create(name="source", hostname="192.0.2.60", login="u", password="p", type=1)
+        Compute.objects.create(name="host-b", hostname="192.0.2.61", login="u", password="p", type=1)
+        Compute.objects.create(name="host-c", hostname="192.0.2.62", login="u", password="p", type=1)
+
+    def test_the_paths_another_host_uses(self):
+        hosts = self.hosts({"192.0.2.61": {"/nfs/a.qcow2", "/nfs/unrelated.qcow2"}, "192.0.2.62": {"/nfs/b.qcow2"}})
+        own = ["/nfs/a.qcow2", "/nfs/b.qcow2", "/local/c.qcow2"]
+        with patch("instances.utils.wvmConnect", side_effect=lambda host, *args: hosts[host]):
+            self.assertEqual(paths_used_on_other_computes(self.source, own), {"/nfs/a.qcow2", "/nfs/b.qcow2"})
+        # the VM's own host is checked by split_disk_paths_by_use itself; on the
+        # others too, the backing files of the VM's own volumes do not count
+        hosts["192.0.2.60"].paths_in_use.assert_not_called()
+        hosts["192.0.2.61"].paths_in_use.assert_called_once_with(own=own)
+        hosts["192.0.2.62"].paths_in_use.assert_called_once_with(own=own)
+
+    def test_another_record_of_the_vm_s_own_host_is_skipped(self):
+        # there the VM itself uses its files: they would all be kept
+        hosts = self.hosts({"192.0.2.61": {"/pool/a.qcow2"}, "192.0.2.62": set()}, same_host="192.0.2.61")
+        with patch("instances.utils.wvmConnect", side_effect=lambda host, *args: hosts[host]):
+            self.assertEqual(paths_used_on_other_computes(self.source, ["/pool/a.qcow2"]), set())
+        hosts["192.0.2.61"].paths_in_use.assert_not_called()
+
+    def hosts(self, used, same_host=None):
+        """Connections by host name; same_host is the source host under another record."""
+        hosts = {"192.0.2.60": MagicMock(**{"host_uuid.return_value": "aaaaaaaa-0000-0000-0000-000000000060"})}
+        for n, (host, paths) in enumerate(used.items()):
+            uuid = "aaaaaaaa-0000-0000-0000-000000000060" if host == same_host else f"bbbbbbbb-0000-0000-0000-00000000000{n}"
+            hosts[host] = MagicMock(**{"host_uuid.return_value": uuid, "paths_in_use.return_value": paths})
+        return hosts
+
+    def test_a_host_that_cannot_be_read_is_an_error(self):
+        hosts = self.hosts({"192.0.2.61": set(), "192.0.2.62": set()})
+        hosts["192.0.2.61"].paths_in_use.side_effect = libvirtError("connection refused")
+        with patch("instances.utils.wvmConnect", side_effect=lambda host, *args: hosts[host]):
+            with self.assertRaisesMessage(OperationError, "The disk files cannot be checked on host-b"):
+                paths_used_on_other_computes(self.source, ["/nfs/a.qcow2"])

instances/utils.py

@@ -5,13 +5,16 @@ import string
 
 from accounts.models import UserInstance, UserAttributes
 from appsettings.settings import app_settings
+from computes.models import Compute
 from django.conf import settings
 from django.db import transaction
 from django.utils.translation import gettext_lazy as _
 from libvirt import libvirtError
 from logs.audit import reason, write
 from operations.models import Operation
+from vrtManager.connection import wvmConnect
 from vrtManager.instance import wvmInstance, wvmInstances
+from vrtManager.util import OperationError
 
 from .models import Instance
 
@@ -43,6 +46,25 @@ def get_clone_free_names(size=10):
     return free_names
 
 
+def paths_used_on_other_computes(compute, own):
+    """The paths in own that the VMs and volumes of the panel's other computes
+    use (paths_in_use): shared storage holds a file at the same path on every
+    host that mounts it, as migration needs. OperationError naming a compute
+    that cannot be read: it might use them."""
+    used = set()
+    host = wvmConnect(compute.hostname, compute.login, compute.password, compute.type).host_uuid()
+    for other in Compute.objects.exclude(pk=compute.pk):
+        try:
+            conn = wvmConnect(other.hostname, other.login, other.password, other.type)
+            if host and conn.host_uuid() == host:
+                continue  # the VM's own host under another record: its VM is the one going
+            used |= conn.paths_in_use(own=own)
+        except libvirtError as err:
+            raise OperationError(_("The disk files cannot be checked on %(host)s: %(err)s") % {
+                "host": other.name, "err": err.get_error_message()}) from err
+    return used & set(own)
+
+
 def disk_gib(disks):
     """The size of the disks in whole GiB each, as the quota counts them."""
     return sum(int(disk["size"] or 0) >> 30 for disk in disks)

instances/views.py

@@ -803,15 +803,18 @@ def destroy(request, pk):
         with libvirt_instance_lock(instance):
             with audit_vm(request, instance, "delete"):
                 proxy = instance.proxy
+                # the files to delete are known before the VM is stopped: a
+                # host that cannot be checked refuses the destruction
+                to_delete, shared = [], []
+                if options.validated_data["delete_disk"]:
+                    to_delete, shared = proxy.split_disk_paths_by_use(
+                        lambda own: utils.paths_used_on_other_computes(instance.compute, own))
+
                 # 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 options.validated_data["delete_disk"]:
-                    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.

vrtManager/connection.py

@@ -408,6 +408,10 @@ class wvmConnect(object):
         """The host's capabilities, read once per object (a request): most getters below parse them"""
         return self.wvm.getCapabilities()
 
+    def host_uuid(self):
+        """The host's UUID (capabilities): compute records of one host share it"""
+        return util.get_xml_path(self.capabilities_xml, "/capabilities/host/uuid")
+
     @cached_property
     def host_info(self):
         """The host's info (architecture, memory in MiB, CPUs, ...), read once per object"""

vrtManager/instance.py

@@ -1691,18 +1691,25 @@ class wvmInstance(wvmConnect):
         are this VM's: their backing files do not count."""
         return self.paths_in_use(self.instance.UUIDString(), own)
 
-    def split_disk_paths_by_use(self):
+    def split_disk_paths_by_use(self, elsewhere=None):
         """This VM's disk paths, split into (only this VM, also used by another
-        domain). With external snapshots, the files below a disk that the
-        snapshots were taken on, and their memory files, are the VM's too, and
-        so is its cloud-init seed, in a CD-ROM drive or ejected."""
+        domain). Its disks are those it runs with and those of its next start.
+        With external snapshots, the files below a disk that the snapshots
+        were taken on, and their memory files, are the VM's too, and so is its
+        cloud-init seed, in a CD-ROM drive or ejected. elsewhere(own): the
+        paths of these that other hosts use."""
         self.refresh_instance_pools()
-        own = [disk["path"] for disk in self.get_disk_devices() if disk["path"]]
+        own = []
+        for disk in self.get_disk_devices() + self.get_disk_devices(config=True):
+            if disk["path"] and disk["path"] not in own:
+                own.append(disk["path"])
         own += self._snapshot_chain_files(own)
         seed = self._seed_path()
         if seed:
             own.append(seed)
         used_elsewhere = self.paths_used_by_other_domains(own)
+        if elsewhere:
+            used_elsewhere |= elsewhere(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:

vrtManager/tests/test_cloud_init_seed.py

@@ -88,7 +88,7 @@ class DeleteWithDisksTestCase(unittest.TestCase):
         vm.wvm = MagicMock()
         vm.refresh_instance_pools = MagicMock()
         vm.get_uuid = lambda: UUID
-        vm.get_disk_devices = lambda: [{"path": "/pool/web-1.qcow2"}]
+        vm.get_disk_devices = lambda config=False: [{"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
@@ -112,6 +112,13 @@ class DeleteWithDisksTestCase(unittest.TestCase):
         vm, _ = self.vm(seed_in_pool=False)
         self.assertEqual(vm.split_disk_paths_by_use(), (["/pool/web-1.qcow2"], []))
 
+    def test_its_next_start_disks_and_the_files_other_hosts_use(self):
+        vm, _ = self.vm(seed_in_pool=False)
+        vm.get_disk_devices = lambda config=False: [{"path": "/pool/web-1.qcow2"}] + (
+            [{"path": "/nfs/pending.qcow2"}] if config else [])
+        self.assertEqual(vm.split_disk_paths_by_use(lambda own: {"/nfs/pending.qcow2"} & set(own)),
+                         (["/pool/web-1.qcow2"], ["/nfs/pending.qcow2"]))
+
 
 class CloneTestCase(unittest.TestCase):
     def test_a_clone_does_not_take_the_vms_seed(self):

vrtManager/tests/test_external_snapshots.py

@@ -766,7 +766,7 @@ class DestroyFilesTestCase(unittest.TestCase):
                 "<devices/>", f"<devices><disk type='file' device='disk'><source file='{point}'/><target dev='vda'/></disk></devices>"
             )
         p = proxy(external=[snap for snap, _ in snaps], xml=self.TOP)
-        p.get_disk_devices = lambda: [{"path": top}]
+        p.get_disk_devices = lambda config=False: [{"path": top}]
         backing = {
             "/pool/a.two": "/pool/a.one", "/pool/a.one": "/pool/a.qcow2", "/pool/a.qcow2": "/templates/base.qcow2",
         }