perf(instances): the VM page reads each disk, the state and host info once

Measured on a VM with two disks, a CD-ROM and a NIC: 70 libvirt calls per
page, now 43. Within the page's cached_reads block the VM's state, the
host's info and domain capabilities, and each disk path's volume are read
once; the running and the persistent disk lists share a path's volume.
The ISO list takes the active pools in one call instead of looking each up
by name; it still refreshes them, so an image copied in by hand shows up.

The opt-in DRBD status waits at most 10 s for the compute's ssh, then
leaves the status unknown instead of holding the page.

instances/test_detail_reads.py

@@ -1,5 +1,6 @@
-"""The VM page reads each kind of domain XML and the host capabilities once,
-through the real getters, on libvirt's test driver."""
+"""The VM page reads each kind of domain XML, the VM's state, the host's
+capabilities and info, and each disk's volume once, through the real getters,
+on libvirt's test driver."""
 
 from unittest.mock import patch
 
@@ -10,17 +11,22 @@ from django.test import TestCase
 from django.urls import reverse
 from instances.models import Instance
 
-# a VM with a VNC console, on libvirt's test driver
-DOMAIN = """<domain type='test'><name>read-cache-vm</name><memory unit='MiB'>128</memory><vcpu>1</vcpu>
+# a VM with a disk and a VNC console, on libvirt's test driver
+DOMAIN = """<domain type='test'><name>read-cache-vm</name><uuid>0b9f6a52-6d0e-4c1f-9a51-2f3c1d7e8a40</uuid><memory unit='MiB'>128</memory><vcpu>1</vcpu>
 <os><type arch='i686'>hvm</type></os>
-<devices><graphics type='vnc' port='-1' listen='127.0.0.1'><listen type='address' address='127.0.0.1'/></graphics></devices>
+<devices><disk type='file' device='disk'><source file='/default-pool/read-cache.img'/><target dev='vda' bus='virtio'/></disk>
+<graphics type='vnc' port='-1' listen='127.0.0.1'><listen type='address' address='127.0.0.1'/></graphics></devices>
 </domain>"""
 
 
 class DetailReadsTests(TestCase):
     def setUp(self):
-        self.conn = libvirt.open("test:///default")  # each open is a fresh, private test host
+        self.conn = libvirt.open("test:///default")
         self.addCleanup(self.conn.close)
+        # the test host lives on while a libvirt object still refers to it
+        pool = self.conn.storagePoolLookupByName("default-pool")
+        if "read-cache.img" not in pool.listVolumes():
+            pool.createXML("<volume><name>read-cache.img</name><capacity>1048576</capacity></volume>", 0)
         domain = self.conn.defineXML(DOMAIN)
         self.compute = Compute.objects.create(name="testdrv", hostname="localhost", login="", password="", type=4)
         self.vm = Instance.objects.create(compute=self.compute, name=domain.name(), uuid=domain.UUIDString())
@@ -56,6 +62,43 @@ class DetailReadsTests(TestCase):
         self.assertEqual(sorted(xml_reads), sorted(set(xml_reads)), "an XML read twice")
         self.assertEqual(len(cap_reads), 1)
 
+    def test_the_state_host_info_and_disk_volume_are_read_once(self):
+        counted = (
+            (libvirt.virDomain, "info"),
+            (libvirt.virConnect, "getInfo"),
+            (libvirt.virConnect, "getDomainCapabilities"),
+            (libvirt.virConnect, "storageVolLookupByPath"),
+            (libvirt.virStorageVol, "info"),
+        )
+        calls = {}
+
+        def counter(cls, name):
+            real = getattr(cls, name)
+
+            def call(*args, **kwargs):
+                key = f"{cls.__name__}.{name}"
+                calls[key] = calls.get(key, 0) + 1
+                return real(*args, **kwargs)
+
+            return call
+
+        patches = [patch.object(cls, name, counter(cls, name)) for cls, name in counted]
+        for p in patches:
+            p.start()
+            self.addCleanup(p.stop)
+        response, _, _ = self.render()
+        self.assertContains(response, "read-cache.img")
+        self.assertEqual(
+            calls,
+            {
+                "virDomain.info": 1,
+                "virConnect.getInfo": 1,
+                "virConnect.getDomainCapabilities": 1,
+                "virConnect.storageVolLookupByPath": 1,
+                "virStorageVol.info": 1,
+            },
+        )
+
     def test_a_change_after_the_page_reads_fresh_xml(self):
         # outside the page the getters read libvirt each time, as changes need
         with patch("vrtManager.connection.connection_manager.get_connection", return_value=self.conn):

instances/test_drbd_target.py

@@ -1,10 +1,13 @@
 """drbd_status runs ssh with the compute's login@hostname; a compute whose
