bgpd: fix do not skip paths with same nexthop #16153
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I am surprised that it passes, I was thinking there was already something that was covering this. I would like a feedback if you think it is worth configuring this behaviour ? |
pguibert6WIND
force-pushed
the
bgp_recursive_duplicate
branch
from
2a4692b to
a2701c7
Compare
pguibert6WIND
force-pushed
the
bgp_recursive_duplicate
branch
4 times, most recently
from
e635ddb to
079cde9
Compare
ton31337
reviewed
Jun 7, 2024
| @@ -0,0 +1,14 @@ | |||
| debug bgp bestpath 192.0.2.8/32 | |||
| @@ -602,6 +602,30 @@ def is_linux(): | |||
| return False | |||
|
|
|||
|
|
|||
| def iproute2_is_json_capable(): | |||
There was a problem hiding this comment.
Can we put this into a separate commit?
tests/topotests/lib/common_check.py
Outdated
| @@ -0,0 +1,43 @@ | |||
| # SPDX-License-Identifier: ISC | |||
| # | |||
| # Copyright (c) 2019 by VMware, Inc. ("VMware") | |||
There was a problem hiding this comment.
replaced with 6w..
tests/topotests/lib/common_check.py
Outdated
| else: | ||
| output = json.loads(router.run(f"ip -json route show {ipaddr_str}")) | ||
| if output is None: | ||
| return "problem. iproute2 returns nothing" |
There was a problem hiding this comment.
I added some more checks. this said, if you have a correct json, then json_cmp will do the job.
There was a problem hiding this comment.
My point was to return always a valid JSON (for json_cmp() to compare properly) rather than (None, string, JSON-string)...
There was a problem hiding this comment.
I handled the exception, it should be betteR.
| ) | ||
| if rname in ("r1", "r3", "r5", "r6"): | ||
| router.load_config( | ||
| TopoRouter.RD_BGP, os.path.join(CWD, "{}/bgpd.conf".format(rname)) |
There was a problem hiding this comment.
I encourage using "frr.conf" (single file) for upcoming tests...
There was a problem hiding this comment.
thanks for encouragement. I will do it at next test I start from scratch.
| ip_check_path_selection, tgen.gears["r1"], prefix, expected | ||
| ) | ||
| _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5) | ||
| assert result is None, f"Failed to check that {prefix} uses the IGP label 16055" |
There was a problem hiding this comment.
But it can be the label 16006 also, correct?
There was a problem hiding this comment.
right. fixed comment
|
|
||
| def test_bgp_ipv4_convergence(): | ||
| """ | ||
| Check that R1 has received the 192.0.2.9/32 prefix from R5, and R8 |
There was a problem hiding this comment.
replaced r8 with r6
|
|
||
| tgen.gears["r1"].vtysh_cmd( | ||
| """ | ||
| configure terminal\n |
| """, | ||
| isjson=False, | ||
| ) | ||
| tgen.gears["r1"].vtysh_cmd("clear bgp ipv4 *\n") |
pguibert6WIND
force-pushed
the
bgp_recursive_duplicate
branch
2 times, most recently
from
e47fdda to
c912d61
Compare
Under a setup where two BGP prefixes are available from multiple sources, if one of the two prefixes is recursive over the other BGP prefix, then it will not be considered as multipath. The below output shows the two prefixes 192.0.2.24/32 and 192.0.2.21/32. The 192.0.2.[5,6,8] are the known IP addresses visible from the IGP. > # show bgp ipv4 192.0.2.24/32 > *>i 192.0.2.24/32 192.0.2.21 0 100 0 i > * i 192.0.2.21 0 100 0 i > * i 192.0.2.21 0 100 0 i > # show bgp ipv4 192.0.2.21/32 > *>i 192.0.2.21/32 192.0.2.5 0 100 0 i > *=i 192.0.2.6 0 100 0 i > *=i 192.0.2.8 0 100 0 i The bgp best selection algorithm refuses to consider the paths to '192.0.2.24/32' as multipath, whereas the BGP paths which use the BGP peer as nexthop are considered multipath. > ... has the same nexthop as the bestpath, skip it ... Previously, this condition has been added to prevent ZEBRA from installing routes with same nexthop: > Here you can see the two paths with nexthop 210.2.2.2 > superm-redxp-05# show ip route 2.23.24.192/28 > Routing entry for 2.23.24.192/28 > Known via "bgp", distance 20, metric 0, best > Last update 00:32:12 ago > * 210.2.2.2, via swp3 > * 210.2.0.2, via swp1 > * 210.2.1.2, via swp2 > * 210.2.2.2, via swp3 > [..] But today, ZEBRA knows how to handle it. When receiving incoming routes, nexthop groups are used. At creation, duplicated nexthops are identified, and will not be installed. The below output illustrate the duplicate paths to 172.16.0.200 received by an other peer. > r1# show ip route 172.18.1.100 nexthop-group > Routing entry for 172.18.1.100/32 > Known via "bgp", distance 200, metric 0, best > Last update 00:03:03 ago > Nexthop Group ID: 75757580 > 172.16.0.200 (recursive), weight 1 > * 172.31.0.3, via r1-eth1, label 16055, weight 1 > * 172.31.2.4, via r1-eth2, label 16055, weight 1 > * 172.31.0.3, via r1-eth1, label 16006, weight 1 > * 172.31.2.4, via r1-eth2, label 16006, weight 1 > * 172.31.8.7, via r1-eth4, label 16008, weight 1 > 172.16.0.200 (duplicate nexthop removed) (recursive), weight 1 > 172.31.0.3, via r1-eth1 (duplicate nexthop removed), label 16055, weight 1 > 172.31.2.4, via r1-eth2 (duplicate nexthop removed), label 16055, weight 1 > 172.31.0.3, via r1-eth1 (duplicate nexthop removed), label 16006, weight 1 > 172.31.2.4, via r1-eth2 (duplicate nexthop removed), label 16006, weight 1 > 172.31.8.7, via r1-eth4 (duplicate nexthop removed), label 16008, weight 1 Fix this by proposing to let ZEBRA handle this duplicate decision. Fixes: 7dc9d4e ("bgp may add multiple path entries with the same nexthop") Signed-off-by: Philippe Guibert <[email protected]>
Some tests may want to use the json facility of iproute2 to dump some results. Add an internal API in lib/topotest.py that tells whether iproute2 is json capable or not. Signed-off-by: Philippe Guibert <[email protected]>
Add a topotest that ensures that when addpath is enabled and two paths with same nexthop are received, they are sent to ZEBRA which detects 'duplicate nexthop'. Signed-off-by: Philippe Guibert <[email protected]>
pguibert6WIND
force-pushed
the
bgp_recursive_duplicate
branch
from
c912d61 to
d0bac27
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Under a setup where two BGP prefixes are available from multiple sources, if one of the two prefixes is recursive over the other BGP prefix, then it will not be considered as multipath. The below output shows the two prefixes 192.0.2.24/32 and 192.0.2.21/32. The 192.0.2.[5,6,8] are the known IP addresses visible from the IGP.
The bgp best selection algorithm refuses to consider the paths to '192.0.2.24/32' as multipath, whereas the BGP paths which use the BGP peer as nexthop are considered multipath.
Previously, this condition has been added to prevent ZEBRA from installing routes with same nexthop:
But today, ZEBRA knows how to handle it. When receiving incoming routes, nexthop groups are used. At creation, duplicated nexthops are identified, and will not be installed. The below output illustrate the duplicate paths to 172.16.0.200 received by an other peer.
Fix this by proposing to let ZEBRA handle this duplicate decision.
Fixes: 7dc9d4e ("bgp may add multiple path entries with the same nexthop")