fix(drivers): address PR #172 review comments

- _domain2machine: fall back to standard libvirt <vcpu>/<currentMemory>/
  <memory> elements when a domain lacks Genesis metadata (e.g. a
  pre-existing/foreign VM on a shared hypervisor) instead of crashing
  pool listing for the whole hypervisor. New _memory_mib() handles the
  unit conversion, since a foreign domain's <memory> isn't guaranteed
  to be in MiB like Genesis's own tag.
- Fix the `docement` -> `document` typo in document_set_tag/
  document_meta_set_tag.
- _find_attached_volume_element and friends: use `.findall(".//devices/disk")`
  instead of `.find("devices").findall("disk")`, consistent with the
  rest of the file, so a domain without a <devices> element doesn't
  crash with AttributeError.
- resize_volume: replace a while-loop-as-one-shot-if with an actual
  `if`, and guard against a disk with no <target dev=...>.
- create_machine: raise a clear error instead of AttributeError when
  the storage pool's XML has no <target><path>.
- rename_machine: `except:` -> `except Exception:` (PEP 8).
- Use libvirt.VIR_ERR_NO_SUPPORT instead of the bare int 3.

Verified two other flagged spots are not actually reachable and left
alone: Element.get(None) returns None rather than raising (checked
empirically), and MetaMachine.port_info defaults to `dict` at the
model level, so it's never None to begin with.

Author: VasilyStepanov

gcl_sdk/agents/universal/drivers/libvirt.py

@@ -37,6 +37,23 @@
 CONSOLE_LOG_DIR = "/var/log/libvirt/qemu"
 
 
+def _memory_mib(el: ET.Element) -> int:
+    """Read a libvirt <memory>/<currentMemory> element's value, in MiB.
+
+    libvirt defaults the `unit` attribute to KiB when absent; Genesis's
+    own genesis:mem tag (handled separately) is always a plain MiB int.
+    """
+    value = int(el.text)
+    unit = (el.get("unit") or "KiB").lower()
+    if unit in ("gib", "gb", "g"):
+        return value * 1024
+    if unit in ("mib", "mb", "m"):
+        return value
+    if unit in ("b", "bytes"):
+        return value // (1024 * 1024)
+    return value // 1024  # KiB, libvirt's own default
+
+
 def _interface_source(
     iface: ET.Element, source_el: tp.Optional[ET.Element]
 ) -> tp.Optional[str]:
