Merge pull request #1548 from kernelkit/veth-regression

src/confd/src/cni.c

@@ -415,13 +415,15 @@ int cni_netdag_gen_iface(struct dagger *net, const char *ifname,
 
 		fprintf(fp, "container -a -f delete network %s >/dev/null\n", ifname);
 
-		/* If this end belongs to a veth pair, the kernel keeps the pair
-		 * alive after CNI host-device returns the interface to the host
-		 * namespace.  Remove it here, once the container is gone, so the
-		 * pair does not linger and block a later re-creation.  Tolerant:
-		 * the peer's teardown may already have removed it.
+		/* When both ends of a veth pair are in containers, neither end
+		 * is torn down from the host namespace, so the kernel keeps the
+		 * pair alive after CNI host-device returns the interfaces here.
+		 * Have the primary end delete it, once its container is gone.
+		 * When the peer is a host interface it is the primary and deletes
+		 * the pair itself (veth_gen_del); we must not race it here.
+		 * Tolerant: the pair may already be gone.
 		 */
-		if (lydx_get_child(dif, "veth"))
+		if (lydx_get_child(dif, "veth") && veth_is_primary(dif))
 			fprintf(fp, "ip link del dev %s 2>/dev/null || true\n", ifname);
 
 		fclose(fp);

test/case/containers/internal_link/test.adoc

@@ -29,5 +29,6 @@ image::topology.svg[VETH Pair Between Two Containers topology, align=center, sca
 . Create VETH pair with both ends assigned to containers
 . Verify both containers have started
 . Verify {LEFT} reaches {RIGHT} over the internal VETH pair
+. Remove containers and VETH pair, verify clean teardown
 
 

test/case/containers/internal_link/test.py

@@ -98,4 +98,21 @@ with infamy.Test() as test:
 
         until(reachable, attempts=30)
 
+    with test.step("Remove containers and VETH pair, verify clean teardown"):
+        # Removing a pair with both ends in containers must tear it down
+        # cleanly, leaving nothing behind in the host namespace.
+        target.delete_xpaths([
+            f"/infix-containers:containers/container[name='{LEFT}']",
+            f"/infix-containers:containers/container[name='{RIGHT}']",
+            f"/ietf-interfaces:interfaces/interface[name='{IFACE_LEFT}']",
+            f"/ietf-interfaces:interfaces/interface[name='{IFACE_RIGHT}']",
+        ])
+
+        def removed():
+            ifaces = target.get_data("/ietf-interfaces:interfaces")["interfaces"]["interface"]
+            names = [iface["name"] for iface in ifaces]
+            return IFACE_LEFT not in names and IFACE_RIGHT not in names
+
+        until(removed, attempts=30)
+
     test.succeed()

test/case/containers/veth/test.adoc

@@ -28,5 +28,6 @@ image::topology.svg[Container with VETH Pair topology, align=center, scaledwidth
 . Verify container 'web-br0-veth' has started
 . Verify basic DUT connectivity, host:data can ping DUT 10.0.0.2
 . Verify container 'web-br0-veth' is reachable on http://10.0.0.2:91
+. Remove container and VETH pair, verify clean teardown
 
 

test/case/containers/veth/test.py

@@ -108,4 +108,21 @@ with infamy.Test() as test:
         with test.step("Verify container 'web-br0-veth' is reachable on http://10.0.0.2:91"):
             until(lambda: MESG in ns.call(lambda: curl(URL)), attempts=10)
 
+    with test.step("Remove container and VETH pair, verify clean teardown"):
+        # Regression test for #941/#1546: removing a veth pair with one end
+        # in a container must tear the pair down cleanly, leaving nothing
+        # behind in the host namespace.
+        target.delete_xpaths([
+            f"/infix-containers:containers/container[name='{NAME}']",
+            f"/ietf-interfaces:interfaces/interface[name='{NAME}']",
+            "/ietf-interfaces:interfaces/interface[name='veth0b']",
+        ])
+
+        def removed():
+            ifaces = target.get_data("/ietf-interfaces:interfaces")["interfaces"]["interface"]
+            names = [iface["name"] for iface in ifaces]
+            return NAME not in names and "veth0b" not in names
+
+        until(removed, attempts=30)
+
     test.succeed()

test/case/interfaces/veth_delete/test.adoc

@@ -23,8 +23,7 @@ image::topology.svg[Verify that VETH Pairs Can Be Deleted topology, align=center
 . Verify interfaces 'veth0a' and 'veth0b' exist
 . Set IP address on target:data1 (dummy op)
 . Set IP address on target:data2 (dummy op)
-. Reset configuration
-. Verify target:data1 and target:data2 still exist
-. Verify VETH pair have been removed
+. Delete VETH pair
+. Verify VETH pair has been removed
 
 

test/case/interfaces/veth_delete/test.py

@@ -86,23 +86,15 @@ with infamy.Test() as test:
             }
         }})
 
