Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bgpd: fix do not skip paths with same nexthop #16153

Merged
merged 3 commits into from
Jun 18, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 71 additions & 44 deletions bgpd/bgp_mpath.c
Original file line number Diff line number Diff line change
@@ -605,31 +605,43 @@ void bgp_path_info_mpath_update(struct bgp *bgp, struct bgp_dest *dest,
if (mp_node && (listgetdata(mp_node) == cur_mpath)) {
list_delete_node(mp_list, mp_node);
bgp_path_info_mpath_dequeue(cur_mpath);
if ((mpath_count < maxpaths)
&& prev_mpath
&& bgp_path_info_nexthop_cmp(prev_mpath,
cur_mpath)) {
if ((mpath_count < maxpaths) && prev_mpath) {
mpath_count++;
if (bgp_path_info_nexthop_cmp(prev_mpath,
cur_mpath)) {
if (ecommunity_linkbw_present(
bgp_attr_get_ecommunity(
cur_mpath->attr),
&bwval) ||
ecommunity_linkbw_present(
bgp_attr_get_ipv6_ecommunity(
cur_mpath->attr),
&bwval))
cum_bw += bwval;
else
all_paths_lb = false;
if (debug) {
bgp_path_info_path_with_addpath_rx_str(
cur_mpath, path_buf,
sizeof(path_buf));
zlog_debug("%pBD: %s is still multipath, cur count %d",
dest, path_buf,
mpath_count);
}
} else {
if (debug) {
bgp_path_info_path_with_addpath_rx_str(
cur_mpath, path_buf,
sizeof(path_buf));
zlog_debug("%pBD: nexthop equal, however add mpath %s nexthop %pI4, cur count %d",
dest, path_buf,
&cur_mpath->attr->nexthop,
mpath_count);
}
}
bgp_path_info_mpath_enqueue(prev_mpath,
cur_mpath);
prev_mpath = cur_mpath;
mpath_count++;
if (ecommunity_linkbw_present(bgp_attr_get_ecommunity(
cur_mpath->attr),
&bwval) ||
ecommunity_linkbw_present(
bgp_attr_get_ipv6_ecommunity(
cur_mpath->attr),
&bwval))
cum_bw += bwval;
else
all_paths_lb = false;
if (debug) {
bgp_path_info_path_with_addpath_rx_str(
cur_mpath, path_buf,
sizeof(path_buf));
zlog_debug("%pBD: %s is still multipath, cur count %d",
dest, path_buf, mpath_count);
}
} else {
mpath_changed = 1;
if (debug) {
@@ -693,35 +705,50 @@ void bgp_path_info_mpath_update(struct bgp *bgp, struct bgp_dest *dest,
list_delete_node(mp_list, mp_node);
assert(new_mpath);
assert(prev_mpath);
if ((mpath_count < maxpaths) && (new_mpath != new_best)
&& bgp_path_info_nexthop_cmp(prev_mpath,
new_mpath)) {
if ((mpath_count < maxpaths) && (new_mpath != new_best)) {
/* keep duplicate nexthop */
bgp_path_info_mpath_dequeue(new_mpath);

bgp_path_info_mpath_enqueue(prev_mpath,
new_mpath);
prev_mpath = new_mpath;
mpath_changed = 1;
mpath_count++;
if (ecommunity_linkbw_present(bgp_attr_get_ecommunity(
new_mpath->attr),
&bwval) ||
ecommunity_linkbw_present(
bgp_attr_get_ipv6_ecommunity(
new_mpath->attr),
&bwval))
cum_bw += bwval;
else
all_paths_lb = false;
if (debug) {
bgp_path_info_path_with_addpath_rx_str(
new_mpath, path_buf,
sizeof(path_buf));
zlog_debug("%pBD: add mpath %s nexthop %pI4, cur count %d",
dest, path_buf,
&new_mpath->attr->nexthop,
mpath_count);
if (bgp_path_info_nexthop_cmp(prev_mpath,
new_mpath)) {
if (ecommunity_linkbw_present(
bgp_attr_get_ecommunity(
new_mpath->attr),
&bwval) ||
ecommunity_linkbw_present(
bgp_attr_get_ipv6_ecommunity(
new_mpath->attr),
&bwval))
cum_bw += bwval;
else
all_paths_lb = false;
if (debug) {
bgp_path_info_path_with_addpath_rx_str(
new_mpath, path_buf,
sizeof(path_buf));
zlog_debug("%pBD: add mpath %s nexthop %pI4, cur count %d",
dest, path_buf,
&new_mpath->attr
->nexthop,
mpath_count);
}
} else {
if (debug) {
bgp_path_info_path_with_addpath_rx_str(
new_mpath, path_buf,
sizeof(path_buf));
zlog_debug("%pBD: nexthop equal, however add mpath %s nexthop %pI4, cur count %d",
dest, path_buf,
&new_mpath->attr
->nexthop,
mpath_count);
}
}
prev_mpath = new_mpath;
}
mp_node = mp_next_node;
}
9 changes: 0 additions & 9 deletions bgpd/bgp_route.c
Original file line number Diff line number Diff line change
@@ -3256,15 +3256,6 @@ void bgp_best_selection(struct bgp *bgp, struct bgp_dest *dest,
if (!peer_established(pi->peer->connection))
continue;

if (!bgp_path_info_nexthop_cmp(pi, new_select)) {
if (debug)
zlog_debug(
"%pBD(%s): %s has the same nexthop as the bestpath, skip it",
dest, bgp->name_pretty,
path_buf);
continue;
}

bgp_path_info_cmp(bgp, pi, new_select, &paths_eq,
mpath_cfg, debug, pfx_buf, afi, safi,
&dest->reason);
13 changes: 13 additions & 0 deletions tests/topotests/bgp_duplicate_nexthop/r1/bgpd.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
router bgp 64500
bgp router-id 192.0.2.1
no bgp ebgp-requires-policy
neighbor rrserver peer-group
neighbor rrserver remote-as 64500
neighbor rrserver update-source lo
neighbor rrserver timers connect 2
neighbor 192.0.2.3 peer-group rrserver
address-family ipv4 unicast
neighbor rrserver next-hop-self
neighbor rrserver activate
exit-address-family
!
26 changes: 26 additions & 0 deletions tests/topotests/bgp_duplicate_nexthop/r1/isisd.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
hostname r1
interface lo
ip router isis 1
isis passive
!
interface r1-eth1
ip router isis 1
isis network point-to-point
!
interface r1-eth2
ip router isis 1
isis network point-to-point
!
interface r1-eth4
ip router isis 1
isis network point-to-point
!
router isis 1
net 49.0123.6452.0001.00
is-type level-2-only
mpls-te on
segment-routing on
segment-routing global-block 16000 17000
segment-routing node-msd 10
segment-routing prefix 192.0.2.1/32 index 1
!
24 changes: 24 additions & 0 deletions tests/topotests/bgp_duplicate_nexthop/r1/zebra.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
log stdout
interface lo
ip address 192.0.2.1/32
!
interface r1-eth0
ip address 172.31.10.1/24
!
interface r1-eth1
ip address 172.31.0.1/24
mpls enable
!
interface r1-eth2
ip address 172.31.2.1/24
mpls enable
!
interface r1-eth3
ip address 172.31.11.1/24
mpls enable
!
interface r1-eth4
ip address 172.31.8.1/24
mpls enable
!

16 changes: 16 additions & 0 deletions tests/topotests/bgp_duplicate_nexthop/r3/bgpd.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
router bgp 64500 view one
bgp router-id 192.0.2.3
neighbor rr peer-group
neighbor rr remote-as 64500
neighbor rr update-source lo
neighbor 192.0.2.1 peer-group rr
neighbor 192.0.2.5 peer-group rr
neighbor 192.0.2.6 peer-group rr
neighbor 192.0.2.8 peer-group rr
!
address-family ipv4 unicast
neighbor rr activate
neighbor rr route-reflector-client
neighbor rr addpath-tx-all-paths
exit-address-family
!
38 changes: 38 additions & 0 deletions tests/topotests/bgp_duplicate_nexthop/r3/isisd.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
hostname r3
interface lo
ip router isis 1
isis passive
!
interface r3-eth0
ip router isis 1
isis network point-to-point
!
interface r3-eth1
ip router isis 1
isis network point-to-point
!
interface r3-eth2
ip router isis 1
isis network point-to-point
!
interface r3-eth3
ip router isis 1
isis network point-to-point
!
interface r3-eth4
ip router isis 1
isis network point-to-point
!
interface r3-eth5
ip router isis 1
isis network point-to-point
!
router isis 1
net 49.0123.6452.0003.00
is-type level-2-only
mpls-te on
segment-routing on
segment-routing global-block 16000 17000
segment-routing node-msd 10
segment-routing prefix 192.0.2.3/32 index 3
!
16 changes: 16 additions & 0 deletions tests/topotests/bgp_duplicate_nexthop/r3/zebra.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
log stdout
interface lo
ip address 192.0.2.3/32
!
interface r3-eth0
ip address 172.31.0.3/24
mpls enable
!
interface r3-eth1
ip address 172.31.4.3/24
mpls enable
!
interface r3-eth2
ip address 172.31.5.3/24
mpls enable
!
30 changes: 30 additions & 0 deletions tests/topotests/bgp_duplicate_nexthop/r4/isisd.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
hostname r4
interface lo
ip router isis 1
isis passive
!
interface r4-eth0
ip router isis 1
isis network point-to-point
!
interface r4-eth1
ip router isis 1
isis network point-to-point
!
interface r4-eth2
ip router isis 1
isis network point-to-point
!
interface r4-eth3
ip router isis 1
isis network point-to-point
!
router isis 1
net 49.0123.6452.0004.00
is-type level-2-only
mpls-te on
segment-routing on
segment-routing global-block 16000 17000
segment-routing node-msd 10
segment-routing prefix 192.0.2.4/32 index 4
!
20 changes: 20 additions & 0 deletions tests/topotests/bgp_duplicate_nexthop/r4/zebra.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
log stdout
interface lo
ip address 192.0.2.4/32
!
interface r4-eth0
ip address 172.31.2.4/24
mpls enable
!
interface r4-eth1
ip address 172.31.6.4/24
mpls enable
!
interface r4-eth2
ip address 172.31.7.4/24
mpls enable
!
interface r4-eth3
mpls enable
!

19 changes: 19 additions & 0 deletions tests/topotests/bgp_duplicate_nexthop/r5/bgpd.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
router bgp 64500
bgp router-id 192.0.2.5
no bgp ebgp-requires-policy
no bgp network import-check
neighbor rrserver peer-group
neighbor rrserver remote-as 64500
neighbor rrserver update-source lo
neighbor rrserver timers connect 2
neighbor 192.0.2.3 peer-group rrserver
address-family ipv4 unicast
network 192.0.2.9/32
network 192.0.2.8/32 route-map rmap
neighbor rrserver activate
neighbor rrserver addpath-tx-all-paths
exit-address-family
!
route-map rmap permit 1
set ip next-hop 192.0.2.9
exit
26 changes: 26 additions & 0 deletions tests/topotests/bgp_duplicate_nexthop/r5/isisd.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
hostname r5
interface lo
ip router isis 1
isis passive
!
interface r5-eth1
ip router isis 1
isis network point-to-point
!
interface r5-eth2
ip router isis 1
isis network point-to-point
!
interface r5-eth3
ip router isis 1
isis network point-to-point
!
router isis 1
net 49.0123.6452.0005.00
is-type level-2-only
mpls-te on
segment-routing on
segment-routing global-block 16000 17000
segment-routing node-msd 10
segment-routing prefix 192.0.2.5/32 index 55
!
19 changes: 19 additions & 0 deletions tests/topotests/bgp_duplicate_nexthop/r5/zebra.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
log stdout
mpls label dynamic-block 5000 5999
interface lo
ip address 192.0.2.5/32
!
interface r5-eth0
ip address 172.31.12.5/24
!
interface r5-eth1
ip address 172.31.4.5/24
mpls enable
!
interface r5-eth2
ip address 172.31.7.5/24
mpls enable
!
interface r5-eth3
ip address 172.31.21.5/24
!
19 changes: 19 additions & 0 deletions tests/topotests/bgp_duplicate_nexthop/r6/bgpd.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
router bgp 64500
bgp router-id 192.0.2.6
no bgp ebgp-requires-policy
no bgp network import-check
neighbor rrserver peer-group
neighbor rrserver remote-as 64500
neighbor rrserver update-source lo
neighbor rrserver bfd
neighbor 192.0.2.3 peer-group rrserver
address-family ipv4 unicast
network 192.0.2.9/32
network 192.0.2.8/32 route-map rmap
neighbor rrserver activate
neighbor rrserver addpath-tx-all-paths
exit-address-family
!
route-map rmap permit 1
set ip next-hop 192.0.2.9
exit
22 changes: 22 additions & 0 deletions tests/topotests/bgp_duplicate_nexthop/r6/isisd.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
hostname r6
interface lo
ip router isis 1
isis passive
!
interface r6-eth1
ip router isis 1
isis network point-to-point
!
interface r6-eth2
ip router isis 1
isis network point-to-point
!
router isis 1
net 49.0123.6452.0006.00
is-type level-2-only
mpls-te on
segment-routing on
segment-routing global-block 16000 17000
segment-routing node-msd 10
segment-routing prefix 192.0.2.6/32 index 6
!
20 changes: 20 additions & 0 deletions tests/topotests/bgp_duplicate_nexthop/r6/zebra.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
log stdout
mpls label dynamic-block 6000 6999
interface lo
ip address 192.0.2.6/32
!
interface r6-eth0
ip address 172.31.13.6/24
!
interface r6-eth1
ip address 172.31.5.6/24
mpls enable
!
interface r6-eth2
ip address 172.31.6.6/24
mpls enable
!
interface r6-eth3
ip address 172.31.22.6/24
!

458 changes: 458 additions & 0 deletions tests/topotests/bgp_duplicate_nexthop/test_bgp_duplicate_nexthop.py

Large diffs are not rendered by default.

48 changes: 48 additions & 0 deletions tests/topotests/lib/common_check.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
#!/usr/bin/env python
# SPDX-License-Identifier: ISC
#
# common_check.py
#
# Copyright 2024 6WIND S.A.

#
import json
from lib import topotest


def ip_check_path_selection(router, ipaddr_str, expected, vrf_name=None):
if vrf_name:
cmdstr = f'show ip route vrf {vrf_name} {ipaddr_str} json'
else:
cmdstr = f'show ip route {ipaddr_str} json'
try:
output = json.loads(router.vtysh_cmd(cmdstr))
except:
output = {}

ret = topotest.json_cmp(output, expected)
if ret is 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:
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
)
return ret


def iproute2_check_path_selection(router, ipaddr_str, expected, vrf_name=None):
if not topotest.iproute2_is_json_capable():
return None

if vrf_name:
cmdstr = f'ip -json route show vrf {vrf_name} {ipaddr_str}'
else:
cmdstr = f'ip -json route show {ipaddr_str}'
try:
output = json.loads(cmdstr)
except:
output = []

return topotest.json_cmp(output, expected)
24 changes: 24 additions & 0 deletions tests/topotests/lib/topotest.py
Original file line number Diff line number Diff line change
@@ -602,6 +602,30 @@ def is_linux():
return False


def iproute2_is_json_capable():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we put this into a separate commit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

"""
Checks if the iproute2 version installed on the system is capable of
handling JSON outputss
Returns True if capability can be detected, returns False otherwise.
"""
if is_linux():
try:
subp = subprocess.Popen(
["ip", "-json", "route", "show"],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
stdin=subprocess.PIPE,
)
iproute2_err = subp.communicate()[1].splitlines()[0].split()[0]

if iproute2_err != "Error:":
return True
except Exception:
pass
return False


def iproute2_is_vrf_capable():
"""
Checks if the iproute2 version installed on the system is capable of