From 5f0c5ec85d1b7e03b792211fab15eff8288971f2 Mon Sep 17 00:00:00 2001 From: vivek Date: Mon, 25 May 2020 14:06:10 -0700 Subject: [PATCH 1/4] bgpd: Minor tweaks to EVPN route-import debugs Signed-off-by: Vivek Venkatraman --- bgpd/bgp_evpn.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c index a10686189d..b82f00e862 100644 --- a/bgpd/bgp_evpn.c +++ b/bgpd/bgp_evpn.c @@ -2634,17 +2634,17 @@ static int install_evpn_route_entry_in_vrf(struct bgp *bgp_vrf, afi_t afi = 0; safi_t safi = 0; char buf[PREFIX_STRLEN]; - char buf1[PREFIX_STRLEN]; + bool new_pi = false; memset(pp, 0, sizeof(struct prefix)); ip_prefix_from_evpn_prefix(evp, pp); if (bgp_debug_zebra(NULL)) { zlog_debug( - "import evpn prefix %s as ip prefix %s in vrf %s", + "vrf %s: import evpn prefix %s parent %p flags 0x%x", + vrf_id_to_name(bgp_vrf->vrf_id), prefix2str(evp, buf, sizeof(buf)), - prefix2str(pp, buf1, sizeof(buf)), - vrf_id_to_name(bgp_vrf->vrf_id)); + parent_pi, parent_pi->flags); } /* Create (or fetch) route within the VRF. */ @@ -2678,9 +2678,10 @@ static int install_evpn_route_entry_in_vrf(struct bgp *bgp_vrf, && (struct bgp_path_info *)pi->extra->parent == parent_pi) break; - if (!pi) + if (!pi) { pi = bgp_create_evpn_bgp_path_info(parent_pi, rn, &attr); - else { + new_pi = true; + } else { if (attrhash_cmp(pi->attr, &attr) && !CHECK_FLAG(pi->flags, BGP_PATH_REMOVED)) { bgp_unlock_node(rn); @@ -2722,6 +2723,12 @@ static int install_evpn_route_entry_in_vrf(struct bgp *bgp_vrf, bgp_unlock_node(rn); + if (bgp_debug_zebra(NULL)) + zlog_debug( + "... %s pi rn %p (l %d) pi %p (l %d, f 0x%x)", + new_pi ? "new" : "update", + rn, rn->lock, pi, pi->lock, pi->flags); + return ret; } @@ -2839,17 +2846,16 @@ static int uninstall_evpn_route_entry_in_vrf(struct bgp *bgp_vrf, afi_t afi = 0; safi_t safi = 0; char buf[PREFIX_STRLEN]; - char buf1[PREFIX_STRLEN]; memset(pp, 0, sizeof(struct prefix)); ip_prefix_from_evpn_prefix(evp, pp); if (bgp_debug_zebra(NULL)) { zlog_debug( - "uninstalling evpn prefix %s as ip prefix %s in vrf %s", + "vrf %s: unimport evpn prefix %s parent %p flags 0x%x", + vrf_id_to_name(bgp_vrf->vrf_id), prefix2str(evp, buf, sizeof(buf)), - prefix2str(pp, buf1, sizeof(buf)), - vrf_id_to_name(bgp_vrf->vrf_id)); + parent_pi, parent_pi->flags); } /* Locate route within the VRF. */ @@ -2876,6 +2882,11 @@ static int uninstall_evpn_route_entry_in_vrf(struct bgp *bgp_vrf, if (!pi) return 0; + if (bgp_debug_zebra(NULL)) + zlog_debug( + "... delete rn %p (l %d) pi %p (l %d, f 0x%x)", + rn, rn->lock, pi, pi->lock, pi->flags); + /* Process for route leaking. */ vpn_leak_from_vrf_withdraw(bgp_get_default(), bgp_vrf, pi); From 9e15d76adff8c49e43985ad1496fe11d84e5b60c Mon Sep 17 00:00:00 2001 From: vivek Date: Mon, 25 May 2020 14:10:12 -0700 Subject: [PATCH 2/4] bgpd: Enhance NHT path evaluation debugs Signed-off-by: Vivek Venkatraman --- bgpd/bgp_nht.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c index fced2fbcab..cbc58b4632 100644 --- a/bgpd/bgp_nht.c +++ b/bgpd/bgp_nht.c @@ -44,6 +44,7 @@ #include "bgpd/bgp_zebra.h" #include "bgpd/bgp_flowspec_util.h" #include "bgpd/bgp_evpn.h" +#include "bgpd/bgp_rd.h" extern struct zclient *zclient; @@ -700,7 +701,7 @@ static void evaluate_paths(struct bgp_nexthop_cache *bnc) char buf[PREFIX2STR_BUFFER]; bnc_str(bnc, buf, PREFIX2STR_BUFFER); zlog_debug( - "NH update for %s(%s) - flags 0x%x chgflags 0x%x - evaluate paths", + "NH update for %s %s flags 0x%x chgflags 0x%x - evaluate paths", buf, bnc->bgp->name_pretty, bnc->flags, bnc->change_flags); } @@ -757,11 +758,22 @@ static void evaluate_paths(struct bgp_nexthop_cache *bnc) bgp_isvalid_nexthop(bnc) ? 1 : 0; } - if (BGP_DEBUG(nht, NHT)) - zlog_debug("%s: prefix %pRN (vrf %s) %svalid", __func__, - rn, bgp_path->name, - (bnc_is_valid_nexthop ? "" : "not ")); + if (BGP_DEBUG(nht, NHT)) { + char buf1[RD_ADDRSTRLEN]; + if (rn->prn) { + prefix_rd2str((struct prefix_rd *)&rn->prn->p, + buf1, sizeof(buf1)); + zlog_debug( + "... eval path %d/%d %pRN RD %s %s flags 0x%x", + afi, safi, rn, buf1, + bgp_path->name_pretty, path->flags); + } else + zlog_debug( + "... eval path %d/%d %pRN %s flags 0x%x", + afi, safi, rn, bgp_path->name_pretty, + path->flags); + } if ((CHECK_FLAG(path->flags, BGP_PATH_VALID) ? 1 : 0) != bnc_is_valid_nexthop) { if (CHECK_FLAG(path->flags, BGP_PATH_VALID)) { From 34ea39b65a2d6dd48df15d9d8a929c1a6f128122 Mon Sep 17 00:00:00 2001 From: vivek Date: Mon, 25 May 2020 14:15:37 -0700 Subject: [PATCH 3/4] bgpd: Check NHT change for triggering EVPN import or unimport Ensure that only if there is a change to the path's validity based on the NHT update, EVPN import or unimport is invoked. Signed-off-by: Vivek Venkatraman --- bgpd/bgp_nht.c | 55 ++++++++++++++++++++++++++++---------------------- 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c index cbc58b4632..78911c82f2 100644 --- a/bgpd/bgp_nht.c +++ b/bgpd/bgp_nht.c @@ -736,7 +736,8 @@ static void evaluate_paths(struct bgp_nexthop_cache *bnc) * nexthops with labels */ - int bnc_is_valid_nexthop = 0; + bool bnc_is_valid_nexthop = false; + bool path_valid = false; if (safi == SAFI_UNICAST && path->sub_type == BGP_ROUTE_IMPORTED && @@ -744,7 +745,7 @@ static void evaluate_paths(struct bgp_nexthop_cache *bnc) path->extra->num_labels) { bnc_is_valid_nexthop = - bgp_isvalid_labeled_nexthop(bnc) ? 1 : 0; + bgp_isvalid_labeled_nexthop(bnc) ? true : false; } else { if (bgp_update_martian_nexthop( bnc->bgp, afi, safi, path->type, @@ -755,7 +756,7 @@ static void evaluate_paths(struct bgp_nexthop_cache *bnc) __func__, rn, bgp_path->name); } else bnc_is_valid_nexthop = - bgp_isvalid_nexthop(bnc) ? 1 : 0; + bgp_isvalid_nexthop(bnc) ? true : false; } if (BGP_DEBUG(nht, NHT)) { @@ -774,20 +775,6 @@ static void evaluate_paths(struct bgp_nexthop_cache *bnc) afi, safi, rn, bgp_path->name_pretty, path->flags); } - if ((CHECK_FLAG(path->flags, BGP_PATH_VALID) ? 1 : 0) - != bnc_is_valid_nexthop) { - if (CHECK_FLAG(path->flags, BGP_PATH_VALID)) { - bgp_aggregate_decrement(bgp_path, p, path, afi, - safi); - bgp_path_info_unset_flag(rn, path, - BGP_PATH_VALID); - } else { - bgp_path_info_set_flag(rn, path, - BGP_PATH_VALID); - bgp_aggregate_increment(bgp_path, p, path, afi, - safi); - } - } /* Copy the metric to the path. Will be used for bestpath * computation */ @@ -801,13 +788,33 @@ static void evaluate_paths(struct bgp_nexthop_cache *bnc) || CHECK_FLAG(bnc->change_flags, BGP_NEXTHOP_CHANGED)) SET_FLAG(path->flags, BGP_PATH_IGP_CHANGED); - if (safi == SAFI_EVPN && bgp_evpn_is_prefix_nht_supported(p)) { - if (CHECK_FLAG(path->flags, BGP_PATH_VALID)) - bgp_evpn_import_route(bgp_path, afi, safi, p, - path); - else - bgp_evpn_unimport_route(bgp_path, afi, safi, p, - path); + path_valid = !!CHECK_FLAG(path->flags, BGP_PATH_VALID); + if (path_valid != bnc_is_valid_nexthop) { + if (path_valid) { + /* No longer valid, clear flag; also for EVPN + * routes, unimport from VRFs if needed. + */ + bgp_aggregate_decrement(bgp_path, p, path, afi, + safi); + bgp_path_info_unset_flag(rn, path, + BGP_PATH_VALID); + if (safi == SAFI_EVPN && + bgp_evpn_is_prefix_nht_supported(&rn->p)) + bgp_evpn_unimport_route(bgp_path, + afi, safi, &rn->p, path); + } else { + /* Path becomes valid, set flag; also for EVPN + * routes, import from VRFs if needed. + */ + bgp_path_info_set_flag(rn, path, + BGP_PATH_VALID); + bgp_aggregate_increment(bgp_path, p, path, afi, + safi); + if (safi == SAFI_EVPN && + bgp_evpn_is_prefix_nht_supported(&rn->p)) + bgp_evpn_import_route(bgp_path, + afi, safi, &rn->p, path); + } } bgp_process(bgp_path, rn, afi, safi); From 0139efe0848346c8f8f4281f24f065cdce882504 Mon Sep 17 00:00:00 2001 From: vivek Date: Mon, 25 May 2020 14:17:12 -0700 Subject: [PATCH 4/4] bgpd: During NHT change evaluation, skip inappropriate paths When there is a NHT change and the paths dependent on that NHT are being evaluated, skip those that are marked for removal or as history. When a route gets withdrawn, its valid flag is cleared and it is flagged for removal; in the case of an EVPN route, it is also unimported from VRFs (L2 and/or L3). bgp_process is then scheduled. Under rare timing conditions, an NHT update for the route's next hop may arrive right after, and if routes flagged for removal are not skipped, they may not only be incorrectly marked as valid but also re-imported in the case of EVPN, which will be a serious error. Signed-off-by: Vivek Venkatraman --- bgpd/bgp_nht.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c index 78911c82f2..e3b05e8b79 100644 --- a/bgpd/bgp_nht.c +++ b/bgpd/bgp_nht.c @@ -776,6 +776,11 @@ static void evaluate_paths(struct bgp_nexthop_cache *bnc) path->flags); } + /* Skip paths marked for removal or as history. */ + if (CHECK_FLAG(path->flags, BGP_PATH_REMOVED) + || CHECK_FLAG(path->flags, BGP_PATH_HISTORY)) + continue; + /* Copy the metric to the path. Will be used for bestpath * computation */ if (bgp_isvalid_nexthop(bnc) && bnc->metric)