-    # TODO: need target.del_config_dict() or similar for VETH _pairs_,
-    #       because both interfaces must be removed at the same time.
-    # with test.step("Delete VETH pair"):
-    #     xpath = f"/ietf-interfaces:interfaces/interface[name='{veth0a}']"
-    #     target.delete_xpath(xpath)
-    # XXX: temporary workaround
-    with test.step("Reset configuration"):
-        # Calls target.test_reset() to apply safe-config
-        target = env.attach("target", "mgmt")
+    with test.step("Delete VETH pair"):
+        # Both ends are mutually mandatory leafrefs, so they must be
+        # removed in a single transaction.
+        target.delete_xpaths([
+            f"/ietf-interfaces:interfaces/interface[name='{veth0a}']",
+            f"/ietf-interfaces:interfaces/interface[name='{veth0b}']",
+        ])
 
-    with test.step("Verify target:data1 and target:data2 still exist"):
-        assert iface.exist(target, data1), \
-            f"Interface {data1} missing!"
-        assert iface.exist(target, data2), \
-            f"Interface {data2} missing!"
-
-    with test.step("Verify VETH pair have been removed"):
+    with test.step("Verify VETH pair has been removed"):
         assert not iface.exist(target, veth0a), \
             f"Interface <{veth0a}> still exists!"
         assert not iface.exist(target, veth0b), \

test/infamy/netconf.py

@@ -455,3 +455,47 @@ class Device(Transport):
         # Apply the configuration change
         return self.put_config(lyd.print_mem("xml", with_siblings=True,
                                              pretty=False))
+
+    def delete_xpaths(self, xpaths):
+        """Delete several xpaths in a single transaction.
+
+        Needed when the targets reference one another, e.g. both ends of a
+        VETH pair (mutually mandatory leafrefs), and so cannot be removed
+        one at a time.
+        """
+        pattern = r"^/(?P<module>[^:]+):(?P<path>[^/]+)"
+
+        # Group xpaths by their top-level module/container so each is
+        # removed from a single fetched config tree (one diff per module).
+        groups = {}
+        order = []
+        for xpath in xpaths:
+            coverage.track_xpath(xpath)
+            match = re.search(pattern, xpath)
+            if not match:
+                raise ValueError(f"Failed parsing xpath:{xpath}")
+            modpath = f"/{match.group('module')}:{match.group('path')}"
+            if modpath not in groups:
+                groups[modpath] = (match.group('module'), [])
+                order.append(modpath)
+            groups[modpath][1].append(xpath)
+
+        edit = ""
+        for modpath in order:
+            module, paths = groups[modpath]
+            old = self.get_config_dict(modpath)
+            new = copy.deepcopy(old)
+            for xpath in paths:
+                if not libyang.xpath_del(new, xpath):
+                    raise ValueError(f"Failed to delete specified xpath: {xpath}")
+
+            mod = self.ly.get_module(module)
+            oldd = mod.parse_data_dict(old, no_state=True, validate=False)
+            newd = mod.parse_data_dict(new, no_state=True, validate=False)
+
+            lyd = oldd.diff(newd)
+            if lyd is None:
+                raise ValueError(f"Failed generating diff for {modpath}")
+            edit += lyd.print_mem("xml", with_siblings=True, pretty=False)
+
+        return self.put_config(edit)

test/infamy/restconf.py

@@ -502,3 +502,23 @@ class Device(Transport):
         response.raise_for_status()
 
         return True
+
+    def delete_xpaths(self, xpaths):
+        """Delete several xpaths in a single transaction.
+
+        Needed when the targets reference one another, e.g. both ends of a
+        VETH pair (mutually mandatory leafrefs), and so cannot be removed
+        one at a time.  Stage the deletions in the candidate datastore, then
+        copy it to running so they commit and validate together.
+        """
+        self.copy("running", "candidate")
+        for xpath in xpaths:
+            coverage.track_xpath(xpath)
+            path = f"/ds/ietf-datastores:candidate{xpath_to_uri(xpath)}"
+            url = f"{self.restconf_url}{path}"
+            response = requests_workaround_delete(url, headers=self.headers,
+                                                  auth=self.auth, verify=False)
+            response.raise_for_status()
+        self.copy("candidate", "running")
+
+        return True

test/infamy/transport.py

@@ -34,6 +34,10 @@ class Transport(ABC):
     def delete_xpath(self, xpath):
         pass
 
+    @abstractmethod
+    def delete_xpaths(self, xpaths):
+        pass
+
     @abstractmethod
     def copy(self, source, target):
         pass