-values would inject an ssh option (an existing or bypassed row) is skipped."""
+values would inject an ssh option (an existing or bypassed row) is skipped,
+and a compute that does not answer in time leaves the status unknown."""
 
+import subprocess
 import unittest
 from types import SimpleNamespace
+from unittest.mock import patch
 
-from instances.views import _valid_ssh_target
+from instances.views import DRBD_STATUS_TIMEOUT, _valid_ssh_target, drbd_status
 
 
 class DrbdTargetTestCase(unittest.TestCase):
@@ -15,3 +18,11 @@ class DrbdTargetTestCase(unittest.TestCase):
         for login, hostname in (("-oProxyCommand=x", "192.0.2.1"), ("root", "-x"), ("root", "a b")):
             with self.subTest(login=login, hostname=hostname):
                 self.assertFalse(_valid_ssh_target(SimpleNamespace(login=login, hostname=hostname)))
+
+    def test_a_compute_that_does_not_answer_leaves_the_status_unknown(self):
+        vm = SimpleNamespace(name="vm1", compute=SimpleNamespace(type=2, login="root", hostname="192.0.2.1"))
+        with patch("instances.views.get_instance", return_value=vm), patch(
+            "instances.views.subprocess.run", side_effect=subprocess.TimeoutExpired("ssh", DRBD_STATUS_TIMEOUT)
+        ) as run:
+            self.assertEqual(drbd_status(SimpleNamespace(user=None), 1), "None DRBD")
+        self.assertEqual(run.call_args.kwargs["timeout"], DRBD_STATUS_TIMEOUT)

instances/views.py

@@ -52,6 +52,9 @@ from . import utils
 from .forms import ConsoleForm, FlavorForm, NewVMForm
 from .models import Flavor
 
+# the VM page waits this long (seconds) for the compute's drbdadm status
+DRBD_STATUS_TIMEOUT = 10
+
 
 def index(request):
     instances = None
@@ -224,11 +227,15 @@ def drbd_status(request, pk):
 
     if instance.compute.type == 2 and _valid_ssh_target(instance.compute):
         conn = instance.compute.login + "@" + instance.compute.hostname
-        remoteDrbdStatus = subprocess.run(
-            ["ssh", "--", conn, "sudo", "/usr/sbin/drbdadm", "status"],
-            stdout=subprocess.PIPE,
-            text=True,
-        )
+        try:
+            remoteDrbdStatus = subprocess.run(
+                ["ssh", "--", conn, "sudo", "/usr/sbin/drbdadm", "status"],
+                stdout=subprocess.PIPE,
+                text=True,
+                timeout=DRBD_STATUS_TIMEOUT,
+            )
+        except subprocess.TimeoutExpired:
+            return result
 
         if remoteDrbdStatus.stdout:
             instanceFindDrbd = re.compile(

vrtManager/instance.py

@@ -480,9 +480,10 @@ class wvmInstance(wvmConnect):
     @contextlib.contextmanager
     def cached_reads(self):
         """
-        Within the block the domain XML (per flags value) and the host
-        capabilities are read from libvirt once, for pages that only read: a
-        VM page reads the XML in some 30 getters. Code that changes the VM
+        Within the block the domain XML (per flags value), the VM's state,
+        the host's capabilities and info, and each disk's volume are read
+        from libvirt once, for pages that only read: a VM page reads the XML
+        in some 30 getters and each disk in two lists. Code that changes the VM
         must not run inside: most changes (devices, QoS, memory) do not go
         through _defineXML, the only call that drops the cache.
         """
@@ -494,27 +495,31 @@ class wvmInstance(wvmConnect):
         finally:
             self._read_cache = None
 
-    def _XMLDesc(self, flag):
+    def _cached(self, key, read):
         cache = self._read_cache
         if cache is None:
-            return self.instance.XMLDesc(flag)
-        key = ("xml", flag)
+            return read()
         if key not in cache:
-            cache[key] = self.instance.XMLDesc(flag)
+            cache[key] = read()
         return cache[key]
 
+    def _XMLDesc(self, flag):
+        return self._cached(("xml", flag), lambda: self.instance.XMLDesc(flag))
+
     def _defineXML(self, xml):
         if self._read_cache is not None:
             self._read_cache.clear()
         return self.wvm.defineXML(xml)
 
     def get_cap_xml(self):
-        cache = self._read_cache
-        if cache is None:
-            return super().get_cap_xml()
-        if "capabilities" not in cache:
-            cache["capabilities"] = super().get_cap_xml()
-        return cache["capabilities"]
+        return self._cached("capabilities", super().get_cap_xml)
+
+    def get_dom_cap_xml(self, arch, machine):
+        read = super().get_dom_cap_xml
+        return self._cached(("domain capabilities", arch, machine), lambda: read(arch, machine))
+
+    def _host_info(self):
+        return self._cached("host info", self.wvm.getInfo)
 
     def get_status(self):
         """
@@ -523,7 +528,7 @@ class wvmInstance(wvmConnect):
         VIR_DOMAIN_PAUSED = 3
         VIR_DOMAIN_SHUTOFF = 5
         """
-        return self.instance.info()[0]
+        return self._cached("status", lambda: self.instance.info()[0])
 
     def get_autostart(self):
         return self.instance.autostart()
@@ -608,11 +613,11 @@ class wvmInstance(wvmConnect):
         return description or ""
 
     def get_max_memory(self):
-        return self.wvm.getInfo()[1] * 1048576
+        return self._host_info()[1] * 1048576
 
     def get_max_cpus(self):
         """Get number of physical CPUs."""
-        hostinfo = self.wvm.getInfo()
+        hostinfo = self._host_info()
         pcpus = hostinfo[4] * hostinfo[5] * hostinfo[6] * hostinfo[7]
         return range(1, int(pcpus + 1))
 
@@ -775,15 +780,10 @@ class wvmInstance(wvmConnect):
                             else None
                         )
 
