From 7dfe12eef838f2a8be15e8c58e47f4bcd8b64239 Mon Sep 17 00:00:00 2001 From: Philippe Guibert Date: Wed, 3 Jul 2024 14:37:52 +0200 Subject: [PATCH 1/2] topotests: bgp_peer_type_multipath_relax, adds the duplicate flag During the bgp_peer_type_multipath_relax_test, the test does not check the 'duplicate' flag value of the duplicate nexthop. Fix this by adding the duplicate value in the expected json files. Fixes: ee88563ac2ea ("bgpd: Add 'bgp bestpath peer-type multipath-relax'") Signed-off-by: Philippe Guibert --- .../bgp_peer_type_multipath_relax/r1/prefix1-eBGP-confed.json | 1 + .../bgp_peer_type_multipath_relax/r1/prefix1-eBGP-iBGP.json | 1 + 2 files changed, 2 insertions(+) diff --git a/tests/topotests/bgp_peer_type_multipath_relax/r1/prefix1-eBGP-confed.json b/tests/topotests/bgp_peer_type_multipath_relax/r1/prefix1-eBGP-confed.json index 22ec2c298b0e..791b92df65ee 100644 --- a/tests/topotests/bgp_peer_type_multipath_relax/r1/prefix1-eBGP-confed.json +++ b/tests/topotests/bgp_peer_type_multipath_relax/r1/prefix1-eBGP-confed.json @@ -24,6 +24,7 @@ }, { "fib":true, + "duplicate":true, "ip":"10.0.3.2", "active":true } diff --git a/tests/topotests/bgp_peer_type_multipath_relax/r1/prefix1-eBGP-iBGP.json b/tests/topotests/bgp_peer_type_multipath_relax/r1/prefix1-eBGP-iBGP.json index facddcda463f..1fe9a6799f75 100644 --- a/tests/topotests/bgp_peer_type_multipath_relax/r1/prefix1-eBGP-iBGP.json +++ b/tests/topotests/bgp_peer_type_multipath_relax/r1/prefix1-eBGP-iBGP.json @@ -24,6 +24,7 @@ }, { "fib":true, + "duplicate":true, "ip":"10.0.3.2", "active":true } From 731f74e35fa2c1636208f4bf64650d2d00a199b4 Mon Sep 17 00:00:00 2001 From: Philippe Guibert Date: Wed, 3 Jul 2024 08:51:51 +0200 Subject: [PATCH 2/2] zebra, topotests: do not set nexthop's FIB flag when DUPLICATE present The bgp_duplicate_nexthop test installs routes with nexthop's flags set to both DUPLICATE and FIB: this should not happen. The DUPLICATE flag of a nexthop indicates this nexthop is already used in the same nexthop-group, and there is no need to install it twice in the system; having the FIB flag set indicates that the nexthop is installed in the system. This is why both flags should not be set on the same nexthop. This case happens at installation time, but can also happen at update time. - Fix this by not setting the FIB flag value when the DUPLICATE flag is present. - Modify the bgp_duplicate_test to check that the FIB flag is not present on duplicated nexthops. - Modify the bgp_peer_type_multipath_relax test. Signed-off-by: Philippe Guibert --- .../test_bgp_duplicate_nexthop.py | 2 +- .../r1/prefix1-eBGP-confed.json | 1 - .../r1/prefix1-eBGP-iBGP.json | 1 - tests/topotests/lib/common_check.py | 27 +++++++++++++++---- zebra/zebra_dplane.c | 4 +++ zebra/zebra_rib.c | 3 +++ 6 files changed, 30 insertions(+), 8 deletions(-) diff --git a/tests/topotests/bgp_duplicate_nexthop/test_bgp_duplicate_nexthop.py b/tests/topotests/bgp_duplicate_nexthop/test_bgp_duplicate_nexthop.py index 955881e6f905..83fae71bf59f 100644 --- a/tests/topotests/bgp_duplicate_nexthop/test_bgp_duplicate_nexthop.py +++ b/tests/topotests/bgp_duplicate_nexthop/test_bgp_duplicate_nexthop.py @@ -320,7 +320,7 @@ def check_ipv4_prefix_recursive_with_multiple_nexthops( ) test_func = functools.partial( - ip_check_path_selection, tgen.gears["r1"], prefix, expected + ip_check_path_selection, tgen.gears["r1"], prefix, expected, check_fib=True ) _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5) assert ( diff --git a/tests/topotests/bgp_peer_type_multipath_relax/r1/prefix1-eBGP-confed.json b/tests/topotests/bgp_peer_type_multipath_relax/r1/prefix1-eBGP-confed.json index 791b92df65ee..483165c0f377 100644 --- a/tests/topotests/bgp_peer_type_multipath_relax/r1/prefix1-eBGP-confed.json +++ b/tests/topotests/bgp_peer_type_multipath_relax/r1/prefix1-eBGP-confed.json @@ -23,7 +23,6 @@ "recursive":true }, { - "fib":true, "duplicate":true, "ip":"10.0.3.2", "active":true diff --git a/tests/topotests/bgp_peer_type_multipath_relax/r1/prefix1-eBGP-iBGP.json b/tests/topotests/bgp_peer_type_multipath_relax/r1/prefix1-eBGP-iBGP.json index 1fe9a6799f75..638a825395e2 100644 --- a/tests/topotests/bgp_peer_type_multipath_relax/r1/prefix1-eBGP-iBGP.json +++ b/tests/topotests/bgp_peer_type_multipath_relax/r1/prefix1-eBGP-iBGP.json @@ -23,7 +23,6 @@ "recursive":true }, { - "fib":true, "duplicate":true, "ip":"10.0.3.2", "active":true diff --git a/tests/topotests/lib/common_check.py b/tests/topotests/lib/common_check.py index be3241fd2022..19f02dbadc9f 100644 --- a/tests/topotests/lib/common_check.py +++ b/tests/topotests/lib/common_check.py @@ -10,11 +10,13 @@ from lib import topotest -def ip_check_path_selection(router, ipaddr_str, expected, vrf_name=None): +def ip_check_path_selection( + router, ipaddr_str, expected, vrf_name=None, check_fib=False +): if vrf_name: - cmdstr = f'show ip route vrf {vrf_name} {ipaddr_str} json' + cmdstr = f"show ip route vrf {vrf_name} {ipaddr_str} json" else: - cmdstr = f'show ip route {ipaddr_str} json' + cmdstr = f"show ip route {ipaddr_str} json" try: output = json.loads(router.vtysh_cmd(cmdstr)) except: @@ -25,6 +27,21 @@ def ip_check_path_selection(router, ipaddr_str, expected, vrf_name=None): num_nh_expected = len(expected[ipaddr_str][0]["nexthops"]) num_nh_observed = len(output[ipaddr_str][0]["nexthops"]) if num_nh_expected == num_nh_observed: + if check_fib: + # special case: when fib flag is unset, + # an extra test should be done to check that the flag is really unset + for nh_output, nh_expected in zip( + output[ipaddr_str][0]["nexthops"], + expected[ipaddr_str][0]["nexthops"], + ): + if ( + "fib" in nh_output.keys() + and nh_output["fib"] + and ("fib" not in nh_expected.keys() or not nh_expected["fib"]) + ): + return "{}, prefix {} nexthop {} has the fib flag set, whereas it is not expected".format( + router.name, ipaddr_str, nh_output["ip"] + ) return ret return "{}, prefix {} does not have the correct number of nexthops : observed {}, expected {}".format( router.name, ipaddr_str, num_nh_observed, num_nh_expected @@ -37,9 +54,9 @@ def iproute2_check_path_selection(router, ipaddr_str, expected, vrf_name=None): return None if vrf_name: - cmdstr = f'ip -json route show vrf {vrf_name} {ipaddr_str}' + cmdstr = f"ip -json route show vrf {vrf_name} {ipaddr_str}" else: - cmdstr = f'ip -json route show {ipaddr_str}' + cmdstr = f"ip -json route show {ipaddr_str}" try: output = json.loads(cmdstr) except: diff --git a/zebra/zebra_dplane.c b/zebra/zebra_dplane.c index 7910559c4b41..0844b346723b 100644 --- a/zebra/zebra_dplane.c +++ b/zebra/zebra_dplane.c @@ -4314,6 +4314,10 @@ dplane_route_update_internal(struct route_node *rn, NEXTHOP_FLAG_RECURSIVE)) continue; + if (CHECK_FLAG(nexthop->flags, + NEXTHOP_FLAG_DUPLICATE)) + continue; + if (CHECK_FLAG(nexthop->flags, NEXTHOP_FLAG_ACTIVE)) SET_FLAG(nexthop->flags, diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c index b176ea2fe6c5..142f83fb366d 100644 --- a/zebra/zebra_rib.c +++ b/zebra/zebra_rib.c @@ -1659,6 +1659,9 @@ static bool rib_update_nhg_from_ctx(struct nexthop_group *re_nhg, if (!CHECK_FLAG(nexthop->flags, NEXTHOP_FLAG_ACTIVE)) continue; + if (CHECK_FLAG(nexthop->flags, NEXTHOP_FLAG_DUPLICATE)) + continue; + /* Check for a FIB nexthop corresponding to the RIB nexthop */ if (!nexthop_same(ctx_nexthop, nexthop)) { /* If the FIB doesn't know about the nexthop,