diff --git a/instances/test_detail_reads.py b/instances/test_detail_reads.py index d8fa95b5..3e4f667d 100644 --- a/instances/test_detail_reads.py +++ b/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 = """read-cache-vm1281 +# a VM with a disk and a VNC console, on libvirt's test driver +DOMAIN = """read-cache-vm0b9f6a52-6d0e-4c1f-9a51-2f3c1d7e8a401281 hvm - + + """ 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("read-cache.img1048576", 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): diff --git a/instances/test_drbd_target.py b/instances/test_drbd_target.py index 5c29cd15..dc7f32af 100644 --- a/instances/test_drbd_target.py +++ b/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) diff --git a/instances/views.py b/instances/views.py index d95aa9ab..b8ca3c65 100755 --- a/instances/views.py +++ b/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( diff --git a/vrtManager/instance.py b/vrtManager/instance.py index d45e07b6..a92f1841 100644 --- a/vrtManager/instance.py +++ b/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): diff --git a/vrtManager/test_iso_media.py b/vrtManager/test_iso_media.py index f46d42ec..4677f3b6 100644 --- a/vrtManager/test_iso_media.py +++ b/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() diff --git a/vrtManager/test_read_cache.py b/vrtManager/test_read_cache.py index 5facf5e5..ea5e1aab 100644 --- a/vrtManager/test_read_cache.py +++ b/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 "" @@ -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="") 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("") + p._volume_details("/p/a.qcow2") + p._volume_details("/p/a.qcow2") + self.assertEqual(len(p.lookups), 4) + + +def disk(path, cache): + return ( + f"" + f"" + ) + + +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"{live}", 3: f"{config}"} + 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))