From 497ff5792f6a27702bf4ee33b79e1a80ffcf0a09 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Mon, 27 Jan 2020 19:36:01 -0500 Subject: [PATCH 1/6] zebra: handle NHG in NHG dataplane group conversion We were not properly handling the case of a NHG inside of another NHG when converting the rb tree of a multilevel NHG into a flat list of IDs. When constructing, we call the function zebra_nhg_nhe2grp_internal() recursively so that the rare case of a group within a group is handled such that its singleton nexthops are appended to the grp array of IDs we send to the dataplane code. Ex) 1: -> 2: -> 3 -> 4 ->5: ->6 becomes this: 1: ->3 ->4 ->6 when its sent to the dataplane code for final kernel installation. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 60 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 43 insertions(+), 17 deletions(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 7f229b3872..8e923d2d50 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -1834,17 +1834,34 @@ int nexthop_active_update(struct route_node *rn, struct route_entry *re) return curr_active; } -/* Convert a nhe into a group array */ -uint8_t zebra_nhg_nhe2grp(struct nh_grp *grp, struct nhg_hash_entry *nhe, - int max_num) +/* Recursively construct a grp array of fully resolved IDs. + * + * This function allows us to account for groups within groups, + * by converting them into a flat array of IDs. + * + * nh_grp is modified at every level of recursion to append + * to it the next unique, fully resolved ID from the entire tree. + * + * + * Note: + * I'm pretty sure we only allow ONE level of group within group currently. + * But making this recursive just in case that ever changes. + */ +static uint8_t zebra_nhg_nhe2grp_internal(struct nh_grp *grp, + uint8_t curr_index, + struct nhg_hash_entry *nhe, + int max_num) { struct nhg_connected *rb_node_dep = NULL; struct nhg_hash_entry *depend = NULL; - uint8_t i = 0; + uint8_t i = curr_index; frr_each(nhg_connected_tree, &nhe->nhg_depends, rb_node_dep) { bool duplicate = false; + if (i >= max_num) + goto done; + depend = rb_node_dep->nhe; /* @@ -1861,27 +1878,36 @@ uint8_t zebra_nhg_nhe2grp(struct nh_grp *grp, struct nhg_hash_entry *nhe, } } - /* Check for duplicate IDs, kernel doesn't like that */ - for (int j = 0; j < i; j++) { - if (depend->id == grp[j].id) - duplicate = true; - } + if (!zebra_nhg_depends_is_empty(depend)) { + /* This is a group within a group */ + i = zebra_nhg_nhe2grp_internal(grp, i, depend, max_num); + } else { + /* Check for duplicate IDs, kernel doesn't like that */ + for (int j = 0; j < i; j++) { + if (depend->id == grp[j].id) + duplicate = true; + } - if (!duplicate) { - grp[i].id = depend->id; - /* We aren't using weights for anything right now */ - grp[i].weight = depend->nhg.nexthop->weight; - i++; + if (!duplicate) { + grp[i].id = depend->id; + grp[i].weight = depend->nhg.nexthop->weight; + i++; + } } - - if (i >= max_num) - goto done; } done: return i; } +/* Convert a nhe into a group array */ +uint8_t zebra_nhg_nhe2grp(struct nh_grp *grp, struct nhg_hash_entry *nhe, + int max_num) +{ + /* Call into the recursive function */ + return zebra_nhg_nhe2grp_internal(grp, 0, nhe, max_num); +} + void zebra_nhg_install_kernel(struct nhg_hash_entry *nhe) { struct nhg_connected *rb_node_dep = NULL; From 1866b3afc259c565f42a9aab262ede27c142eea4 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Tue, 28 Jan 2020 14:33:10 -0500 Subject: [PATCH 2/6] zebra: don't add ID to kernel nh_grp if not installed/queued When we transform the nexthop group rb trees into a flat array of IDs to send into the dataplane code (zebra_nhg_nhe2grp), don't put an ID in there that has not been in installed or is not currently queued to be installed into the dataplane. Otherwise, if some of the nexthops fail to install, we will still try to create a group with them and then the entire group will fail. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 8e923d2d50..7b54c06836 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -1882,6 +1882,20 @@ static uint8_t zebra_nhg_nhe2grp_internal(struct nh_grp *grp, /* This is a group within a group */ i = zebra_nhg_nhe2grp_internal(grp, i, depend, max_num); } else { + /* If the nexthop not installed/queued for install don't + * put in the ID array. + */ + if (!(CHECK_FLAG(depend->flags, NEXTHOP_GROUP_INSTALLED) + || CHECK_FLAG(depend->flags, + NEXTHOP_GROUP_QUEUED))) { + if (IS_ZEBRA_DEBUG_RIB_DETAILED + || IS_ZEBRA_DEBUG_NHG) + zlog_debug( + "%s: Nexthop ID (%u) not installed or queued for install, not appending to dataplane install group", + __func__, depend->id); + continue; + } + /* Check for duplicate IDs, kernel doesn't like that */ for (int j = 0; j < i; j++) { if (depend->id == grp[j].id) From b1c3f7ef80fb626a15a1043b4c46f53c3fd34826 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Tue, 28 Jan 2020 15:20:18 -0500 Subject: [PATCH 3/6] zebra: add debug for duplicate NH in dataplane array conversion When we find a nexthop ID thats a duplicate in the code that converts NHG rb trees into a flat list of nexthop IDs for the dataplane, output a debug message. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 7b54c06836..6a14e53b4b 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -1896,17 +1896,24 @@ static uint8_t zebra_nhg_nhe2grp_internal(struct nh_grp *grp, continue; } - /* Check for duplicate IDs, kernel doesn't like that */ + /* Check for duplicate IDs, ignore if found. */ for (int j = 0; j < i; j++) { if (depend->id == grp[j].id) duplicate = true; } - if (!duplicate) { - grp[i].id = depend->id; - grp[i].weight = depend->nhg.nexthop->weight; - i++; + if (duplicate) { + if (IS_ZEBRA_DEBUG_RIB_DETAILED + || IS_ZEBRA_DEBUG_NHG) + zlog_debug( + "%s: Nexthop ID (%u) is duplicate, not appending to dataplane install group", + __func__, depend->id); + continue; } + + grp[i].id = depend->id; + grp[i].weight = depend->nhg.nexthop->weight; + i++; } } From 715e5c70d564017a8b709599e23bf94884ab9bb9 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Thu, 30 Jan 2020 12:34:35 -0500 Subject: [PATCH 4/6] zebra: set valid on re->nhe directly in nexthop_active_update() We were still doing a lookup on the nhe_id from before we started referencing re->nhe directly. Change set flag to just use re->nhe directly here since they should always be the same at this point in the code anyway. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 6a14e53b4b..6956d4d34e 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -1811,19 +1811,8 @@ int nexthop_active_update(struct route_node *rn, struct route_entry *re) route_entry_update_nhe(re, new_nhe); } - if (curr_active) { - struct nhg_hash_entry *nhe = NULL; - - nhe = zebra_nhg_lookup_id(re->nhe_id); - - if (nhe) - SET_FLAG(nhe->flags, NEXTHOP_GROUP_VALID); - else - flog_err( - EC_ZEBRA_TABLE_LOOKUP_FAILED, - "Active update on NHE id=%u that we do not have in our tables", - re->nhe_id); - } + if (curr_active) + SET_FLAG(re->nhe->flags, NEXTHOP_GROUP_VALID); /* * Do not need these nexthops anymore since they From 086e4e02f5fe02849ad9a6c17472094c1c1cbd98 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Thu, 30 Jan 2020 16:43:09 -0500 Subject: [PATCH 5/6] zebra: properly set the NEXTHOP_GROUP_VALID flag Properly set the NEXTHOP_GROUP_VALID flag and use it as a conditional for installation decisions for individual nexthop and groups containing it. We set the NEXTHOP_GROUP_VALID flag it is: 1) A fully resolved active nexthop or 2) Its a group that contains at least one VALID NHE Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 45 insertions(+), 2 deletions(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 6956d4d34e..c2582b93e1 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -1737,6 +1737,36 @@ static unsigned nexthop_active_check(struct route_node *rn, return CHECK_FLAG(nexthop->flags, NEXTHOP_FLAG_ACTIVE); } +/* Helper function called after resolution to walk nhg rb trees + * and toggle the NEXTHOP_GROUP_VALID flag if the nexthop + * is active on singleton NHEs. + */ +static bool zebra_nhg_set_valid_if_active(struct nhg_hash_entry *nhe) +{ + struct nhg_connected *rb_node_dep = NULL; + bool valid = false; + + if (!zebra_nhg_depends_is_empty(nhe)) { + /* Is at least one depend valid? */ + frr_each(nhg_connected_tree, &nhe->nhg_depends, rb_node_dep) { + if (zebra_nhg_set_valid_if_active(rb_node_dep->nhe)) + valid = true; + } + + goto done; + } + + /* should be fully resolved singleton at this point */ + if (CHECK_FLAG(nhe->nhg.nexthop->flags, NEXTHOP_FLAG_ACTIVE)) + valid = true; + +done: + if (valid) + SET_FLAG(nhe->flags, NEXTHOP_GROUP_VALID); + + return valid; +} + /* * Iterate over all nexthops of the given RIB entry and refresh their * ACTIVE flag. If any nexthop is found to toggle the ACTIVE flag, @@ -1811,8 +1841,11 @@ int nexthop_active_update(struct route_node *rn, struct route_entry *re) route_entry_update_nhe(re, new_nhe); } + /* Walk the NHE depends tree and toggle NEXTHOP_GROUP_VALID + * flag where appropriate. + */ if (curr_active) - SET_FLAG(re->nhe->flags, NEXTHOP_GROUP_VALID); + zebra_nhg_set_valid_if_active(re->nhe); /* * Do not need these nexthops anymore since they @@ -1871,6 +1904,15 @@ static uint8_t zebra_nhg_nhe2grp_internal(struct nh_grp *grp, /* This is a group within a group */ i = zebra_nhg_nhe2grp_internal(grp, i, depend, max_num); } else { + if (!CHECK_FLAG(depend->flags, NEXTHOP_GROUP_VALID)) { + if (IS_ZEBRA_DEBUG_RIB_DETAILED + || IS_ZEBRA_DEBUG_NHG) + zlog_debug( + "%s: Nexthop ID (%u) not valid, not appending to dataplane install group", + __func__, depend->id); + continue; + } + /* If the nexthop not installed/queued for install don't * put in the ID array. */ @@ -1930,7 +1972,8 @@ void zebra_nhg_install_kernel(struct nhg_hash_entry *nhe) zebra_nhg_install_kernel(rb_node_dep->nhe); } - if (!CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_INSTALLED) + if (CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_VALID) + && !CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_INSTALLED) && !CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_QUEUED)) { /* Change its type to us since we are installing it */ nhe->type = ZEBRA_ROUTE_NHG; From d43122b58f4bf9cb729c786b990ae254b8dde35b Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Thu, 26 Mar 2020 10:57:45 -0400 Subject: [PATCH 6/6] zebra: break if duplicate nexthop found in nhe2grp If we find that a nexthop is a duplicate, break immediately rather than continuing to look through the rest of the list. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index c2582b93e1..dc0af050d7 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -1929,8 +1929,10 @@ static uint8_t zebra_nhg_nhe2grp_internal(struct nh_grp *grp, /* Check for duplicate IDs, ignore if found. */ for (int j = 0; j < i; j++) { - if (depend->id == grp[j].id) + if (depend->id == grp[j].id) { duplicate = true; + break; + } } if (duplicate) {