-                        try:
-                            vol = self.get_volume_by_path(src_file)
-                            volume = vol.name()
-
-                            disk_size = vol.info()[1]
-                            used_size = vol.info()[2]
-                            stg = vol.storagePoolLookupByVolume()
-                            storage = stg.name()
-                        except libvirtError:
+                        details = self._volume_details(src_file)
+                        if details:
+                            volume, disk_size, used_size, storage = details
+                        else:
                             volume = src_file
                     except Exception as e:
                         print(f"Exception: {e}")
@@ -813,6 +813,20 @@ class wvmInstance(wvmConnect):
         flags = VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE if config else 0
         return util.get_xml_path(self._XMLDesc(flags), func=disks)
 
+    def _volume_details(self, path):
+        """(name, capacity, allocation, pool name) of the volume at path, or
+        None when no pool holds it."""
+
+        def read():
+            try:
+                vol = self.get_volume_by_path(path)
+                info = vol.info()
+                return vol.name(), info[1], info[2], vol.storagePoolLookupByVolume().name()
+            except libvirtError:
+                return None
+
+        return self._cached(("volume", path), read)
+
     def get_media_devices(self):
         def disks(doc):
             result = []
@@ -1541,16 +1555,15 @@ class wvmInstance(wvmConnect):
 
     def get_iso_media(self):
         iso = []
-        storages = self.get_storages(only_actives=True)
-        for storage in storages:
-            stg = self.get_storage(storage)
-            if stg.info()[0] != 0:
-                with contextlib.suppress(Exception):
-                    stg.refresh(0)
-
-                for img in stg.listVolumes():
-                    if img.lower().endswith(".iso"):
-                        iso.append(img)
+        for pool in self.wvm.listAllStoragePools(VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE):
+            # refreshed so that an image copied to the pool by hand shows up
+            with contextlib.suppress(libvirtError):
+                pool.refresh(0)
+            try:
+                names = pool.listVolumes()
+            except libvirtError:  # stopped or removed meanwhile
+                continue
+            iso.extend(name for name in names if name.lower().endswith(".iso"))
         return iso
 
     def paths_used_by_other_domains(self):

vrtManager/test_iso_media.py

@@ -6,7 +6,12 @@ from django.conf import settings
 if not settings.configured:
     settings.configure(MAC_OUI="52:54:10")
 
-from libvirt import VIR_DOMAIN_AFFECT_CONFIG, VIR_DOMAIN_AFFECT_LIVE, libvirtError
+from libvirt import (
+    VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE,
+    VIR_DOMAIN_AFFECT_CONFIG,
+    VIR_DOMAIN_AFFECT_LIVE,
+    libvirtError,
+)
 from lxml import etree
 from vrtManager.instance import wvmInstance
 
