fix(storages): keep volumes a VM still needs

libvirt deletes any volume it is asked to. The storage page and the API
deleted a VM's disk, the base image of another volume or the file a
snapshot was taken on without a check, breaking the VM or its
snapshots.

wvmStorage.del_volume now refuses a volume a domain on the compute uses
as a disk or a backing file of one, a file of a domain's snapshots, or
the backing file of another volume. The check is the one destroy
already used, moved to wvmConnect and extended to snapshot files, so
destroy also keeps the files of another VM's snapshots. The storage
page shows the refusal; the API answers 400 with it, as for other
refusals. Deleting a disk from a VM refuses after the detach when the
VM's own snapshots need the volume, and says the volume was kept.

Network disks count by their source name, pool volume disks by their
volume's path. A volume whose metadata cannot be read stops the check
instead of counting as unused; only one removed meanwhile is skipped.
The check and the delete hold the compute lock, so no VM operation
attaches the volume in between; a busy compute is a message on the page
and 409 in the API.

vrtManager/connection.py

@@ -11,6 +11,7 @@ import libvirt
 from django.conf import settings
 from django.utils.functional import cached_property
 from libvirt import libvirtError
+from lxml import etree
 from vrtManager import util
 from vrtManager.rwlock import ReadWriteLock
 
@@ -946,6 +947,59 @@ class wvmConnect(object):
             instances.append(name)
         return instances
 
+    def paths_in_use(self, skip_uuid=None, own=()):
+        """Paths (names for network disks) a domain uses as a disk or a
+        backing file of one, or as a file of its snapshots (overlays, memory,
+        the disks they were taken on); and the backing files of volumes.
+        skip_uuid leaves out one domain, own the volumes whose backing files
+        do not count."""
+        used = set()
+        for dom in self.wvm.listAllDomains():
+            if dom.UUIDString() == skip_uuid:
+                continue
+            for flags in (0, libvirt.VIR_DOMAIN_XML_INACTIVE):
+                used.update(self._source_paths(etree.fromstring(dom.XMLDesc(flags))))
+            for snap in dom.listAllSnapshots(0):
+                tree = etree.fromstring(snap.getXMLDesc(0))
+                used.update(tree.xpath("disks/disk/source/@file|memory/@file"))
+                for saved in tree.xpath("domain|inactiveDomain"):
+                    used.update(self._source_paths(saved))
+        # 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():
+                # only a volume removed meanwhile is skipped: one that cannot
+                # be read may still keep a backing file
+                try:
+                    if vol.path() not in own:
+                        used.update(etree.fromstring(vol.XMLDesc(0)).xpath("./backingStore/path/text()"))
+                except libvirtError as err:
+                    if err.get_error_code() != libvirt.VIR_ERR_NO_STORAGE_VOL:
+                        raise
+        return used
+
+    def _source_paths(self, domain):
+        """The files, devices or network names of a domain definition's
+        disks and of their backing chains ("//" reaches a running domain's
+        backingStore); a pool volume source as its volume's path."""
+        paths = []
+        for source in domain.xpath("devices/disk//source"):
+            if source.get("pool") and source.get("volume"):
+                try:
+                    pool = self.wvm.storagePoolLookupByName(source.get("pool"))
+                    paths.append(pool.storageVolLookupByName(source.get("volume")).path())
+                except libvirtError as err:
+                    # a volume that is not there cannot be the one deleted
+                    if err.get_error_code() not in (libvirt.VIR_ERR_NO_STORAGE_VOL, libvirt.VIR_ERR_NO_STORAGE_POOL):
+                        raise
+            else:
+                paths += [v for v in (source.get("file"), source.get("dev"), source.get("name")) if v]
+        return paths
+
     def get_snapshots(self):
         instance = []
         for snap_id in self.wvm.listDomainsID():

vrtManager/instance.py

@@ -1480,29 +1480,9 @@ class wvmInstance(wvmConnect):
         return iso
 
     def paths_used_by_other_domains(self, own=()):
