diff --git a/tests/topotests/bgp_default_originate/test_default_orginate_vrf.py b/tests/topotests/bgp_default_originate/test_default_orginate_vrf.py index 1506b02e5d..905c3e2b66 100644 --- a/tests/topotests/bgp_default_originate/test_default_orginate_vrf.py +++ b/tests/topotests/bgp_default_originate/test_default_orginate_vrf.py @@ -546,13 +546,7 @@ def test_verify_default_originate_route_with_non_default_VRF_p1(request): tc_name, result ) - result = verify_rib( - tgen, - addr_type, - "r2", - static_routes_input, - next_hop=DEFAULT_ROUTE_NXT_HOP_R1[addr_type], - ) + result = verify_rib(tgen, addr_type, "r2", static_routes_input) assert result is True, "Testcase {} : Failed \n Error: {}".format( tc_name, result ) diff --git a/tests/topotests/lib/common_config.py b/tests/topotests/lib/common_config.py index cb71112af3..f34c48b890 100644 --- a/tests/topotests/lib/common_config.py +++ b/tests/topotests/lib/common_config.py @@ -3371,7 +3371,7 @@ def verify_rib( found_hops = [ rib_r["ip"] for rib_r in rib_routes_json[st_rt][0]["nexthops"] - if "ip" in rib_r + if "ip" in rib_r and "active" in rib_r ] # If somehow key "ip" is not found in nexthops JSON diff --git a/zebra/rib.h b/zebra/rib.h index 4293b5f240..071cc7b3de 100644 --- a/zebra/rib.h +++ b/zebra/rib.h @@ -637,6 +637,10 @@ extern pid_t pid; extern uint32_t rt_table_main_id; +void route_entry_dump_nh(const struct route_entry *re, const char *straddr, + const struct vrf *re_vrf, + const struct nexthop *nexthop); + /* Name of hook calls */ #define ZEBRA_ON_RIB_PROCESS_HOOK_CALL "on_rib_process_dplane_results" diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 4ee9dc5fcf..81f1411ee5 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -1101,11 +1101,15 @@ void zebra_nhg_check_valid(struct nhg_hash_entry *nhe) bool valid = false; /* - * If I have other nhe's depending on me, then this is a + * If I have other nhe's depending on me, or I have nothing + * I am depending on then this is a * singleton nhe so set this nexthops flag as appropriate. */ - if (nhg_connected_tree_count(&nhe->nhg_depends)) + if (nhg_connected_tree_count(&nhe->nhg_depends) || + nhg_connected_tree_count(&nhe->nhg_dependents) == 0) { + UNSET_FLAG(nhe->nhg.nexthop->flags, NEXTHOP_FLAG_FIB); UNSET_FLAG(nhe->nhg.nexthop->flags, NEXTHOP_FLAG_ACTIVE); + } /* If anthing else in the group is valid, the group is valid */ frr_each(nhg_connected_tree, &nhe->nhg_depends, rb_node_dep) { @@ -2921,6 +2925,154 @@ static uint32_t proto_nhg_nexthop_active_update(struct nexthop_group *nhg) return curr_active; } +/* + * This function takes the start of two comparable nexthops from two different + * nexthop groups and walks them to see if they can be considered the same + * or not. This is being used to determine if zebra should reuse a nhg + * from the old_re to the new_re, when an interface goes down and the + * new nhg sent down from the upper level protocol would resolve to it + */ +static bool zebra_nhg_nexthop_compare(const struct nexthop *nhop, + const struct nexthop *old_nhop, + const struct route_node *rn) +{ + bool same = true; + + while (nhop && old_nhop) { + if (IS_ZEBRA_DEBUG_NHG_DETAIL) + zlog_debug("%s: %pRN Comparing %pNHvv(%u) to old: %pNHvv(%u)", + __func__, rn, nhop, nhop->flags, old_nhop, + old_nhop->flags); + if (!CHECK_FLAG(old_nhop->flags, NEXTHOP_FLAG_ACTIVE)) { + if (IS_ZEBRA_DEBUG_NHG_DETAIL) + zlog_debug("%s: %pRN Old is not active going to the next one", + __func__, rn); + old_nhop = old_nhop->next; + continue; + } + + if (nexthop_same(nhop, old_nhop)) { + struct nexthop *new_recursive, *old_recursive; + + if (IS_ZEBRA_DEBUG_NHG_DETAIL) + zlog_debug("%s: %pRN New and old are same, continuing search", + __func__, rn); + + new_recursive = nhop->resolved; + old_recursive = old_nhop->resolved; + + while (new_recursive && old_recursive) { + if (!nexthop_same(new_recursive, old_recursive)) { + same = false; + break; + } + + new_recursive = new_recursive->next; + old_recursive = old_recursive->next; + } + + if (new_recursive) + same = false; + else if (old_recursive) { + while (old_recursive) { + if (CHECK_FLAG(old_recursive->flags, + NEXTHOP_FLAG_ACTIVE)) + break; + old_recursive = old_recursive->next; + } + + if (old_recursive) + same = false; + } + + if (!same) + break; + + nhop = nhop->next; + old_nhop = old_nhop->next; + continue; + } else { + if (IS_ZEBRA_DEBUG_NHG_DETAIL) + zlog_debug("%s:%pRN They are not the same, stopping using new nexthop entry", + __func__, rn); + same = false; + break; + } + } + + if (nhop) + same = false; + else if (old_nhop) { + while (old_nhop) { + if (CHECK_FLAG(old_nhop->flags, NEXTHOP_FLAG_ACTIVE)) + break; + old_nhop = old_nhop->next; + } + + if (old_nhop) + same = false; + } + + return same; +} + +static struct nhg_hash_entry *zebra_nhg_rib_compare_old_nhe( + const struct route_node *rn, const struct route_entry *re, + struct nhg_hash_entry *new_nhe, struct nhg_hash_entry *old_nhe) +{ + struct nexthop *nhop, *old_nhop; + bool same = true; + struct vrf *vrf = vrf_lookup_by_id(re->vrf_id); + + if (IS_ZEBRA_DEBUG_NHG_DETAIL) { + char straddr[PREFIX_STRLEN]; + + prefix2str(&rn->p, straddr, sizeof(straddr)); + zlog_debug("%s: %pRN new id: %u old id: %u", __func__, rn, + new_nhe->id, old_nhe->id); + zlog_debug("%s: %pRN NEW", __func__, rn); + for (ALL_NEXTHOPS(new_nhe->nhg, nhop)) + route_entry_dump_nh(re, straddr, vrf, nhop); + + zlog_debug("%s: %pRN OLD", __func__, rn); + for (ALL_NEXTHOPS(old_nhe->nhg, nhop)) + route_entry_dump_nh(re, straddr, vrf, nhop); + } + + nhop = new_nhe->nhg.nexthop; + old_nhop = old_nhe->nhg.nexthop; + + same = zebra_nhg_nexthop_compare(nhop, old_nhop, rn); + + if (same) { + struct nexthop_group *bnhg, *old_bnhg; + + bnhg = zebra_nhg_get_backup_nhg(new_nhe); + old_bnhg = zebra_nhg_get_backup_nhg(old_nhe); + + if (bnhg || old_bnhg) { + if (bnhg && !old_bnhg) + same = false; + else if (!bnhg && old_bnhg) + same = false; + else + same = zebra_nhg_nexthop_compare(bnhg->nexthop, + old_bnhg->nexthop, + rn); + } + } + + if (IS_ZEBRA_DEBUG_NHG_DETAIL) + zlog_debug("%s:%pRN They are %sthe same, using the %s nhg entry", + __func__, rn, same ? "" : "not ", + same ? "old" : "new"); + + if (same) + return old_nhe; + else + return new_nhe; +} + /* * Iterate over all nexthops of the given RIB entry and refresh their * ACTIVE flag. If any nexthop is found to toggle the ACTIVE flag, @@ -2928,7 +3080,8 @@ static uint32_t proto_nhg_nexthop_active_update(struct nexthop_group *nhg) * * Return value is the new number of active nexthops. */ -int nexthop_active_update(struct route_node *rn, struct route_entry *re) +int nexthop_active_update(struct route_node *rn, struct route_entry *re, + struct route_entry *old_re) { struct nhg_hash_entry *curr_nhe; uint32_t curr_active = 0, backup_active = 0; @@ -2984,6 +3137,11 @@ backups_done: new_nhe = zebra_nhg_rib_find_nhe(curr_nhe, rt_afi); + if (old_re && old_re->type == re->type && + old_re->instance == re->instance) + new_nhe = zebra_nhg_rib_compare_old_nhe(rn, re, new_nhe, + old_re->nhe); + if (IS_ZEBRA_DEBUG_NHG_DETAIL) zlog_debug( "%s: re %p CHANGED: nhe %p (%pNG) => new_nhe %p (%pNG)", @@ -3779,6 +3937,17 @@ void zebra_interface_nhg_reinstall(struct interface *ifp) frr_each_safe (nhg_connected_tree, &rb_node_dep->nhe->nhg_dependents, rb_node_dependent) { + struct nexthop *nhop_dependent = + rb_node_dependent->nhe->nhg.nexthop; + + while (nhop_dependent && + !nexthop_same(nhop_dependent, nh)) + nhop_dependent = nhop_dependent->next; + + if (nhop_dependent) + SET_FLAG(nhop_dependent->flags, + NEXTHOP_FLAG_ACTIVE); + if (IS_ZEBRA_DEBUG_NHG) zlog_debug("%s dependent nhe %pNG Setting Reinstall flag", __func__, diff --git a/zebra/zebra_nhg.h b/zebra/zebra_nhg.h index 712c1057a1..435ccb0d01 100644 --- a/zebra/zebra_nhg.h +++ b/zebra/zebra_nhg.h @@ -404,7 +404,8 @@ extern void zebra_nhg_mark_keep(void); /* Nexthop resolution processing */ struct route_entry; /* Forward ref to avoid circular includes */ -extern int nexthop_active_update(struct route_node *rn, struct route_entry *re); +extern int nexthop_active_update(struct route_node *rn, struct route_entry *re, + struct route_entry *old_re); #ifdef _FRR_ATTRIBUTE_PRINTFRR #pragma FRR printfrr_ext "%pNG" (const struct nhg_hash_entry *) diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c index 402a3104b9..721eca70a4 100644 --- a/zebra/zebra_rib.c +++ b/zebra/zebra_rib.c @@ -1313,7 +1313,7 @@ static void rib_process(struct route_node *rn) */ if (CHECK_FLAG(re->status, ROUTE_ENTRY_CHANGED)) { proto_re_changed = re; - if (!nexthop_active_update(rn, re)) { + if (!nexthop_active_update(rn, re, old_fib)) { const struct prefix *p; struct rib_table_info *info; @@ -4118,9 +4118,8 @@ void rib_delnode(struct route_node *rn, struct route_entry *re) /* * Helper that debugs a single nexthop within a route-entry */ -static void _route_entry_dump_nh(const struct route_entry *re, - const char *straddr, const struct vrf *re_vrf, - const struct nexthop *nexthop) +void route_entry_dump_nh(const struct route_entry *re, const char *straddr, + const struct vrf *re_vrf, const struct nexthop *nexthop) { char nhname[PREFIX_STRLEN]; char backup_str[50]; @@ -4243,7 +4242,7 @@ void _route_entry_dump(const char *func, union prefixconstptr pp, /* Dump nexthops */ for (ALL_NEXTHOPS(re->nhe->nhg, nexthop)) - _route_entry_dump_nh(re, straddr, vrf, nexthop); + route_entry_dump_nh(re, straddr, vrf, nexthop); if (zebra_nhg_get_backup_nhg(re->nhe)) { zlog_debug("%s(%s): backup nexthops:", straddr, @@ -4251,7 +4250,7 @@ void _route_entry_dump(const char *func, union prefixconstptr pp, nhg = zebra_nhg_get_backup_nhg(re->nhe); for (ALL_NEXTHOPS_PTR(nhg, nexthop)) - _route_entry_dump_nh(re, straddr, vrf, nexthop); + route_entry_dump_nh(re, straddr, vrf, nexthop); } zlog_debug("%s(%s): dump complete", straddr, VRF_LOGNAME(vrf));