fix(instances): the VM page shows each device's own settings

A NIC without QoS showed the previous NIC's limits, an empty CD-ROM the
previous drive's pool; a NIC without a source or model broke the page.
The boot order shows the target of block and network disks and of bridge
and direct NICs too. Refreshing the VM's pools before a snapshot or a
delete no longer fails on a disk outside every pool.

README.md

@@ -537,6 +537,7 @@ A rollback leaves the clone on a detached HEAD at the restored commit; the updat
 - A host's CPU graph shows the average since its previous point (normally 5 s) instead of a 1 s sample taken while a worker waits.
 - A VM's stats are polled every 5 s like the host graph, the first point as soon as the tab opens; each point is the average since the previous one, disk rates in MB/s (they were MiB/s labelled Mb/s); a counter the host does not report shows a gap instead of 0.
 - An ISO uploads to an SSH compute about 3.5 times faster (SFTP writes no longer wait for each 32 KiB); a failed write still fails the upload.
+- The VM page shows each NIC's own QoS and each CD-ROM's own pool (they showed the previous device's), and a NIC without a source or model no longer breaks it.
 - A running VM without a guest agent shows the IPv4 address the host learned by ARP (the lookup always failed before).
 - After a live migration the VM's NVRAM file is deleted on the source, when it is in libvirt's `/var/lib/libvirt/qemu/nvram` and the destination does not share that directory (a VM created there later with the same name would start with those UEFI variables).
 - gstfsd finds the guest's operating system the way libguestfs inspects it, so a root on LVM works, and it always answers (an error when it finds no or several operating systems). Update `/usr/local/bin/gstfsd` on every compute (see [Adding a Compute Node](#adding-a-compute-node), step 3).

vrtManager/instance.py

@@ -683,16 +683,17 @@ class wvmInstance(wvmConnect):
     def get_net_devices(self, config=False):
         def networks(ctx):
             result = []
-            inbound = outbound = []
             for net in ctx.xpath("/domain/devices/interface"):
+                inbound = outbound = []
                 interface_type = net.xpath("@type")[0]
                 mac_inst = net.xpath("mac/@address")[0]
-                nic_inst = net.xpath("source/@network|source/@bridge|source/@dev")[0]
+                source = net.xpath("source/@network|source/@bridge|source/@dev")
+                nic_inst = source[0] if source else ""
                 target_inst = net.xpath("target/@dev")[0] if net.xpath("target/@dev") else ""
                 link_state = net.xpath("link/@state")[0] if net.xpath("link") else "up"
                 filterref_inst = net.xpath("filterref/@filter")[0] if net.xpath("filterref/@filter") else ""
 
-                model_type = net.xpath("model/@type")[0]
+                model_type = net.xpath("model/@type")[0] if net.xpath("model/@type") else ""
                 if net.xpath("bandwidth/inbound"):
                     in_attr = net.xpath("bandwidth/inbound")[0]
                     in_av = in_attr.get("average")
@@ -831,34 +832,22 @@ class wvmInstance(wvmConnect):
     def get_media_devices(self):
         def disks(doc):
             result = []
-            dev = volume = storage = bus = None
-            src_file = None
-            for media in doc.xpath("/domain/devices/disk"):
-                device = media.xpath("@device")[0]
-                if device == "cdrom":
-                    try:
-                        dev = media.xpath("target/@dev")[0]
-                        bus = media.xpath("target/@bus")[0]
-                        src_file = None
-                        volume = src_file
-                        with contextlib.suppress(Exception):
-                            src_file = media.xpath("source/@file")[0]
-                            vol = self.get_volume_by_path(src_file)
-                            volume = vol.name()
-                            stg = vol.storagePoolLookupByVolume()
-                            storage = stg.name()
-                    except Exception:
-                        pass
-                    finally:
-                        result.append(
-                            {
-                                "dev": dev,
-                                "image": volume,
-                                "storage": storage,
-                                "path": src_file,
-                                "bus": bus,
-                            }
-                        )
+            for media in doc.xpath("/domain/devices/disk[@device='cdrom']"):
+                dev = media.xpath("target/@dev")
+                bus = media.xpath("target/@bus")
+                src_file = media.xpath("source/@file")
+                src_file = src_file[0] if src_file else None
+                details = self._volume_details(src_file) if src_file else None
+                volume, _, _, storage = details or (None, None, None, None)
+                result.append(
+                    {
+                        "dev": dev[0] if dev else None,
+                        "image": volume,
+                        "storage": storage,
+                        "path": src_file,
+                        "bus": bus[0] if bus else None,
+                    }
+                )
             return result
 
         return util.get_xml_path(self._XMLDesc(0), func=disks)
@@ -923,10 +912,10 @@ class wvmInstance(wvmConnect):
                 dev_type = dev.get("type")
                 dev_device = dev.get("device")
 
-                if dev_type == "file" or (dev_device == "disk" and dev_type == "network"):
+                if dev.tag == "disk":  # file, block, volume or network
                     dev_target = dev.find("target").get("dev")
 
-                elif dev_type == "network":
+                elif dev.tag == "interface":  # network, bridge or direct
                     dev_mac = dev.find("mac").get("address")
                     dev_device = "network"
                     dev_target = f"nic-{dev_mac[9:]}"
@@ -1798,13 +1787,14 @@ class wvmInstance(wvmConnect):
         return wvmStorages(self.host, self.login, self.passwd, self.conn)
 
     def refresh_instance_pools(self):
-        disks = self.get_disk_devices()
-        target_paths = set()
-        for disk in disks:
-            disk_path = disk.get("path")
-            target_paths.add(os.path.dirname(disk_path))
+        """Refresh the pools whose target directory holds one of the VM's
+        disks; a disk outside every pool has none to refresh."""
+        target_paths = {os.path.dirname(disk["path"]) for disk in self.get_disk_devices() if disk.get("path")}
+        storages = self.get_wvmStorages()
         for target_path in target_paths:
-            self.get_wvmStorages().get_pool_by_target(target_path).refresh(0)
+            pool = storages.get_pool_by_target(target_path)
+            if pool is not None:
+                pool.refresh(0)
 
     def fix_mac(self, mac):
         if ":" in mac:

vrtManager/test_instance_getters.py

@@ -0,0 +1,82 @@
+"""The VM page's device getters read each device on its own: a value of one
+NIC, CD-ROM or boot device does not carry over to the next."""
+
+import unittest
+from unittest.mock import MagicMock
+
+from django.conf import settings
+
+if not settings.configured:
+    settings.configure(MAC_OUI="52:54:10")
+
+from libvirt import libvirtError
+from vrtManager.instance import wvmInstance
+
+XML = """<domain>
+  <os><type arch='x86_64'>hvm</type></os>
+  <devices>
+    <disk type='file' device='disk'><source file='/pool/a.qcow2'/><target dev='vda' bus='virtio'/><boot order='3'/></disk>
+    <disk type='block' device='disk'><source dev='/dev/vg/b'/><target dev='vdb' bus='virtio'/><boot order='4'/></disk>
+    <disk type='file' device='cdrom'><source file='/iso/x.iso'/><target dev='sda' bus='sata'/></disk>
+    <disk type='file' device='cdrom'><target dev='sdb' bus='sata'/></disk>
+    <interface type='network'>
+      <mac address='52:54:10:00:00:01'/><source network='default'/><model type='virtio'/>
+      <bandwidth><inbound average='100' peak='200' burst='300'/><outbound average='10' peak='20' burst='30'/></bandwidth>
+    </interface>
+    <interface type='bridge'><mac address='52:54:10:00:00:02'/><source bridge='br0'/><boot order='1'/></interface>
+    <interface type='direct'><mac address='52:54:10:00:00:03'/><boot order='2'/></interface>
+  </devices>
+</domain>"""
+
+
+def proxy():
+    p = wvmInstance.__new__(wvmInstance)  # no connection: only these calls
+    p._ip_cache = {"qemuga": {}, "arp": {}}
+    p._XMLDesc = lambda flags: XML
+    volumes = {"/iso/x.iso": ("x.iso", "iso")}
+
+    def volume(path):
+        if path not in volumes:
+            raise libvirtError("no volume")
+        name, pool = volumes[path]
+        return MagicMock(**{"name.return_value": name, "info.return_value": [0, 10, 4],
+                            "storagePoolLookupByVolume.return_value.name.return_value": pool})
+
+    p.get_volume_by_path = volume
+    return p
+
+
+class DeviceGettersTestCase(unittest.TestCase):
+    def test_a_nic_without_qos_source_or_model_has_none(self):
+        nics = proxy().get_net_devices()
+        self.assertEqual(nics[0]["inbound"], {"average": "100", "peak": "200", "burst": "300"})
+        self.assertEqual(nics[0]["outbound"], {"average": "10", "peak": "20", "burst": "30"})
+        self.assertEqual([(n["inbound"], n["outbound"]) for n in nics[1:]], [([], [])] * 2)
+        self.assertEqual([(n["nic"], n["model"]) for n in nics], [("default", "virtio"), ("br0", ""), ("", "")])
+
+    def test_an_empty_cdrom_shows_no_pool(self):
+        media = proxy().get_media_devices()
+        self.assertEqual(
+            media,
+            [
+                {"dev": "sda", "image": "x.iso", "storage": "iso", "path": "/iso/x.iso", "bus": "sata"},
+                {"dev": "sdb", "image": None, "storage": None, "path": None, "bus": "sata"},
+            ],
+        )
+
+    def test_every_boot_device_has_its_target(self):
+        order = proxy().get_bootorder()
+        self.assertEqual(
+            [order[i]["target"] for i in range(4)],
+            ["nic-00:00:02", "nic-00:00:03", "vda", "vdb"],
+        )
+
+
+class RefreshInstancePoolsTestCase(unittest.TestCase):
+    def test_only_pools_holding_a_disk_are_refreshed(self):
+        p = proxy()
+        p.get_disk_devices = lambda: [{"path": "/pool/a.qcow2"}, {"path": "/elsewhere/b.img"}, {"path": None}]
+        pool = MagicMock()
+        p.get_wvmStorages = lambda: MagicMock(get_pool_by_target=lambda target: pool if target == "/pool" else None)
+        p.refresh_instance_pools()
+        pool.refresh.assert_called_once_with(0)