diff --git a/gns3server/compute/kernel_datapath.py b/gns3server/compute/kernel_datapath.py index 5d820eab0..b73f1afc3 100644 --- a/gns3server/compute/kernel_datapath.py +++ b/gns3server/compute/kernel_datapath.py @@ -135,6 +135,21 @@ class KernelDatapathMixin: # Link attach / detach # ------------------------------------------------------------------ + @staticmethod + def _owns_anchor_impairments(nio): + """ + Whether this end owns the tc state (netem, cls_bpf, eBPF) of the + anchor it was handed. A NIOBridge with no bridge of its own + (``bridge is None``) means the anchor is absorbed by an Ethernet + switch: the switch owns the anchor's bridge membership and its + impairment state — the controller routes the link's filters to the + switch end, so this NIO always carries an empty filter set, whose + reconcile is a plain ``tc reset``. Applying it here would silently + wipe the netem/bpf/eBPF the switch just put on the shared anchor. + """ + + return not (isinstance(nio, NIOBridge) and nio.bridge is None) + async def _kernel_attach(self, anchor, nio): """ Attach an anchor to the per-link kernel bridge carried by *nio* @@ -150,9 +165,11 @@ class KernelDatapathMixin: A NIO whose ``bridge`` is None means the anchor is bridged elsewhere — an Ethernet switch absorbed it into the switch's own bridge — so the membership work is skipped here and only the link - state the anchor carries (capture, markers, impairment filters) is - applied. Exactly one end of such a link owns the anchor's bridge - state; the other end is a passive carrier. + state this end owns is applied: capture and markers always, the + impairment filters only when this end owns the anchor's tc state + (see _owns_anchor_impairments). Exactly one end of such a link + owns the anchor's bridge and impairment state; the other end is a + passive carrier. """ if nio.bridge is not None: @@ -174,10 +191,13 @@ class KernelDatapathMixin: # (restored here on node restart, like the capture above); bpf # expressions become cls_bpf match-drop classifiers and # frequency_drop/quota/window_drop become the eBPF stateful - # classifier. - await self._ubridge_apply_netem(anchor, nio.filters) - await self._ubridge_apply_bpf_drops(anchor, nio.filters) - await self._ubridge_apply_ebpf_drops(anchor, nio.filters) + # classifier. A passive end owns none of this: its empty filter + # set is a tc reset, which would wipe the switch's qdisc off the + # shared anchor. + if self._owns_anchor_impairments(nio): + await self._ubridge_apply_netem(anchor, nio.filters) + await self._ubridge_apply_bpf_drops(anchor, nio.filters) + await self._ubridge_apply_ebpf_drops(anchor, nio.filters) async def _relay_attach(self, anchor, bridge_name, nio): """ @@ -226,16 +246,19 @@ class KernelDatapathMixin: """ Re-apply everything a kernel link carries on an already-attached anchor (filters and markers changed): no re-enslaving, no bridge - create. + create. A passive end (bridge is None — an anchor absorbed by an + Ethernet switch) leaves the anchor's impairment state to the + switch that owns it. """ await self._ubridge_apply_markers(anchor, nio) # The netem apply resets the interface first (the kernel merges # optional netem attrs on replace), so everything anchored on clsact # must re-apply after it: bpf drops flush + re-add, eBPF modes re-set. - await self._ubridge_apply_netem(anchor, nio.filters) - await self._ubridge_apply_bpf_drops(anchor, nio.filters) - await self._ubridge_apply_ebpf_drops(anchor, nio.filters) + if self._owns_anchor_impairments(nio): + await self._ubridge_apply_netem(anchor, nio.filters) + await self._ubridge_apply_bpf_drops(anchor, nio.filters) + await self._ubridge_apply_ebpf_drops(anchor, nio.filters) async def _remove_kernel_nio(self, nio, adapter_number, port_number=0): """ diff --git a/tests/compute/docker/test_docker_kernel_datapath.py b/tests/compute/docker/test_docker_kernel_datapath.py index f68ddb11f..f54be3dc9 100644 --- a/tests/compute/docker/test_docker_kernel_datapath.py +++ b/tests/compute/docker/test_docker_kernel_datapath.py @@ -322,6 +322,32 @@ async def test_connect_nio_kernel_eexist_still_reapplies_everything(vm): vm._ubridge_send.assert_any_call(f'tc netem set "{host_ifc}" delay 10') +@pytest.mark.asyncio +async def test_connect_nio_kernel_passive_anchor_skips_impairments(vm): + """ + A NIOBridge with no bridge of its own (bridge is None) is the absorbed + end of an Ethernet switch link: the anchor belongs to the switch, which + owns its tc state and applies the link's filters there (the controller + routes them to the switch end, so this NIO always carries {}). The + passive end must not reconcile impairments — an empty filter set is a + tc reset, which would silently wipe the switch's qdisc off the shared + anchor at every node start. + """ + + vm._ubridge_hypervisor = MagicMock() + vm._ubridge_send = AsyncioMagicMock() + host_ifc, _ = vm._veth_names(0, 0) + vm._kernel_veths[(0, 0)] = host_ifc + nio = vm.manager.create_nio({"type": "nio_bridge", "bridge": None}) + + await vm._connect_nio(0, nio) + + tc_calls = [c.args[0] for c in vm._ubridge_send.call_args_list if str(c.args[0]).startswith("tc ")] + assert tc_calls == [], tc_calls + # The membership is the switch's as well: no per-link bridge work here. + assert not any("brctl" in c.args[0] for c in vm._ubridge_send.call_args_list) + + @pytest.mark.asyncio async def test_connect_nio_kernel_create_failure_propagates(vm): @@ -533,6 +559,33 @@ async def test_update_nio_kernel_turns_ebpf_modes_off(vm): assert "nth_drop" not in c.args[0] or "off" in c.args[0] +@pytest.mark.asyncio +async def test_update_nio_kernel_passive_anchor_skips_impairments(vm): + """ + The link-update reconcile on the absorbed (passive) end must leave the + anchor's tc state to the switch: no tc reset / netem / drop classifiers + from this end, or the reset would wipe what the switch applied moments + earlier in the same link update (switch-first links PUT this side last). + The carrier (suspend) is still driven here, and markers still reconcile + (this end may host some). + """ + + vm._ubridge_hypervisor = MagicMock() + vm._ubridge_send = AsyncioMagicMock() + vm.status = "started" + host_ifc, _ = vm._veth_names(0, 0) + vm._kernel_veths[(0, 0)] = host_ifc + nio = vm.manager.create_nio({"type": "nio_bridge", "bridge": None}) + vm._ethernet_adapters[0].add_nio(0, nio) + + await vm.adapter_update_nio_binding(0, nio) + + tc_calls = [c.args[0] for c in vm._ubridge_send.call_args_list if str(c.args[0]).startswith("tc ")] + assert tc_calls == [], tc_calls + # The carrier still follows the suspend flag on the shared anchor. + vm._ubridge_send.assert_any_call(f'link set "{host_ifc}" up') + + @pytest.mark.asyncio async def test_apply_ebpf_drops_requires_ebpf_capability(vm): """ diff --git a/tests/e2e/test_ethernet_switch_kernel_datapath.py b/tests/e2e/test_ethernet_switch_kernel_datapath.py index bbc0e00fa..6dc85fe2c 100644 --- a/tests/e2e/test_ethernet_switch_kernel_datapath.py +++ b/tests/e2e/test_ethernet_switch_kernel_datapath.py @@ -29,7 +29,9 @@ Real ICMP crossing the switch proves the datapath: * VLAN programming applies to absorbed anchors: different access VLANs isolate the routers (ping dies), the same VLAN bridges them again; * a ``delay 100`` filter lands as netem on the absorbed anchor (single - interface, switch-owned) and the measured RTT grows by ~100 ms; + interface, switch-owned) and the measured RTT grows by ~100 ms — on both + link endpoint orders (peer-first and switch-first; the order decides + which side's NIO update runs last and must not decide the outcome); * suspend admin-downs the anchor and kills the traffic; resume restores; * capture (hosted by the switch side) writes a pcap of ICMP records; * deleting a link detaches the anchor (which survives — the router owns @@ -128,8 +130,12 @@ def test_ethernet_switch_kernel_fast_path(): # The switch bridge exists from the moment the switch node does; a # link against a stopped router must not fail — the join defers. assert harness.bridge_members(sw_bridge) == [] + # link1 is created peer-first, link2 switch-first: the endpoint order + # decides which side's NIO update runs last, and only the switch-first + # order used to expose the passive end's tc-reset wipe (its empty-filter + # reconcile ran after the switch's apply). Both orders must work. link1 = compute.create_link(pid, (r1_id, *ETH), (switch_id, 0, 0)) - link2 = compute.create_link(pid, (r2_id, *ETH), (switch_id, 0, 1)) + link2 = compute.create_link(pid, (switch_id, 0, 1), (r2_id, *ETH)) assert link1["kernel_datapath"] is True, link1 assert link2["kernel_datapath"] is True, link2 assert harness.bridge_members(sw_bridge) == [] @@ -223,6 +229,22 @@ def test_ethernet_switch_kernel_fast_path(): fast = harness.wait_ping(c1, "10.1.1.2") assert fast["success"] == 100 and fast["avg"] < 50, fast + # The same single-owned impairment on link2 — created switch-first, + # the order whose peer NIO update runs LAST. The passive end's empty + # reconcile used to tc-reset the shared anchor right after the switch + # applied the netem, wiping it (API green, data plane unimpaired); + # the qdisc and the RTT must both hold under this order too. + compute.call("PUT", f"/projects/{pid}/links/{link2['link_id']}", {"filters": {"delay": [100]}}) + assert "netem" in harness.qdiscs(a2), harness.qdiscs(a2) + assert "netem" not in harness.qdiscs(a1), harness.qdiscs(a1) + delayed2 = harness.wait_ping(c1, "10.1.1.2") + assert delayed2["success"] == 100, delayed2["raw"] + assert 60 <= delayed2["avg"] <= 250, (baseline, delayed2) + compute.call("PUT", f"/projects/{pid}/links/{link2['link_id']}", {"filters": {}}) + assert "netem" not in harness.qdiscs(a2) + fast2 = harness.wait_ping(c1, "10.1.1.2") + assert fast2["success"] == 100 and fast2["avg"] < 50, fast2 + # suspend: the anchor admin-downs, the link is dead; resume restores compute.call("PUT", f"/projects/{pid}/links/{link1['link_id']}", {"suspend": True}) assert not harness.tap_up(a1)