@@ -261,45 +278,45 @@ def add_element(
     @classmethod
     def document_set_tag(
         cls,
-        docement: minidom.Document,
+        document: minidom.Document,
         tag_name: str,
         text: tp.Optional[str] = None,
         meta_tag: tp.Optional[str] = None,
         parent: tp.Optional[minidom.Element] = None,
         **kwargs,
     ) -> None:
-        root = parent or docement.firstChild
+        root = parent or document.firstChild
         # Firstly we need to remove the old value
         for node in root.getElementsByTagName(tag_name):
             root.removeChild(node)
 
         # Also we need to remove the old value from the meta
         if meta_tag is not None:
-            meta_node = docement.getElementsByTagName(META_TAG)[0]
-            for node in docement.getElementsByTagName(meta_tag):
+            meta_node = document.getElementsByTagName(META_TAG)[0]
+            for node in document.getElementsByTagName(meta_tag):
                 meta_node.removeChild(node)
 
             # Add the new value
-            cls.add_element(docement, meta_tag, parent=meta_node, text=text)
+            cls.add_element(document, meta_tag, parent=meta_node, text=text)
 
         # Set the new value
-        cls.add_element(docement, tag_name, parent=root, text=text, **kwargs)
+        cls.add_element(document, tag_name, parent=root, text=text, **kwargs)
 
     @classmethod
     def document_meta_set_tag(
         cls,
-        docement: minidom.Document,
+        document: minidom.Document,
         tag: str,
         text: tp.Optional[str] = None,
         **kwargs,
     ) -> None:
         # Remove the old value from the meta
-        meta_node = docement.getElementsByTagName(META_TAG)[0]
-        for node in docement.getElementsByTagName(tag):
+        meta_node = document.getElementsByTagName(META_TAG)[0]
+        for node in document.getElementsByTagName(tag):
             meta_node.removeChild(node)
 
         # Add the new value
-        cls.add_element(docement, tag, parent=meta_node, text=text, **kwargs)
+        cls.add_element(document, tag, parent=meta_node, text=text, **kwargs)
 
 
 class XMLLibvirtVolume(XMLLibvirtMixin):
@@ -603,10 +620,24 @@ def _domain2machine(
     ) -> tp.Tuple[pool_base.Machine, tp.Tuple[pool_base.Port, ...]]:
         element = element or ET.fromstring(domain.XMLDesc())
 
+        # listAllDomains() enumerates every domain libvirt knows about, not
+        # just ones this driver created - a domain without Genesis metadata
+        # (pre-existing/foreign, or from before these tags existed) falls
+        # back to the standard libvirt elements every domain has, rather
+        # than crashing pool listing for the whole hypervisor.
         cores_xml = element.find(f".//{{{GENESIS_NS}}}vcpu")
+        if cores_xml is None:
+            cores_xml = element.find(".//vcpu")
         cores = int(cores_xml.text)
+
         ram_xml = element.find(f".//{{{GENESIS_NS}}}mem")
-        ram = int(ram_xml.text)
+        if ram_xml is not None:
+            ram = int(ram_xml.text)
+        else:
+            mem_xml = element.find(".//currentMemory")
+            if mem_xml is None:
+                mem_xml = element.find(".//memory")
+            ram = _memory_mib(mem_xml)
         image_el = element.find(f".//{{{GENESIS_NS}}}image")
         image = image_el.get("uri") if image_el is not None else None
 
@@ -820,7 +851,7 @@ def _find_attached_volume_element(
         self, domain: ET.Element, volume: pool_base.MachineVolume
     ) -> tp.Optional[ET.Element]:
         # Check the volume is attached to the domain
-        for disk in domain.find("devices").findall("disk"):
+        for disk in domain.findall(".//devices/disk"):
             # Check source and path
             source_node = disk.find("source")
             if source_node is None:
@@ -1002,7 +1033,7 @@ def delete_volume(self, volume: pool_base.MachineVolume) -> None:
             v.wipe()
         except libvirt.libvirtError as e:
             # Some backends don't need wiping, for ex. ZFS
-            if e.get_error_code() != 3:  # VIR_ERR_NO_SUPPORT
+            if e.get_error_code() != libvirt.VIR_ERR_NO_SUPPORT:
                 raise
         max_iters = 20
         for i in range(max_iters + 1):
@@ -1039,7 +1070,7 @@ def attach_volume(self, volume: pool_base.MachineVolume) -> None:
         # FIXME(akremenetsky): Use the simplest check by UUID
         # FIXME(akremenetsky): Check volume name in path. We can check by
         # name as the name is actual UUID of the volume.
-        disks = domain_element.find("devices").findall("disk")
+        disks = domain_element.findall(".//devices/disk")
         for disk in disks:
             if bytes(volume.name, "utf-8") in ET.tostring(disk):
                 raise pool_base.VolumeAlreadyAttachedError(
@@ -1061,7 +1092,7 @@ def attach_volume(self, volume: pool_base.MachineVolume) -> None:
         image_path = vir_volume.path()
 
         # Detect next device name from domain XML
-        devices = len(domain_element.find("devices").findall("disk"))
+        devices = len(domain_element.findall(".//devices/disk"))
         device_name = "vd" + chr(ord("a") + devices)
 
         disk_xml = XMLLibvirtInstance.disk_device_xml(image_path, device_name)
@@ -1143,34 +1174,34 @@ def resize_volume(self, volume: pool_base.MachineVolume) -> None:
 
         # If the volume is attached and a domain is active use
         # `blockResize`
-        while volume.machine is not None:
+        if volume.machine is not None:
             domain = self._client.lookupByUUIDString(str(volume.machine))
 
-            # Not active, break
-            if domain.isActive() == 0:
-                break
-
-            domain_xml = domain.XMLDesc()
-            domain_element = ET.fromstring(domain_xml)
-            disk = self._find_attached_volume_element(domain_element, volume)
-            if disk is None:
-                raise pool_base.VolumeNotAttachedError(
-                    volume=volume.uuid, machine=volume.machine
+            if domain.isActive() != 0:
+                domain_xml = domain.XMLDesc()
+                domain_element = ET.fromstring(domain_xml)
+                disk = self._find_attached_volume_element(domain_element, volume)
+                if disk is None:
+                    raise pool_base.VolumeNotAttachedError(
+                        volume=volume.uuid, machine=volume.machine
+                    )
+                target = disk.find("target")
+                dev = target.get("dev") if target is not None else None
+                if not dev:
+                    raise ValueError(f"Disk {disk} has no target dev")
+
+                pool_xml = ET.fromstring(storage_pool.XMLDesc())
+                pool_type = StoragePoolType(pool_xml.get("type"))
+
+                # ZFS zvols are block devices; QEMU cannot grow them directly.
+                # Resize the zvol first, then notify the running guest.
+                if pool_type == StoragePoolType.ZFS:
+                    vir_volume.resize(new_size_bytes)
+
+                domain.blockResize(
+                    dev, new_size_bytes, libvirt.VIR_DOMAIN_BLOCK_RESIZE_BYTES
                 )
-            dev = disk.find("target").get("dev")
-
-            pool_xml = ET.fromstring(storage_pool.XMLDesc())
-            pool_type = StoragePoolType(pool_xml.get("type"))
-
-            # ZFS zvols are block devices; QEMU cannot grow them directly.
-            # Resize the zvol first, then notify the running guest.
-            if pool_type == StoragePoolType.ZFS:
-                vir_volume.resize(new_size_bytes)
-
-            domain.blockResize(
-                dev, new_size_bytes, libvirt.VIR_DOMAIN_BLOCK_RESIZE_BYTES
-            )
-            return
+                return
 
         # Ordinary resize via qemu-img
         vir_volume.resize(new_size_bytes)
@@ -1306,7 +1337,13 @@ def create_machine(
 
         storage_pool_xml = ET.fromstring(storage_pool.XMLDesc())
         pool_type = StoragePoolType(storage_pool_xml.get("type"))
-        pool_path = storage_pool_xml.find("target").find("path").text
+        target_el = storage_pool_xml.find("target")
+        path_el = target_el.find("path") if target_el is not None else None
+        if path_el is None or not path_el.text:
+            raise ValueError(
+                f"Storage pool {self._spec.storage_pool} has no target path"
+            )
+        pool_path = path_el.text
 
         # Add the volumes to the domain
         for i, volume in enumerate(volumes):
@@ -1469,7 +1506,7 @@ def rename_machine(self, machine: pool_base.Machine, name: str) -> None:
                 ports=ports,
                 legacy_machine=legacy_domain,
             )
-        except:
+        except Exception:
             machine.name = origin_name
             raise
         LOG.debug("The domain %s was renamed to %s", origin_name, name)

gcl_sdk/tests/unit/agents/drivers/test_libvirt.py

@@ -24,9 +24,64 @@
 pytest.importorskip("libvirt")
 
 from gcl_sdk.agents.universal.drivers.libvirt import XMLLibvirtInstance  # noqa: E402
+from gcl_sdk.agents.universal.drivers.libvirt import _interface_source  # noqa: E402
+from gcl_sdk.agents.universal.drivers.libvirt import _memory_mib  # noqa: E402
 from gcl_sdk.agents.universal.drivers.libvirt import domain_template  # noqa: E402
 
 
+class TestMemoryMib:
+    def test_defaults_to_kib_when_unit_is_absent(self):
+        el = ET.fromstring("<memory>2097152</memory>")
+        assert _memory_mib(el) == 2048
+
+    def test_kib(self):
+        el = ET.fromstring('<memory unit="KiB">2097152</memory>')
+        assert _memory_mib(el) == 2048
+
+    def test_mib(self):
+        el = ET.fromstring('<memory unit="MiB">2048</memory>')
+        assert _memory_mib(el) == 2048
+
+    def test_gib(self):
+        el = ET.fromstring('<currentMemory unit="GiB">2</currentMemory>')
+        assert _memory_mib(el) == 2048
+
+    def test_bytes(self):
+        el = ET.fromstring('<memory unit="bytes">2147483648</memory>')
+        assert _memory_mib(el) == 2048
+
+
+class TestInterfaceSource:
+    def test_no_source_element(self):
+        iface = ET.fromstring("<interface type='network'/>")
+        assert _interface_source(iface, None) is None
+
+    def test_prefers_network_attribute_over_bridge(self):
+        # libvirt rewrites type='network' interfaces backed by a
+        # forward=bridge network to type='bridge' in the live domain XML,
+        # but keeps both attributes on <source> - the logical name is
+        # what the orchestrator's target state actually tracks.
+        iface = ET.fromstring(
+            "<interface type='bridge'>"
+            "<source network='exordos-core-net' bridge='br-core'/>"
+            "</interface>"
+        )
+        source_el = iface.find("source")
+        assert _interface_source(iface, source_el) == "exordos-core-net"
+
+    def test_falls_back_to_own_type_attribute(self):
+        iface = ET.fromstring(
+            "<interface type='bridge'><source bridge='br-core'/></interface>"
+        )
+        source_el = iface.find("source")
+        assert _interface_source(iface, source_el) == "br-core"
+
+    def test_missing_type_attribute_does_not_crash(self):
+        iface = ET.fromstring("<interface><source bridge='br-core'/></interface>")
+        source_el = iface.find("source")
+        assert _interface_source(iface, source_el) is None
+
+
 def test_domain_console_logs_to_file():
     log_path = "/var/log/libvirt/qemu/test-vm.console.log"