@@ -52,5 +57,43 @@ class IsoMediaTestCase(unittest.TestCase):
         inst.instance.updateDeviceFlags.assert_not_called()
 
 
+def pool(volumes, refresh_error=False, list_error=False):
+    p = MagicMock()
+    if refresh_error:
+        p.refresh.side_effect = libvirtError("refresh failed")
+    if list_error:
+        p.listVolumes.side_effect = libvirtError("pool is not active")
+    else:
+        p.listVolumes.return_value = volumes
+    return p
+
+
+class IsoListTestCase(unittest.TestCase):
+    """The CD-ROM choices are the .iso volumes of the active pools, refreshed
+    so that an image copied in by hand shows up."""
+
+    def iso(self, *pools):
+        inst = wvmInstance.__new__(wvmInstance)
+        inst.wvm = MagicMock()
+        inst.wvm.listAllStoragePools.return_value = list(pools)
+        return inst, inst.get_iso_media()
+
+    def test_iso_volumes_of_the_active_pools(self):
+        a, b = pool(["a.iso", "disk.qcow2"]), pool(["B.ISO"])
+        inst, iso = self.iso(a, b)
+        self.assertEqual(iso, ["a.iso", "B.ISO"])
+        inst.wvm.listAllStoragePools.assert_called_once_with(VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE)
+        a.refresh.assert_called_once_with(0)
+        b.refresh.assert_called_once_with(0)
+
+    def test_a_failed_refresh_still_lists_the_pool(self):
+        _, iso = self.iso(pool(["a.iso"], refresh_error=True))
+        self.assertEqual(iso, ["a.iso"])
+
+    def test_a_pool_stopped_meanwhile_is_skipped(self):
+        _, iso = self.iso(pool([], list_error=True), pool(["b.iso"]))
+        self.assertEqual(iso, ["b.iso"])
+
+
 if __name__ == "__main__":
     unittest.main()

vrtManager/test_read_cache.py

@@ -1,14 +1,18 @@
-"""wvmInstance.cached_reads(): inside the block the domain XML (per flags) and
-the host capabilities are read from libvirt once; outside it every read goes
-to libvirt, as code that changes the VM needs."""
+"""wvmInstance.cached_reads(): inside the block the domain XML (per flags),
+the VM's state, the host capabilities and info, and each disk's volume are
+read from libvirt once; outside it every read goes to libvirt, as code that
+changes the VM needs."""
 
 import unittest
+from unittest.mock import MagicMock, patch
 
 from django.conf import settings
 
 if not settings.configured:
     settings.configure(MAC_OUI="52:54:10")
 
+from libvirt import libvirtError
+from vrtManager.connection import wvmConnect
 from vrtManager.instance import wvmInstance
 
 
@@ -17,6 +21,10 @@ class FakeDomain:
         self.reads = []
         self.fail = False
 
+    def info(self):
+        self.reads.append("info")
+        return [1, 0, 0, 0, 0]
+
     def XMLDesc(self, flags):
         if self.fail:
             raise RuntimeError("libvirt error")
@@ -29,6 +37,10 @@ class FakeConn:
         self.caps = 0
         self.defined = []
 
+    def getInfo(self):
+        self.caps += 1
+        return ["x86_64", 2048, 4, 0, 1, 2, 2, 1]
+
     def getCapabilities(self):
         self.caps += 1
         return "<capabilities/>"
@@ -111,3 +123,100 @@ class CachedReadsTestCase(unittest.TestCase):
             b._XMLDesc(0)
             b._XMLDesc(0)
         self.assertEqual((a.instance.reads, b.instance.reads), ([0], [0, 0]))
