fix(drivers): fix DOM child-removal and idempotent delete_machine (#528)

- document_set_tag/document_meta_set_tag used root.getElementsByTagName(tag).removeChild(node), but getElementsByTagName searches the whole subtree recursively; a match found deeper than a direct child makes removeChild raise xml.dom.NotFoundErr instead of removing anything (confirmed empirically). New _remove_direct_children() only removes actual direct children, shared by both methods.
- delete_machine() called lookupByUUIDString unguarded, crashing with an uncaught libvirtError if the domain was already gone (e.g. a retry after an earlier delete destroyed/undefined it but failed later during volume cleanup). Now treats VIR_ERR_NO_DOMAIN as nothing-to-do and proceeds straight to volume cleanup.

Author: VasilyStepanov

exordos_core/compute/pool/drivers/libvirt.py

@@ -243,6 +243,19 @@ def add_element(
 
         root.appendChild(element)
 
+    @classmethod
+    def _remove_direct_children(cls, parent: minidom.Element, tag_name: str) -> None:
+        """Remove `parent`'s direct-child elements named `tag_name`.
+
+        getElementsByTagName searches the whole subtree recursively, not
+        just direct children - removeChild() raises NotFoundErr if a
+        match it returns isn't actually a direct child of the node
+        you're calling it on.
+        """
+        for node in list(parent.childNodes):
+            if node.nodeType == node.ELEMENT_NODE and node.tagName == tag_name:
+                parent.removeChild(node)
+
     @classmethod
     def document_set_tag(
         cls,
@@ -255,14 +268,12 @@ def document_set_tag(
     ) -> None:
         root = parent or docement.firstChild
         # Firstly we need to remove the old value
-        for node in root.getElementsByTagName(tag_name):
-            root.removeChild(node)
+        cls._remove_direct_children(root, tag_name)
 
         # 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.removeChild(node)
+            cls._remove_direct_children(meta_node, meta_tag)
 
             # Add the new value
             cls.add_element(docement, meta_tag, parent=meta_node, text=text)
@@ -280,8 +291,7 @@ def document_meta_set_tag(
     ) -> None:
         # Remove the old value from the meta
         meta_node = docement.getElementsByTagName(META_TAG)[0]
-        for node in docement.getElementsByTagName(tag):
-            meta_node.removeChild(node)
+        cls._remove_direct_children(meta_node, tag)
 
         # Add the new value
         cls.add_element(docement, tag, parent=meta_node, text=text, **kwargs)
@@ -1321,16 +1331,30 @@ def delete_machine(
 
         :param machine: The machine to delete
         """
-        domain = self._client.lookupByUUIDString(str(machine.uuid))
-
-        # Remove the libvirt domain
+        # Idempotent: a retry after a previous delete already destroyed/
+        # undefined the domain but failed later (e.g. during volume
+        # cleanup below) must not crash here just because the domain is
+        # already gone.
         try:
-            domain.destroy()
-        except libvirt.libvirtError:
-            LOG.debug("The domain is not in the running state")
-        # FIXME(akremenetsky): Actully we should undefine the
-        # domain before volume deletion
-        domain.undefine()
+            domain = self._client.lookupByUUIDString(str(machine.uuid))
+        except libvirt.libvirtError as e:
+            if e.get_error_code() != libvirt.VIR_ERR_NO_DOMAIN:
+                raise
+            LOG.debug(
+                "Domain for machine %s not found, assuming already deleted",
+                machine.uuid,
+            )
+            domain = None
+
+        if domain is not None:
+            # Remove the libvirt domain
+            try:
+                domain.destroy()
+            except libvirt.libvirtError:
+                LOG.debug("The domain is not in the running state")
+            # FIXME(akremenetsky): Actully we should undefine the
+            # domain before volume deletion
+            domain.undefine()
 
         if delete_volumes:
             for volume in self.list_volumes(machine):

exordos_core/tests/unit/compute/pool/drivers/test_libvirt.py

@@ -14,10 +14,33 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import uuid as sys_uuid
+from unittest import mock
+from xml.dom import minidom
 from xml.etree import ElementTree as ET
 
-from exordos_core.compute.pool.drivers.libvirt import XMLLibvirtInstance
-from exordos_core.compute.pool.drivers.libvirt import domain_template
+import pytest
+
+# The libvirt driver imports the `libvirt` python bindings at module level.
+# They aren't always installed, so skip this module instead of failing
+# collection when they're not available.
+pytest.importorskip("libvirt")
+
+from exordos_core.compute.dm import models  # noqa: E402
+from exordos_core.compute.pool.drivers.libvirt import LibvirtPoolDriver  # noqa: E402
+from exordos_core.compute.pool.drivers.libvirt import XMLLibvirtInstance  # noqa: E402
+from exordos_core.compute.pool.drivers.libvirt import domain_template  # noqa: E402
+
+
+def _local_driver() -> LibvirtPoolDriver:
+    # libvirt's built-in "test" driver simulates a hypervisor in-memory -
+    # no real virtualization or daemon needed, so real libvirt calls
+    # (lookupByUUIDString, etc.) can be exercised end-to-end.
+    spec = models.LibvirtPoolDriverSpec(connection_uri="test:///default")
+    pool = models.MachinePool(
+        uuid=sys_uuid.uuid4(), name="test-pool", driver_spec=spec
+    )
+    return LibvirtPoolDriver(pool)
 
 
 def test_domain_console_logs_to_file():
@@ -35,3 +58,82 @@ def test_domain_console_logs_to_file():
     assert console.get("type") == "pty"
     assert log.get("file") == log_path
     assert log.get("append") == "on"
+
+
+class TestRemoveDirectChildren:
+    def test_removes_only_direct_children_leaving_nested_matches_alone(self):
+        # getElementsByTagName searches the whole subtree recursively -
+        # a naive removeChild(node) on a match found deeper in the tree
+        # (not a direct child of root) raises NotFoundErr.
+        doc = minidom.parseString(
+            "<root><a>direct</a><b><a>nested</a></b></root>"
+        )
+        root = doc.firstChild
+
+        XMLLibvirtInstance._remove_direct_children(root, "a")
+
+        assert root.getElementsByTagName("a") == doc.getElementsByTagName("b")[
+            0
+        ].getElementsByTagName("a")
+        assert len(doc.getElementsByTagName("a")) == 1
+        assert doc.getElementsByTagName("a")[0].firstChild.data == "nested"
+
+    def test_leaves_other_tag_names_alone(self):
+        doc = minidom.parseString("<root><a>1</a><c>2</c></root>")
+        root = doc.firstChild
+
+        XMLLibvirtInstance._remove_direct_children(root, "a")
+
+        assert len(doc.getElementsByTagName("a")) == 0
+        assert len(doc.getElementsByTagName("c")) == 1
+
+    def test_re_setting_a_tag_with_a_same_named_nested_element_does_not_crash(self):
+        # Regression: domain_set_vcpu/domain_set_memory/etc. re-set their
+        # tag on every call - this must not crash even if some unrelated
+        # nested element happens to share the tag name.
+        domain = XMLLibvirtInstance(domain_template)
+        devices = ET.fromstring(domain.xml).find("devices")
+        assert devices is not None  # sanity: domain_template has one
+
+        domain.set_vcpu(2)
+        domain.set_vcpu(4)
+        domain.set_memory(1024)
+        domain.set_memory(2048)
+
+        element = ET.fromstring(domain.xml)
+        assert element.find(".//vcpu").text == "4"
+        assert element.find(".//currentMemory").text == "2048"
+
+
+class TestDeleteMachine:
+    def test_is_idempotent_when_the_domain_is_already_gone(self):
+        driver = _local_driver()
+        machine = models.Machine(
+            uuid=sys_uuid.uuid4(),
+            project_id=sys_uuid.uuid4(),
+            name="never-existed",
+            cores=1,
+            ram=512,
+        )
+
+        # Must not raise, even though no such domain was ever defined.
+        driver.delete_machine(machine, delete_volumes=False)
+
+    def test_volume_cleanup_still_runs_when_the_domain_is_already_gone(self):
+        driver = _local_driver()
+        machine = models.Machine(
+            uuid=sys_uuid.uuid4(),
+            project_id=sys_uuid.uuid4(),
+            name="never-existed",
+            cores=1,
+            ram=512,
+        )
+
+        # The missing-domain path must fall through to volume cleanup,
+        # not skip it.
+        with mock.patch.object(
+            driver, "list_volumes", return_value=[]
+        ) as mock_list_volumes:
+            driver.delete_machine(machine, delete_volumes=True)
+
+        mock_list_volumes.assert_called_once_with(machine)