perf(storages): a pool's volumes are read with one libvirt call each

wvmStorage.update_volumes looked each volume up by name three times,
read its info twice and its XML once: six round trips per volume to
the host. It now lists the pool's volumes as objects (listAllVolumes)
and reads each one's XML only (volume_details): name, capacity,
allocation and format are all in it, and libvirt refreshes a volume
for its XML as it does for info(). A pool of N volumes takes 2 + N
calls instead of 2 + 6N; 100 volumes over SSH on a test host read in
about 110 ms instead of 720 ms, with the same values.

- A volume deleted between the listing and its read is left out; any
  other error is raised. Volumes come sorted by name.
- get_volume_details (the API's one volume) reads it the same way.
- Only a refused refresh (a clone running in the pool) is tolerated,
  not any exception.
- The pool page reads its volumes and space only to render itself;
  its actions, which all redirect, no longer read every volume first.
- get_volume_size and get_volume_allocation go: nothing else used them.

storages/views.py

@@ -136,24 +136,6 @@ def storage(request, compute_id, pool):
 
     storages = conn.get_storages()
     state = conn.is_active()
-    # first: it refreshes the pool, and libvirt measures a pool's space only then
-    volumes = conn.update_volumes() if state else None
-    for volume in volumes or []:
-        volume["iso"] = is_iso(volume["name"])
-    try:
-        size, free = conn.get_size()
-        status = conn.get_status()
-        used = size - free
-        if state and size:  # an iSCSI pool without LUNs has no size
-            percent = (used * 100) // size
-        else:
-            percent = 0
-    except libvirtError:  # both read the pool's info: the page says it could not be read
-        size = free = used = percent = status = None
-
-    path = conn.get_target_path() or conn.get_source_name()  # an RBD pool: its Ceph pool
-    type = conn.get_type()
-    autostart = conn.get_autostart()
 
     if request.method == "POST":
         subject = f"Storage pool {pool}"
@@ -222,6 +204,25 @@ def storage(request, compute_id, pool):
                     messages.error(request, msg_err.as_text())
             return redirect(reverse("storage", args=[compute.id, pool]))
 
+    # read only for the page: every action above redirects
+    # first: it refreshes the pool, and libvirt measures a pool's space only then
+    volumes = conn.update_volumes() if state else None
+    for volume in volumes or []:
+        volume["iso"] = is_iso(volume["name"])
+    try:
+        size, free = conn.get_size()
+        status = conn.get_status()
+        used = size - free
+        if state and size:  # an iSCSI pool without LUNs has no size
+            percent = (used * 100) // size
+        else:
+            percent = 0
+    except libvirtError:  # both read the pool's info: the page says it could not be read
+        size = free = used = percent = status = None
+
+    path = conn.get_target_path() or conn.get_source_name()  # an RBD pool: its Ceph pool
+    type = conn.get_type()
+    autostart = conn.get_autostart()
     job_resource = claims.pool_key(compute.id, pool)
     conn.close()
 

vrtManager/storage.py

@@ -5,6 +5,20 @@ from vrtManager import util
 from vrtManager.connection import wvmConnect
 
 
+def volume_details(vol):
+    """A volume's name, capacity (size), allocation and format, from its XML:
+    one libvirt call, which refreshes the volume as info() does. Logical and
+    iSCSI volumes have no format (None)."""
+    def read(doc):
+        return {
+            "name": util.get_xpath(doc, "/volume/name"),
+            "size": int(util.get_xpath(doc, "/volume/capacity")),
+            "allocation": int(util.get_xpath(doc, "/volume/allocation")),
+            "type": util.get_xpath(doc, "/volume/target/format/@type"),
+        }
+    return util.get_xml_path(vol.XMLDesc(0), func=read)
+
+
 class wvmStorages(wvmConnect):
     def get_storages_info(self):
         """Each pool's state, autostart, type, path (an RBD pool's Ceph pool),
@@ -255,14 +269,6 @@ class wvmStorage(wvmConnect):
     def get_volume(self, name):
         return self.pool.storageVolLookupByName(name)
 
-    def get_volume_size(self, name):
-        vol = self.get_volume(name)
-        return vol.info()[1]
-
-    def get_volume_allocation(self, name):
-        vol = self.get_volume(name)
-        return vol.info()[2]
-
     def _vol_XMLDesc(self, name):
         vol = self.get_volume(name)
         return vol.XMLDesc(0)
@@ -286,28 +292,25 @@ class wvmStorage(wvmConnect):
         return self.pool.refresh(0)
 
     def get_volume_details(self, volname):
-        with contextlib.suppress(Exception):
+        """The volume's details (volume_details), the pool refreshed first."""
+        with contextlib.suppress(libvirt.libvirtError):  # refused while a volume is cloned in the pool
             self.refresh()
-        return {
-            "name": volname,
-            "size": self.get_volume_size(volname),
-            "allocation": self.get_volume_allocation(volname),
-            "type": self.get_volume_format_type(volname),
-        }
+        return volume_details(self.get_volume(volname))
 
     def update_volumes(self):
-        with contextlib.suppress(Exception):
+        """Every volume's details (volume_details) by name, the pool refreshed
+        first: two calls and one per volume, where a lookup, info() and the
+        XML of each took six. A volume deleted meanwhile is left out."""
+        with contextlib.suppress(libvirt.libvirtError):  # refused while a volume is cloned in the pool
             self.refresh()
-        vols = self.get_volumes()
-        return [
-            {
-                "name": volname,
-                "size": self.get_volume_size(volname),
-                "allocation": self.get_volume_allocation(volname),
-                "type": self.get_volume_format_type(volname),
-            }
-            for volname in vols
-        ]
+        volumes = []
+        for vol in self.pool.listAllVolumes(0):
+            try:
+                volumes.append(volume_details(vol))
+            except libvirt.libvirtError as err:
+                if err.get_error_code() != libvirt.VIR_ERR_NO_STORAGE_VOL:
+                    raise
+        return sorted(volumes, key=lambda volume: volume["name"])
 
     def create_volume(
         self,

vrtManager/tests/test_host_reads.py

@@ -18,6 +18,7 @@ from libvirt import (
     VIR_DOMAIN_PAUSED,
     VIR_DOMAIN_RUNNING,
     VIR_DOMAIN_SHUTOFF,
+    VIR_ERR_NO_STORAGE_VOL,
     VIR_ERR_NO_SUPPORT,
     VIR_ERR_OPERATION_DENIED,
     VIR_ERR_XML_ERROR,
@@ -25,7 +26,7 @@ from libvirt import (
 )
 from vrtManager.connection import wvmConnect
 from vrtManager.hostdetails import wvmHostDetails
-from vrtManager.storage import wvmStorages
+from vrtManager.storage import wvmStorage, wvmStorages
 
 CAPABILITIES = """<capabilities><host><cpu><arch>x86_64</arch></cpu></host>
 <guest><os_type>hvm</os_type><arch name='x86_64'><emulator>/usr/bin/qemu-kvm</emulator>
@@ -217,3 +218,55 @@ class StoragesInfoTestCase(unittest.TestCase):
         conn.get_storage = lambda name: pool
         conn.get_storages_info()
         pool.refresh.assert_not_called()
+
+
+def volume(name, capacity, allocation, fmt=None, error=None):
+    target = f"<target><format type='{fmt}'/></target>" if fmt else "<target/>"
+    xml = (f"<volume type='file'><name>{name}</name><capacity unit='bytes'>{capacity}</capacity>"
+           f"<allocation unit='bytes'>{allocation}</allocation>{target}</volume>")
+    return MagicMock(**{"XMLDesc.side_effect": error} if error else {"XMLDesc.return_value": xml})
+
+
+def gone(code):
+    err = libvirtError("no storage vol")
+    err.get_error_code = lambda: code
+    return err
+
+
+class VolumesTestCase(unittest.TestCase):
+    """A pool's volumes are read with one call each, their XML, after one listing."""
+
+    def storage(self, volumes):
+        stg = wvmStorage.__new__(wvmStorage)  # no libvirt connection: only the pool's calls
+        stg.pool = MagicMock(**{"listAllVolumes.return_value": volumes})
+        return stg
+
+    def test_each_volume_is_one_call(self):
+        vols = [volume("b.qcow2", 2048, 512, "qcow2"), volume("a-lv", 1024, 1024)]
+        stg = self.storage(vols)
+        self.assertEqual(stg.update_volumes(), [
+            {"name": "a-lv", "size": 1024, "allocation": 1024, "type": None},  # a logical volume has no format
+            {"name": "b.qcow2", "size": 2048, "allocation": 512, "type": "qcow2"},
+        ])
+        self.assertEqual([c[0] for c in stg.pool.mock_calls], ["refresh", "listAllVolumes"])
+        for vol in vols:
+            self.assertEqual([c[0] for c in vol.mock_calls], ["XMLDesc"])
+
+    def test_a_volume_deleted_meanwhile_is_left_out(self):
+        stg = self.storage([volume("a.img", 1, 1, "raw"), volume("b.img", 1, 1, error=gone(VIR_ERR_NO_STORAGE_VOL))])
+        self.assertEqual([v["name"] for v in stg.update_volumes()], ["a.img"])
+        # any other failure is not hidden
+        stg = self.storage([volume("c.img", 1, 1, error=gone(VIR_ERR_OPERATION_DENIED))])
+        with self.assertRaises(libvirtError):
+            stg.update_volumes()
+
+    def test_a_refused_refresh_still_lists(self):
+        # libvirt refuses to refresh a pool while a volume is cloned in it
+        stg = self.storage([volume("a.img", 1, 1, "raw")])
+        stg.pool.refresh.side_effect = libvirtError("pool has asynchronous jobs running")
+        self.assertEqual(len(stg.update_volumes()), 1)
+
+    def test_one_volume(self):
+        stg = self.storage([])
+        stg.pool.storageVolLookupByName.return_value = volume("a.img", 4096, 0, "raw")
+        self.assertEqual(stg.get_volume_details("a.img"), {"name": "a.img", "size": 4096, "allocation": 0, "type": "raw"})