perf(instances): the VM page reads each kind of domain XML once

The VM page's getters read the domain XML on their own: one load made
86 libvirt calls for a real VM, 29 of them for the same three XML
documents (live, live with secrets, persistent) and 6 for the host
capabilities. Each call is a round trip to the host.

wvmInstance.cached_reads() is a block in which _XMLDesc (per flags
value) and get_cap_xml are read from libvirt once; it ends with the
block, even on an error, does not nest, and _defineXML drops it.
Outside a block every read goes to libvirt as before. Only the VM page
view, which does not change the VM, uses it, around its rendering.
Views that change the VM keep reading fresh XML: most changes (devices,
QoS, memory) do not go through _defineXML, and several views read,
change and define more than once in one request.

Measured on the test host: 86 -> 55 calls (XMLDesc 29 -> 3,
getCapabilities 6 -> 1) for one VM, 82 -> 54 for another; the page took
0.23-0.25 s before, 0.14-0.15 s after. The HTML of both pages is
identical before and after (CSRF tokens aside). A test renders the page
on libvirt's test driver through the real getters: 25 XML reads before,
one per kind now.

instances/test_detail_reads.py

@@ -0,0 +1,66 @@
+"""The VM page reads each kind of domain XML and the host capabilities once,
+through the real getters, on libvirt's test driver."""
+
+from unittest.mock import patch
+
+import libvirt
+from computes.models import Compute
+from django.contrib.auth import get_user_model
+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>
+<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>
+</domain>"""
+
+
+class DetailReadsTests(TestCase):
+    def setUp(self):
+        self.conn = libvirt.open("test:///default")  # each open is a fresh, private test host
+        self.addCleanup(self.conn.close)
+        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())
+        # what the test driver lacks: machine types, network filters
+        for target, value in (("get_machine_types", ["pc"]), ("get_nwfilters", [])):
+            patcher = patch(f"vrtManager.connection.wvmConnect.{target}", return_value=value)
+            patcher.start()
+            self.addCleanup(patcher.stop)
+        self.admin = get_user_model().objects.create_superuser("dr-admin", "dr@example.com", "pw")
+
+    def render(self):
+        xml_reads, cap_reads = [], []
+        real_xml, real_caps = libvirt.virDomain.XMLDesc, libvirt.virConnect.getCapabilities
+
+        def xml(dom, flags=0):
+            xml_reads.append(flags)
+            return real_xml(dom, flags)
+
+        def caps(conn):
+            cap_reads.append(1)
+            return real_caps(conn)
+
+        self.client.force_login(self.admin)
+        with patch("vrtManager.connection.connection_manager.get_connection", return_value=self.conn), patch.object(
+            libvirt.virDomain, "XMLDesc", xml
+        ), patch.object(libvirt.virConnect, "getCapabilities", caps):
+            response = self.client.get(reverse("instances:instance", args=[self.vm.id]))
+        self.assertEqual(response.status_code, 200)
+        return response, xml_reads, cap_reads
+
+    def test_each_xml_and_the_capabilities_are_read_once(self):
+        _, xml_reads, cap_reads = self.render()
+        self.assertEqual(sorted(xml_reads), sorted(set(xml_reads)), "an XML read twice")
+        self.assertEqual(len(cap_reads), 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):
+            proxy = self.vm.proxy
+            proxy.set_console_passwd("pass1")
+            proxy.set_console_keymap("de")
+            self.assertEqual(proxy.get_console_passwd(), "pass1")
+            self.assertEqual(proxy.get_console_keymap(), "de")

instances/views.py

@@ -95,6 +95,13 @@ def instance(request, pk):
             msg = _("The host of '%(name)s' cannot be reached right now.") % {"name": instance.name}
         messages.error(request, msg)
         return redirect(reverse("instances:index"))
+    # The page only reads the VM, and its getters read the same XML some 30
+    # times: libvirt is asked once per kind of XML.
+    with instance.proxy.cached_reads():
+        return _instance_page(request, pk, instance, compute)
+
+
+def _instance_page(request, pk, instance, compute):
     computes = Compute.objects.all().order_by("name")
     computes_count = computes.count()
     users = User.objects.all().order_by("username")

vrtManager/instance.py

@@ -186,6 +186,9 @@ class wvmInstances(wvmConnect):
 
 
 class wvmInstance(wvmConnect):
+    # set by cached_reads(): libvirt answers reused within the block
+    _read_cache = None
+
     def __init__(self, host, login, passwd, conn, vname, uuid=None):
         wvmConnect.__init__(self, host, login, passwd, conn)
         self._ip_cache = None
@@ -249,12 +252,45 @@ class wvmInstance(wvmConnect):
     def delete(self, flags=0):
         self.instance.undefineFlags(flags)
 
+    @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
+        must not run inside: most changes (devices, QoS, memory) do not go
+        through _defineXML, the only call that drops the cache.
+        """
+        if self._read_cache is not None:
+            raise RuntimeError("cached_reads blocks do not nest")
+        self._read_cache = {}
+        try:
+            yield self
+        finally:
+            self._read_cache = None
+
     def _XMLDesc(self, flag):