+
+    def test_the_state_and_host_info_are_read_once(self):
+        p = proxy()
+        with p.cached_reads():
+            for _ in range(2):
+                p.get_status()
+                p.get_max_memory()
+                p.get_max_cpus()
+        self.assertEqual((p.instance.reads, p.wvm.caps), (["info"], 1))
+        p.get_status()
+        self.assertEqual(p.instance.reads, ["info", "info"])
+
+    def test_domain_capabilities_are_read_once_per_arch_and_machine(self):
+        p = proxy()
+        with patch.object(wvmConnect, "get_dom_cap_xml", return_value="<domainCapabilities/>") as read:
+            with p.cached_reads():
+                for _ in range(2):
+                    p.get_dom_cap_xml("x86_64", "q35")
+                    p.get_dom_cap_xml("x86_64", "pc")
+            p.get_dom_cap_xml("x86_64", "q35")
+        self.assertEqual([c.args for c in read.call_args_list], [("x86_64", "q35"), ("x86_64", "pc"), ("x86_64", "q35")])
+
+
+def volume_proxy(paths):
+    """A proxy whose host has volumes at paths (pool "pool", size 10, used 4)."""
+    p = proxy()
+    p.lookups = []
+
+    def lookup(path):
+        p.lookups.append(path)
+        if path not in paths:
+            raise libvirtError("no storage vol with matching path")
+        vol = MagicMock()
+        vol.name.return_value = path.rsplit("/", 1)[-1]
+        vol.info.return_value = [0, 10, 4]
+        vol.storagePoolLookupByVolume.return_value.name.return_value = "pool"
+        return vol
+
+    p.get_volume_by_path = lookup
+    return p
+
+
+class VolumeDetailsTestCase(unittest.TestCase):
+    def test_inside_the_block_each_path_is_looked_up_once(self):
+        p = volume_proxy({"/p/a.qcow2", "/p/b.raw"})
+        with p.cached_reads():
+            for _ in range(2):
+                self.assertEqual(p._volume_details("/p/a.qcow2"), ("a.qcow2", 10, 4, "pool"))
+                p._volume_details("/p/b.raw")
+                self.assertIsNone(p._volume_details("/dev/sdx"))
+        self.assertEqual(p.lookups, ["/p/a.qcow2", "/p/b.raw", "/dev/sdx"])
+
+    def test_outside_the_block_and_after_a_change_the_volume_is_read_again(self):
+        p = volume_proxy({"/p/a.qcow2"})
+        p._volume_details("/p/a.qcow2")
+        with p.cached_reads():
+            p._volume_details("/p/a.qcow2")
+            p._defineXML("<domain/>")
+            p._volume_details("/p/a.qcow2")
+        p._volume_details("/p/a.qcow2")
+        self.assertEqual(len(p.lookups), 4)
+
+
+def disk(path, cache):
+    return (
+        f"<disk type='file' device='disk'><driver name='qemu' type='qcow2' cache='{cache}'/>"
+        f"<source file='{path}'/><target dev='vda' bus='virtio'/></disk>"
+    )
+
+
+class DiskListsTestCase(unittest.TestCase):
+    """The page lists the running and the persistent disks; both share a
+    path's volume, each keeps its own settings."""
+
+    def lists(self, live, config, volumes):
+        p = volume_proxy(volumes)
+        xml = {0: f"<domain><devices>{live}</devices></domain>", 3: f"<domain><devices>{config}</devices></domain>"}
+        p.instance.XMLDesc = lambda flags: xml[flags]
+        with p.cached_reads():
+            return p, p.get_disk_devices(), p.get_disk_devices(config=True)
+
+    def test_one_path_in_both_lists_is_looked_up_once(self):
+        p, live, config = self.lists(disk("/p/a.qcow2", "none"), disk("/p/a.qcow2", "writeback"), {"/p/a.qcow2"})
+        self.assertEqual(p.lookups, ["/p/a.qcow2"])
+        self.assertEqual([(d["image"], d["storage"], d["size"], d["cache"]) for d in live], [("a.qcow2", "pool", 10, "none")])
+        self.assertEqual([d["cache"] for d in config], ["writeback"])
+        self.assertIsNot(live[0], config[0])
+
+    def test_a_pending_disk_change_is_looked_up_on_its_own(self):
+        p, live, config = self.lists(disk("/p/a.qcow2", "none"), disk("/p/b.qcow2", "none"), {"/p/a.qcow2", "/p/b.qcow2"})
+        self.assertEqual(p.lookups, ["/p/a.qcow2", "/p/b.qcow2"])
+        self.assertEqual((live[0]["image"], config[0]["image"]), ("a.qcow2", "b.qcow2"))
+
+    def test_a_disk_outside_any_pool_shows_its_path(self):
+        p, live, _ = self.lists(disk("/srv/x.img", "none"), disk("/srv/x.img", "none"), set())
+        self.assertEqual(p.lookups, ["/srv/x.img"])
+        self.assertEqual((live[0]["image"], live[0]["storage"], live[0]["size"]), ("/srv/x.img", None, None))