From b9538fe481d1955090e41c04d86b2322362a83f5 Mon Sep 17 00:00:00 2001 From: anlan_cs Date: Sat, 30 Nov 2024 21:11:04 +0800 Subject: [PATCH 1/4] zebra: fix wrong nexthop check The kernel routes are wrongly selected even the nexthop interface is linkdown. Use `ip link set dev down` on the other box to set the box's nexthop interface linkdown. The kernel routes will be kept as `linkdown`, but are still with active nexthop in `zebra`. Add three changes/commits for kernel routes in this PR: 1) The active nexthop should be the operative interface. 2) Don't uninstall the kernel routes from `zebra` even no active nexthops. (It doesn't affect the kernel routes' deletion from kernel netlink messages.) 3) Update the kernel routes when the nexthop interface becomes up. Before: (during nexthop interface is linkdown) ``` K>* 3.3.3.3/32 [0/0] via 88.88.88.1, enp2s0, weight 1, 00:00:14 ``` After: (during nexthop interface is linkdown, with all three changes) ``` K 3.3.3.3/32 [0/0] via 88.88.88.1, enp2s0 inactive, weight 1, 00:00:07 ``` This commit is 1st change: Improve the judgment for "active" nexthop to be more accurate, the active nexthop should be the operative interface. Signed-off-by: anlan_cs --- zebra/zebra_nhg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 1519246c17..e61c158ca9 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -2648,7 +2648,7 @@ static unsigned nexthop_active_check(struct route_node *rn, ifp = if_lookup_by_index(nexthop->ifindex, nexthop->vrf_id); - if (ifp && ifp->vrf->vrf_id == vrf_id && if_is_up(ifp)) { + if (ifp && ifp->vrf->vrf_id == vrf_id && if_is_up(ifp) && if_is_operative(ifp)) { SET_FLAG(nexthop->flags, NEXTHOP_FLAG_ACTIVE); goto skip_check; } From 298bc623e78a15ae950bfbac9a103e24ac57e8bd Mon Sep 17 00:00:00 2001 From: anlan_cs Date: Sat, 14 Dec 2024 18:30:18 +0800 Subject: [PATCH 2/4] zebra: don't uninstall kernel routes After the nexthop check is fixed, zebra will wrongly uninstall the kernel routes with inactive nexthop. This commit would skip the uninstallation for kernel routes. Signed-off-by: anlan_cs --- zebra/zebra_rib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c index e64a620f00..0521029e6c 100644 --- a/zebra/zebra_rib.c +++ b/zebra/zebra_rib.c @@ -1480,7 +1480,7 @@ static void rib_process(struct route_node *rn) rib_process_update_fib(zvrf, rn, old_fib, new_fib); else if (new_fib) rib_process_add_fib(zvrf, rn, new_fib); - else if (old_fib) + else if (old_fib && !RIB_SYSTEM_ROUTE(old_fib)) rib_process_del_fib(zvrf, rn, old_fib); /* Remove all RE entries queued for removal */ From 4d2ac714f01c5d33cb3fc47f34a0eafba5dfd13e Mon Sep 17 00:00:00 2001 From: anlan_cs Date: Sat, 14 Dec 2024 18:40:45 +0800 Subject: [PATCH 3/4] zebra: check kernel routes when interface becomes up Just like `link down`, check all kernel routes when interface become up. And, they maybe will be selected as the best one by zebra. Signed-off-by: anlan_cs --- zebra/interface.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/zebra/interface.c b/zebra/interface.c index f7fd112cd4..1c86a6a5c7 100644 --- a/zebra/interface.c +++ b/zebra/interface.c @@ -972,6 +972,8 @@ void if_up(struct interface *ifp, bool install_connected) event_ignore_late_timer(zif->speed_update); if_addr_wakeup(ifp); + + rib_update_handle_vrf_all(RIB_UPDATE_KERNEL, ZEBRA_ROUTE_KERNEL); } /* Interface goes down. We have to manage different behavior of based From 2f1bd3fe4dd663b29a0e62ad9f78997a176de5ae Mon Sep 17 00:00:00 2001 From: anlan_cs Date: Fri, 13 Dec 2024 18:38:08 +0800 Subject: [PATCH 4/4] tests: add nexthop/interface's down/up topo for kernel routes Signed-off-by: anlan_cs --- .../r1/ip_route_kernel_interface_down.json | 20 ++++++++ .../r1/ip_route_kernel_interface_up.json | 21 +++++++++ .../test_zebra_multiple_connected.py | 47 +++++++++++++++++++ 3 files changed, 88 insertions(+) create mode 100644 tests/topotests/zebra_multiple_connected/r1/ip_route_kernel_interface_down.json create mode 100644 tests/topotests/zebra_multiple_connected/r1/ip_route_kernel_interface_up.json diff --git a/tests/topotests/zebra_multiple_connected/r1/ip_route_kernel_interface_down.json b/tests/topotests/zebra_multiple_connected/r1/ip_route_kernel_interface_down.json new file mode 100644 index 0000000000..50871ae038 --- /dev/null +++ b/tests/topotests/zebra_multiple_connected/r1/ip_route_kernel_interface_down.json @@ -0,0 +1,20 @@ +{ + "5.5.6.7/32":[ + { + "prefix":"5.5.6.7/32", + "prefixLen":32, + "protocol":"kernel", + "vrfName":"default", + "internalFlags":0, + "internalNextHopNum":1, + "internalNextHopActiveNum":0, + "nexthops":[ + { + "flags":0, + "interfaceName":"r1-eth2" + } + ] + + } + ] +} diff --git a/tests/topotests/zebra_multiple_connected/r1/ip_route_kernel_interface_up.json b/tests/topotests/zebra_multiple_connected/r1/ip_route_kernel_interface_up.json new file mode 100644 index 0000000000..d0ab2fa187 --- /dev/null +++ b/tests/topotests/zebra_multiple_connected/r1/ip_route_kernel_interface_up.json @@ -0,0 +1,21 @@ +{ + "5.5.6.7/32":[ + { + "prefix":"5.5.6.7/32", + "prefixLen":32, + "protocol":"kernel", + "vrfName":"default", + "internalFlags":8, + "internalNextHopNum":1, + "internalNextHopActiveNum":1, + "nexthops":[ + { + "flags":3, + "fib":true, + "interfaceName":"r1-eth2", + "active":true + } + ] + } + ] +} diff --git a/tests/topotests/zebra_multiple_connected/test_zebra_multiple_connected.py b/tests/topotests/zebra_multiple_connected/test_zebra_multiple_connected.py index eda8c88706..89bc6cf8e0 100644 --- a/tests/topotests/zebra_multiple_connected/test_zebra_multiple_connected.py +++ b/tests/topotests/zebra_multiple_connected/test_zebra_multiple_connected.py @@ -65,6 +65,9 @@ def build_topo(tgen): switch.add_link(tgen.gears["r2"]) switch.add_link(tgen.gears["r3"]) + # Create a p2p connection between r1 and r2 + tgen.add_link(tgen.gears["r1"], tgen.gears["r2"]) + ##################################################### ## @@ -222,6 +225,50 @@ def test_zebra_kernel_route_blackhole_add(): result, _ = topotest.run_and_expect(test_func, None, count=20, wait=1) assert result, "Blackhole Route should have not been removed\n{}".format(_) +def test_zebra_kernel_route_interface_linkdown(): + "Test that a kernel routes should be affected by interface change" + + tgen = get_topogen() + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + router = tgen.gears["r1"] + router.run("ip route add 5.5.6.7/32 via 10.0.1.66 dev r1-eth2") + + kernel = "{}/{}/ip_route_kernel_interface_up.json".format(CWD, router.name) + expected = json.loads(open(kernel).read()) + + test_func = partial( + topotest.router_json_cmp, router, "show ip route 5.5.6.7/32 json", expected + ) + result, _ = topotest.run_and_expect(test_func, None, count=20, wait=1) + assert result, "Kernel Route should be selected:\n{}".format(_) + + # link down + router2 = tgen.gears["r2"] + router2.run("ip link set dev r2-eth2 down") + + kernel = "{}/{}/ip_route_kernel_interface_down.json".format(CWD, router.name) + expected = json.loads(open(kernel).read()) + + test_func = partial( + topotest.router_json_cmp, router, "show ip route 5.5.6.7/32 json", expected + ) + result, _ = topotest.run_and_expect(test_func, None, count=20, wait=1) + assert result, "Kernel Route should not be selected:\n{}".format(_) + + # link up + router2 = tgen.gears["r2"] + router2.run("ip link set dev r2-eth2 up") + + kernel = "{}/{}/ip_route_kernel_interface_up.json".format(CWD, router.name) + expected = json.loads(open(kernel).read()) + + test_func = partial( + topotest.router_json_cmp, router, "show ip route 5.5.6.7/32 json", expected + ) + result, _ = topotest.run_and_expect(test_func, None, count=20, wait=1) + assert result, "Kernel Route should be selected:\n{}".format(_) if __name__ == "__main__": args = ["-s"] + sys.argv[1:]