-        return self.instance.XMLDesc(flag)
+        cache = self._read_cache
+        if cache is None:
+            return self.instance.XMLDesc(flag)
+        key = ("xml", flag)
+        if key not in cache:
+            cache[key] = self.instance.XMLDesc(flag)
+        return cache[key]
 
     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"]
+
     def get_status(self):
         """
         VIR_DOMAIN_NOSTATE = 0

vrtManager/test_read_cache.py

@@ -0,0 +1,113 @@
+"""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."""
+
+import unittest
+
+from django.conf import settings
+
+if not settings.configured:
+    settings.configure(MAC_OUI="52:54:10")
+
+from vrtManager.instance import wvmInstance
+
+
+class FakeDomain:
+    def __init__(self):
+        self.reads = []
+        self.fail = False
+
+    def XMLDesc(self, flags):
+        if self.fail:
+            raise RuntimeError("libvirt error")
+        self.reads.append(flags)
+        return f"<domain flags='{flags}' read='{len(self.reads)}'/>"
+
+
+class FakeConn:
+    def __init__(self):
+        self.caps = 0
+        self.defined = []
+
+    def getCapabilities(self):
+        self.caps += 1
+        return "<capabilities/>"
+
+    def defineXML(self, xml):
+        self.defined.append(xml)
+
+
+def proxy():
+    p = wvmInstance.__new__(wvmInstance)  # no connection: only these calls
+    p.instance = FakeDomain()
+    p.wvm = FakeConn()
+    return p
+
+
+class CachedReadsTestCase(unittest.TestCase):
+    def test_outside_the_block_every_read_goes_to_libvirt(self):
+        p = proxy()
+        p._XMLDesc(0)
+        p._XMLDesc(0)
+        p.get_cap_xml()
+        p.get_cap_xml()
+        self.assertEqual((p.instance.reads, p.wvm.caps), ([0, 0], 2))
+
+    def test_inside_the_block_each_flags_value_is_read_once(self):
+        p = proxy()
+        with p.cached_reads():
+            first = [p._XMLDesc(0), p._XMLDesc(1), p._XMLDesc(3)]
+            again = [p._XMLDesc(0), p._XMLDesc(1), p._XMLDesc(3)]
+            p.get_cap_xml()
+            p.get_cap_xml()
+        self.assertEqual(first, again)
+        self.assertEqual(p.instance.reads, [0, 1, 3])
+        self.assertEqual(p.wvm.caps, 1)
+
+    def test_the_block_ends_with_its_cache(self):
+        p = proxy()
+        with p.cached_reads():
+            p._XMLDesc(0)
+        with p.cached_reads():
+            p._XMLDesc(0)
+        p._XMLDesc(0)
+        self.assertEqual(p.instance.reads, [0, 0, 0])
+
+    def test_an_error_inside_the_block_still_ends_it(self):
+        p = proxy()
+        with self.assertRaises(ValueError), p.cached_reads():
+            p._XMLDesc(0)
+            raise ValueError
+        p._XMLDesc(0)
+        self.assertEqual(p.instance.reads, [0, 0])
+
+    def test_a_failed_read_is_not_cached(self):
+        p = proxy()
+        with p.cached_reads():
+            p.instance.fail = True
+            with self.assertRaises(RuntimeError):
+                p._XMLDesc(0)
+            p.instance.fail = False
+            p._XMLDesc(0)
+        self.assertEqual(p.instance.reads, [0])
+
+    def test_a_definition_change_drops_the_cache(self):
+        p = proxy()
+        with p.cached_reads():
+            p._XMLDesc(3)
+            p._defineXML("<domain/>")
+            p._XMLDesc(3)
+        self.assertEqual(p.instance.reads, [3, 3])
+
+    def test_blocks_do_not_nest(self):
+        p = proxy()
+        with p.cached_reads(), self.assertRaises(RuntimeError), p.cached_reads():
+            pass
+
+    def test_two_proxies_do_not_share_a_cache(self):
+        a, b = proxy(), proxy()
+        with a.cached_reads():
+            a._XMLDesc(0)
+            b._XMLDesc(0)
+            b._XMLDesc(0)
+        self.assertEqual((a.instance.reads, b.instance.reads), ([0], [0, 0]))