-        """Paths another domain uses as a disk or as a backing file of one.
-        The volumes in own are this VM's: their backing files do not count."""
-        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):
-                    if vol.path() not in own:
-                        used.update(etree.fromstring(vol.XMLDesc(0)).xpath("./backingStore/path/text()"))
-        return used
+        """Paths another domain uses (see paths_in_use). The volumes in own
+        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):
         """This VM's disk paths, split into (only this VM, also used by another

vrtManager/storage.py

@@ -238,7 +238,10 @@ class wvmStorage(wvmConnect):
         return vol.XMLDesc(0)
 
     def del_volume(self, name):
+        """libvirt deletes any volume; one a VM still needs is refused."""
         vol = self.pool.storageVolLookupByName(name)
+        if vol.path() in self.paths_in_use():
+            raise util.OperationError(f"Volume {name} is in use: a VM's disk, a backing file or a snapshot's file")
         vol.delete(0)
 
     def get_volume_format_type(self, name):

vrtManager/tests/test_volume_in_use.py

@@ -0,0 +1,138 @@
+"""A volume a VM still needs is never deleted from a storage pool: not a
+disk, not a backing file, not a file of a snapshot. libvirt deletes any
+volume it is asked to; the live cases are in instances/tests/test_live_dataloss.py."""
+
+import unittest
+from unittest.mock import MagicMock
+
+import libvirt
+from django.conf import settings
+from libvirt import libvirtError
+
+if not settings.configured:
+    settings.configure(MAC_OUI="52:54:10")
+
+from vrtManager import util
+from vrtManager.storage import wvmStorage
+
+
+def domain(uuid, disks=(), snapshots=()):
+    dom = MagicMock()
+    dom.UUIDString.return_value = uuid
+    devices = "".join(f"<disk type='file' device='disk'><source file='{d}'/></disk>" for d in disks)
+    dom.XMLDesc.return_value = f"<domain><devices>{devices}</devices></domain>"
+    snaps = []
+    for xml in snapshots:
+        snap = MagicMock()
+        snap.getXMLDesc.return_value = xml
+        snaps.append(snap)
+    dom.listAllSnapshots.return_value = snaps
+    return dom
+
+
+def volume(path, backing=None):
+    vol = MagicMock()
+    vol.path.return_value = path
+    parent = f"<backingStore><path>{backing}</path></backingStore>" if backing else ""
+    vol.XMLDesc.return_value = f"<volume>{parent}</volume>"
+    return vol
+
+
+def storage(domains=(), volumes=()):
+    conn = wvmStorage.__new__(wvmStorage)
+    conn.wvm = MagicMock()
+    conn.wvm.listAllDomains.return_value = list(domains)
+    pool = MagicMock(**{"isActive.return_value": True})
+    pool.listAllVolumes.return_value = list(volumes)
+    by_name = {v.path().rsplit("/", 1)[1]: v for v in volumes}
+    pool.storageVolLookupByName.side_effect = lambda name: by_name[name]
+    conn.wvm.listAllStoragePools.return_value = [pool]
+    conn.pool = pool
+    return conn, by_name
+
+
+SNAPSHOT = (
+    "<domainsnapshot><name>s</name><memory snapshot='external' file='/pool/vm.s.mem'/>"
+    "<disks><disk name='vda' snapshot='external'><source file='/pool/vm.s'/></disk></disks>"
+    "<domain><devices><disk type='file' device='disk'><source file='/pool/vm.qcow2'/></disk></devices></domain>"
+    "</domainsnapshot>"
+)
+
+
+class PathsInUseTestCase(unittest.TestCase):
+    def test_disks_backing_files_and_snapshot_files_are_in_use(self):
+        conn, _ = storage(
+            domains=[domain("u1", ["/pool/vm.now"], [SNAPSHOT])],
+            volumes=[volume("/pool/clone.qcow2", backing="/pool/template.qcow2")],
+        )
+        self.assertEqual(
+            conn.paths_in_use(),
+            {"/pool/vm.now", "/pool/vm.s", "/pool/vm.s.mem", "/pool/vm.qcow2", "/pool/template.qcow2"},
+        )
+
+    def test_one_domain_and_the_backing_files_of_own_volumes_can_be_left_out(self):
+        conn, _ = storage(
+            domains=[domain("u1", ["/pool/a.qcow2"]), domain("u2", ["/pool/b.qcow2"])],
+            volumes=[volume("/pool/a.qcow2", backing="/pool/base.qcow2")],
+        )
+        self.assertEqual(conn.paths_in_use(skip_uuid="u1", own=["/pool/a.qcow2"]), {"/pool/b.qcow2"})
+
+
+    def test_a_network_disk_is_in_use_by_its_name(self):
+        dom = domain("u1")
+        dom.XMLDesc.return_value = (
+            "<domain><devices><disk type='network' device='disk'>"
+            "<source protocol='rbd' name='rbdpool/disk'/></disk></devices></domain>"
+        )
+        conn, _ = storage(domains=[dom])
+        self.assertIn("rbdpool/disk", conn.paths_in_use())
+
+    def test_a_pool_volume_disk_is_in_use_by_its_path(self):
+        for xml in (
+            "<domain><devices>{}</devices></domain>",
+            "<domain/>",  # in a snapshot's saved definition
+        ):
+            with self.subTest(xml):
+                disk = "<disk type='volume' device='disk'><source pool='default' volume='disk.qcow2'/></disk>"
+                dom = domain("u1")
+                if "{}" in xml:
+                    dom.XMLDesc.return_value = xml.format(disk)
+                else:
+                    snap = MagicMock()
+                    snap.getXMLDesc.return_value = f"<domainsnapshot><domain><devices>{disk}</devices></domain></domainsnapshot>"
+                    dom.listAllSnapshots.return_value = [snap]
+                conn, _ = storage(domains=[dom], volumes=[volume("/pool/disk.qcow2")])
+                conn.wvm.storagePoolLookupByName.return_value = conn.pool
+                self.assertIn("/pool/disk.qcow2", conn.paths_in_use())
+
+    def test_a_volume_that_cannot_be_read_is_not_taken_as_unused(self):
+        removed, unreadable = volume("/pool/gone.qcow2"), volume("/pool/top.qcow2")
+        error = libvirtError("removed")
+        error.err = (libvirt.VIR_ERR_NO_STORAGE_VOL,)
+        removed.XMLDesc.side_effect = error
+        conn, _ = storage(volumes=[removed])
+        self.assertEqual(conn.paths_in_use(), set())
+        unreadable.XMLDesc.side_effect = libvirtError("permission denied")
+        conn, _ = storage(volumes=[unreadable])
+        with self.assertRaises(libvirtError):
+            conn.paths_in_use()
+
+
+class DeleteVolumeTestCase(unittest.TestCase):
+    def test_a_volume_in_use_is_refused(self):
+        for case, domains, volumes in (
+            ("disk", [domain("u1", ["/pool/disk.qcow2"])], [volume("/pool/disk.qcow2")]),
+            ("backing file", [], [volume("/pool/disk.qcow2"), volume("/pool/top.qcow2", backing="/pool/disk.qcow2")]),
+            ("snapshot point", [domain("u1", ["/pool/vm.s"], [SNAPSHOT.replace("vm.qcow2", "disk.qcow2")])],
+             [volume("/pool/disk.qcow2")]),
+        ):
+            with self.subTest(case):
+                conn, vols = storage(domains, volumes)
+                with self.assertRaisesRegex(util.OperationError, "disk.qcow2 is in use"):
+                    conn.del_volume("disk.qcow2")
+                vols["disk.qcow2"].delete.assert_not_called()
+
+    def test_an_unused_volume_is_deleted(self):
+        conn, vols = storage([domain("u1", ["/pool/other.qcow2"])], [volume("/pool/disk.qcow2")])
+        conn.del_volume("disk.qcow2")
+        vols["disk.qcow2"].delete.assert_called_once_with(0)