From 786a9bd9eb6c66fb69acb388e127b809f029a391 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 9 Jan 2020 09:01:10 -0500 Subject: [PATCH 01/57] zebra: Convert zserv_nexthop_num_warn to return bool Allow us to key of the warning if we have one. Signed-off-by: Donald Sharp --- zebra/zapi_msg.c | 6 ++++-- zebra/zapi_msg.h | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index e436e5a288..6d7e842354 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -1411,8 +1411,7 @@ stream_failure: return; } - -void zserv_nexthop_num_warn(const char *caller, const struct prefix *p, +bool zserv_nexthop_num_warn(const char *caller, const struct prefix *p, const unsigned int nexthop_num) { if (nexthop_num > zrouter.multipath_num) { @@ -1423,7 +1422,10 @@ void zserv_nexthop_num_warn(const char *caller, const struct prefix *p, EC_ZEBRA_MORE_NH_THAN_MULTIPATH, "%s: Prefix %s has %d nexthops, but we can only use the first %d", caller, buff, nexthop_num, zrouter.multipath_num); + return true; } + + return false; } /* diff --git a/zebra/zapi_msg.h b/zebra/zapi_msg.h index e7c755c2bf..9f23a313bf 100644 --- a/zebra/zapi_msg.h +++ b/zebra/zapi_msg.h @@ -90,7 +90,7 @@ zsend_ipset_entry_notify_owner(struct zebra_pbr_ipset_entry *ipset, enum zapi_ipset_entry_notify_owner note); extern void zsend_iptable_notify_owner(struct zebra_pbr_iptable *iptable, enum zapi_iptable_notify_owner note); -extern void zserv_nexthop_num_warn(const char *caller, const struct prefix *p, +extern bool zserv_nexthop_num_warn(const char *caller, const struct prefix *p, const unsigned int nexthop_num); extern void zsend_capabilities_all_clients(void); From 224b3c8a79a6635400d7b92f5d89eab1bd9e08a2 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 22 Apr 2020 09:41:01 -0400 Subject: [PATCH 02/57] lib: Add the ability to grab a nhg starting id Add new function zclient_get_nhg_start that will allow an upper level protocol to get a starting point for it's own nhg space. Give each protocol a space of 50 million. zebra will own the space from 0 - 199999999 because of SYSTEM, KERNEL and CONNECT route types. This is the start of some work that will allow upper level protocols to install and maintain their own NHG's. Signed-off-by: Donald Sharp --- lib/zclient.c | 10 ++++++++++ lib/zclient.h | 8 ++++++++ 2 files changed, 18 insertions(+) diff --git a/lib/zclient.c b/lib/zclient.c index c5016d22e2..b0549bf84e 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -4001,3 +4001,13 @@ int zclient_send_neigh_discovery_req(struct zclient *zclient, stream_putw_at(s, 0, stream_get_endp(s)); return zclient_send_message(zclient); } + +/* + * Get a starting nhg point for a routing protocol + */ +uint32_t zclient_get_nhg_start(uint32_t proto) +{ + assert(proto < ZEBRA_ROUTE_MAX); + + return ZEBRA_NHG_SPACING * proto; +} diff --git a/lib/zclient.h b/lib/zclient.h index 050877f27a..d2f559c45d 100644 --- a/lib/zclient.h +++ b/lib/zclient.h @@ -671,6 +671,14 @@ struct zclient_options { extern struct zclient_options zclient_options_default; +/* + * Each client is going to get it's own nexthop group space + * and we'll separate them by 50 million, we'll figure out where + * to start based upon the route_types.h + */ +#define ZEBRA_NHG_SPACING 50000000 +extern uint32_t zclient_get_nhg_start(uint32_t proto); + extern struct zclient *zclient_new(struct thread_master *m, struct zclient_options *opt); From f70da2a390dbcad64f86118976b3abefa3da1fa2 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 29 Apr 2020 10:11:34 -0400 Subject: [PATCH 03/57] zebra: Refactor nexthop reading from zapi messages Take the zebra code that reads nexthops and combine it into one function so that when we add zapi messages to send/receive nexthops we can take advantage of this function. Signed-off-by: Donald Sharp --- zebra/zapi_msg.c | 293 ++++++++++++++++++++++------------------------- 1 file changed, 134 insertions(+), 159 deletions(-) diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 6d7e842354..1301a577f2 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -1578,23 +1578,145 @@ done: return nexthop; } +static bool zapi_read_nexthops(struct zserv *client, struct zapi_route *api, + struct route_entry *re, + struct nexthop_group **png, + struct nhg_backup_info **pbnhg) +{ + struct nexthop_group *ng = NULL; + struct nhg_backup_info *bnhg = NULL; + uint16_t nexthop_num, i;; + struct zapi_nexthop *nhops; + struct nexthop *last_nh = NULL; + + assert(!(png && pbnhg)); + + if (png) { + *png = ng = nexthop_group_new(); + nexthop_num = api->nexthop_num; + nhops = api->nexthops; + } + + if (pbnhg && api->backup_nexthop_num > 0) { + if (IS_ZEBRA_DEBUG_RECV) + zlog_debug("%s: adding %d backup nexthops", + __func__, api->backup_nexthop_num); + + nexthop_num = api->backup_nexthop_num; + *pbnhg = bnhg = zebra_nhg_backup_alloc(); + nhops = api->backup_nexthops; + } + + /* + * TBD should _all_ of the nexthop add operations use + * api_nh->vrf_id instead of re->vrf_id ? I only changed + * for cases NEXTHOP_TYPE_IPV4 and NEXTHOP_TYPE_IPV6. + */ + for (i = 0; i < nexthop_num; i++) { + struct nexthop *nexthop; + enum lsp_types_t label_type; + char nhbuf[NEXTHOP_STRLEN]; + char labelbuf[MPLS_LABEL_STRLEN]; + struct zapi_nexthop *api_nh = &nhops[i]; + + /* Convert zapi nexthop */ + nexthop = nexthop_from_zapi(re, api_nh, api); + if (!nexthop) { + flog_warn( + EC_ZEBRA_NEXTHOP_CREATION_FAILED, + "%s: Nexthops Specified: %u(%u) but we failed to properly create one", + __func__, nexthop_num, i); + if (ng) + nexthop_group_delete(&ng); + if (bnhg) + zebra_nhg_backup_free(&bnhg); + return false; + } + + if (bnhg && CHECK_FLAG(nexthop->flags, NEXTHOP_FLAG_HAS_BACKUP)) { + if (IS_ZEBRA_DEBUG_RECV) { + nexthop2str(nexthop, nhbuf, sizeof(nhbuf)); + zlog_debug("%s: backup nh %s with BACKUP flag!", + __func__, nhbuf); + } + UNSET_FLAG(nexthop->flags, NEXTHOP_FLAG_HAS_BACKUP); + nexthop->backup_num = 0; + } + + if (CHECK_FLAG(api->message, ZAPI_MESSAGE_SRTE)) { + SET_FLAG(nexthop->flags, NEXTHOP_FLAG_SRTE); + nexthop->srte_color = api_nh->srte_color; + } + + /* MPLS labels for BGP-LU or Segment Routing */ + if (CHECK_FLAG(api_nh->flags, ZAPI_NEXTHOP_FLAG_LABEL) + && api_nh->type != NEXTHOP_TYPE_IFINDEX + && api_nh->type != NEXTHOP_TYPE_BLACKHOLE + && api_nh->label_num > 0) { + + label_type = lsp_type_from_re_type(client->proto); + nexthop_add_labels(nexthop, label_type, + api_nh->label_num, + &api_nh->labels[0]); + } + + if (IS_ZEBRA_DEBUG_RECV) { + labelbuf[0] = '\0'; + nhbuf[0] = '\0'; + + nexthop2str(nexthop, nhbuf, sizeof(nhbuf)); + + if (nexthop->nh_label && + nexthop->nh_label->num_labels > 0) { + mpls_label2str(nexthop->nh_label->num_labels, + nexthop->nh_label->label, + labelbuf, sizeof(labelbuf), + false); + } + + zlog_debug("%s: nh=%s, vrf_id=%d %s", + __func__, nhbuf, api_nh->vrf_id, labelbuf); + } + + if (png) { + /* Add new nexthop to temporary list. This list is + * canonicalized - sorted - so that it can be hashed later + * in route processing. We expect that the sender has sent + * the list sorted, and the zapi client api attempts to enforce + * that, so this should be inexpensive - but it is necessary + * to support shared nexthop-groups. + */ + nexthop_group_add_sorted(ng, nexthop); + } + if (bnhg) { + /* Note that the order of the backup nexthops is significant, + * so we don't sort this list as we do the primary nexthops, + * we just append. + */ + if (last_nh) + NEXTHOP_APPEND(last_nh, nexthop); + else + bnhg->nhe->nhg.nexthop = nexthop; + + last_nh = nexthop; + } + } + + return true; +} + static void zread_route_add(ZAPI_HANDLER_ARGS) { struct stream *s; struct zapi_route api; - struct zapi_nexthop *api_nh; afi_t afi; struct prefix_ipv6 *src_p = NULL; struct route_entry *re; - struct nexthop *nexthop = NULL, *last_nh; struct nexthop_group *ng = NULL; struct nhg_backup_info *bnhg = NULL; - int i, ret; + int ret; vrf_id_t vrf_id; struct nhg_hash_entry nhe; - enum lsp_types_t label_type; - char nhbuf[NEXTHOP_STRLEN]; - char labelbuf[MPLS_LABEL_STRLEN]; s = msg; if (zapi_route_decode(s, &api) < 0) { @@ -1611,7 +1733,8 @@ static void zread_route_add(ZAPI_HANDLER_ARGS) prefix2str(&api.prefix, buf_prefix, sizeof(buf_prefix)); zlog_debug("%s: p=(%u:%u)%s, msg flags=0x%x, flags=0x%x", - __func__, vrf_id, api.tableid, buf_prefix, (int)api.message, api.flags); + __func__, vrf_id, api.tableid, buf_prefix, + (int)api.message, api.flags); } /* Allocate new route. */ @@ -1647,158 +1770,10 @@ static void zread_route_add(ZAPI_HANDLER_ARGS) zebra_route_string(client->proto), &api.prefix); } - /* Use temporary list of nexthops */ - ng = nexthop_group_new(); - - /* - * TBD should _all_ of the nexthop add operations use - * api_nh->vrf_id instead of re->vrf_id ? I only changed - * for cases NEXTHOP_TYPE_IPV4 and NEXTHOP_TYPE_IPV6. - */ - for (i = 0; i < api.nexthop_num; i++) { - api_nh = &api.nexthops[i]; - - /* Convert zapi nexthop */ - nexthop = nexthop_from_zapi(re, api_nh, &api); - if (!nexthop) { - flog_warn( - EC_ZEBRA_NEXTHOP_CREATION_FAILED, - "%s: Nexthops Specified: %d but we failed to properly create one", - __func__, api.nexthop_num); - nexthop_group_delete(&ng); - XFREE(MTYPE_RE, re); - return; - } - - if (CHECK_FLAG(api.message, ZAPI_MESSAGE_SRTE)) { - SET_FLAG(nexthop->flags, NEXTHOP_FLAG_SRTE); - nexthop->srte_color = api_nh->srte_color; - } - - /* MPLS labels for BGP-LU or Segment Routing */ - if (CHECK_FLAG(api_nh->flags, ZAPI_NEXTHOP_FLAG_LABEL) - && api_nh->type != NEXTHOP_TYPE_IFINDEX - && api_nh->type != NEXTHOP_TYPE_BLACKHOLE - && api_nh->label_num > 0) { - - label_type = lsp_type_from_re_type(client->proto); - nexthop_add_labels(nexthop, label_type, - api_nh->label_num, - &api_nh->labels[0]); - } - - if (IS_ZEBRA_DEBUG_RECV) { - labelbuf[0] = '\0'; - nhbuf[0] = '\0'; - - nexthop2str(nexthop, nhbuf, sizeof(nhbuf)); - - if (nexthop->nh_label && - nexthop->nh_label->num_labels > 0) { - mpls_label2str(nexthop->nh_label->num_labels, - nexthop->nh_label->label, - labelbuf, sizeof(labelbuf), - false); - } - - zlog_debug("%s: nh=%s, vrf_id=%d %s", - __func__, nhbuf, api_nh->vrf_id, labelbuf); - } - - /* Add new nexthop to temporary list. This list is - * canonicalized - sorted - so that it can be hashed later - * in route processing. We expect that the sender has sent - * the list sorted, and the zapi client api attempts to enforce - * that, so this should be inexpensive - but it is necessary - * to support shared nexthop-groups. - */ - nexthop_group_add_sorted(ng, nexthop); - } - - /* Allocate temporary list of backup nexthops, if necessary */ - if (api.backup_nexthop_num > 0) { - if (IS_ZEBRA_DEBUG_RECV) - zlog_debug("%s: adding %d backup nexthops", - __func__, api.backup_nexthop_num); - - bnhg = zebra_nhg_backup_alloc(); - nexthop = NULL; - last_nh = NULL; - } - - /* Copy backup nexthops also, if present */ - for (i = 0; i < api.backup_nexthop_num; i++) { - api_nh = &api.backup_nexthops[i]; - - /* Convert zapi backup nexthop */ - nexthop = nexthop_from_zapi(re, api_nh, &api); - if (!nexthop) { - flog_warn( - EC_ZEBRA_NEXTHOP_CREATION_FAILED, - "%s: Backup Nexthops Specified: %d but we failed to properly create one", - __func__, api.backup_nexthop_num); - nexthop_group_delete(&ng); - zebra_nhg_backup_free(&bnhg); - XFREE(MTYPE_RE, re); - return; - } - - /* Backup nexthops can't have backups; that's not valid. */ - if (CHECK_FLAG(nexthop->flags, NEXTHOP_FLAG_HAS_BACKUP)) { - if (IS_ZEBRA_DEBUG_RECV) { - nexthop2str(nexthop, nhbuf, sizeof(nhbuf)); - zlog_debug("%s: backup nh %s with BACKUP flag!", - __func__, nhbuf); - } - UNSET_FLAG(nexthop->flags, NEXTHOP_FLAG_HAS_BACKUP); - nexthop->backup_num = 0; - } - - if (CHECK_FLAG(api.message, ZAPI_MESSAGE_SRTE)) { - SET_FLAG(nexthop->flags, NEXTHOP_FLAG_SRTE); - nexthop->srte_color = api_nh->srte_color; - } - - /* MPLS labels for BGP-LU or Segment Routing */ - if (CHECK_FLAG(api_nh->flags, ZAPI_NEXTHOP_FLAG_LABEL) - && api_nh->type != NEXTHOP_TYPE_IFINDEX - && api_nh->type != NEXTHOP_TYPE_BLACKHOLE - && api_nh->label_num > 0) { - - label_type = lsp_type_from_re_type(client->proto); - nexthop_add_labels(nexthop, label_type, - api_nh->label_num, - &api_nh->labels[0]); - } - - if (IS_ZEBRA_DEBUG_RECV) { - labelbuf[0] = '\0'; - nhbuf[0] = '\0'; - - nexthop2str(nexthop, nhbuf, sizeof(nhbuf)); - - if (nexthop->nh_label && - nexthop->nh_label->num_labels > 0) { - mpls_label2str(nexthop->nh_label->num_labels, - nexthop->nh_label->label, - labelbuf, sizeof(labelbuf), - false); - } - - zlog_debug("%s: backup nh=%s, vrf_id=%d %s", - __func__, nhbuf, api_nh->vrf_id, labelbuf); - } - - /* Note that the order of the backup nexthops is significant, - * so we don't sort this list as we do the primary nexthops, - * we just append. - */ - if (last_nh) - NEXTHOP_APPEND(last_nh, nexthop); - else - bnhg->nhe->nhg.nexthop = nexthop; - - last_nh = nexthop; + if (!zapi_read_nexthops(client, &api, re, &ng, NULL) || + !zapi_read_nexthops(client, &api, re, NULL, &bnhg)) { + XFREE(MTYPE_RE, re); + return; } if (CHECK_FLAG(api.message, ZAPI_MESSAGE_DISTANCE)) From 2f35a820bf51f114c3c382f65c2dd3e1e9b67194 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 29 Apr 2020 10:49:21 -0400 Subject: [PATCH 04/57] lib, zebra: Add ZAPI_NHG_ADD|DELETE Add the ability to send a NHG from an upper level protocol down to zebra. ZAPI_NHG_ADD encompasses both the addition and replace semantics ( If the id passed down does not exist yet, it's Add, else it's a replace ). Effectively zebra will take this nhg passed down save the nhg in the id hash for nhg's and then create the appropriate nhg's and finally install them into the linux kernel. Notification will be the ZAPI_NHG_NOTIFY_OWNER zapi message for normal success/failure messaging to the installing protocol. This work is being done to allow us to work with EVPN MH which needs the ability to modify NHG's that BGP will own and operate on. Signed-off-by: Donald Sharp --- lib/zclient.c | 70 ++++++++++++++++++++- lib/zclient.h | 22 ++++++- zebra/zapi_msg.c | 161 +++++++++++++++++++++++++++++++++++++++-------- 3 files changed, 224 insertions(+), 29 deletions(-) diff --git a/lib/zclient.c b/lib/zclient.c index b0549bf84e..9ff3a8eea3 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -1017,6 +1017,51 @@ done: return ret; } +extern void zclient_nhg_del(struct zclient *zclient, uint32_t id) +{ + struct stream *s = zclient->obuf; + + stream_reset(s); + zclient_create_header(s, ZEBRA_NHG_DEL, VRF_DEFAULT); + + stream_putw(s, zclient->redist_default); + stream_putl(s, id); + + stream_putw_at(s, 0, stream_get_endp(s)); +} + +static void zclient_nhg_writer(struct stream *s, uint16_t proto, int cmd, + uint32_t id, size_t nhops, + struct zapi_nexthop *znh) +{ + size_t i; + + stream_reset(s); + + zapi_nexthop_group_sort(znh, nhops); + + zclient_create_header(s, cmd, VRF_DEFAULT); + + stream_putw(s, proto); + stream_putl(s, id); + stream_putw(s, nhops); + for (i = 0; i < nhops; i++) { + zapi_nexthop_encode(s, znh, 0, 0); + znh++; + } + + stream_putw_at(s, 0, stream_get_endp(s)); +} + +extern void zclient_nhg_add(struct zclient *zclient, uint32_t id, + size_t nhops, struct zapi_nexthop *znh) +{ + struct stream *s = zclient->obuf; + + zclient_nhg_writer(s, zclient->redist_default, ZEBRA_NHG_ADD, id, nhops, + znh); +} + int zapi_route_encode(uint8_t cmd, struct stream *s, struct zapi_route *api) { struct zapi_nexthop *api_nh; @@ -1171,8 +1216,8 @@ int zapi_route_encode(uint8_t cmd, struct stream *s, struct zapi_route *api) /* * Decode a single zapi nexthop object */ -static int zapi_nexthop_decode(struct stream *s, struct zapi_nexthop *api_nh, - uint32_t api_flags, uint32_t api_message) +int zapi_nexthop_decode(struct stream *s, struct zapi_nexthop *api_nh, + uint32_t api_flags, uint32_t api_message) { int i, ret = -1; @@ -1432,6 +1477,22 @@ int zapi_pbr_rule_encode(uint8_t cmd, struct stream *s, struct pbr_rule *zrule) return 0; } +bool zapi_nhg_notify_decode(struct stream *s, uint32_t *id, + enum zapi_nhg_notify_owner *note) +{ + uint16_t read_id; + + STREAM_GETL(s, read_id); + STREAM_GET(note, s, sizeof(*note)); + + *id = read_id; + + return true; + +stream_failure: + return false; +} + bool zapi_route_notify_decode(struct stream *s, struct prefix *p, uint32_t *tableid, enum zapi_route_notify_owner *note) @@ -3733,6 +3794,11 @@ static int zclient_read(struct thread *thread) (*zclient->rule_notify_owner)(command, zclient, length, vrf_id); break; + case ZEBRA_NHG_NOTIFY_OWNER: + if (zclient->nhg_notify_owner) + (*zclient->nhg_notify_owner)(command, zclient, length, + vrf_id); + break; case ZEBRA_GET_LABEL_CHUNK: if (zclient->label_chunk) (*zclient->label_chunk)(command, zclient, length, diff --git a/lib/zclient.h b/lib/zclient.h index d2f559c45d..40f345a1be 100644 --- a/lib/zclient.h +++ b/lib/zclient.h @@ -209,6 +209,9 @@ typedef enum { ZEBRA_MLAG_CLIENT_REGISTER, ZEBRA_MLAG_CLIENT_UNREGISTER, ZEBRA_MLAG_FORWARD_MSG, + ZEBRA_NHG_ADD, + ZEBRA_NHG_DEL, + ZEBRA_NHG_NOTIFY_OWNER, ZEBRA_ERROR, ZEBRA_CLIENT_CAPABILITIES, ZEBRA_OPAQUE_MESSAGE, @@ -354,6 +357,7 @@ struct zclient { int (*mlag_process_up)(void); int (*mlag_process_down)(void); int (*mlag_handle_msg)(struct stream *msg, int len); + int (*nhg_notify_owner)(ZAPI_CALLBACK_ARGS); int (*handle_error)(enum zebra_error_types error); int (*opaque_msg_handler)(ZAPI_CALLBACK_ARGS); int (*opaque_register_handler)(ZAPI_CALLBACK_ARGS); @@ -592,6 +596,13 @@ enum zapi_route_notify_owner { ZAPI_ROUTE_REMOVE_FAIL, }; +enum zapi_nhg_notify_owner { + ZAPI_NHG_FAIL_INSTALL, + ZAPI_NHG_INSTALLED, + ZAPI_NHG_REMOVED, + ZAPI_NHG_REMOVE_FAIL, +}; + enum zapi_rule_notify_owner { ZAPI_RULE_FAIL_INSTALL, ZAPI_RULE_INSTALLED, @@ -861,7 +872,11 @@ extern int zclient_send_rnh(struct zclient *zclient, int command, int zapi_nexthop_encode(struct stream *s, const struct zapi_nexthop *api_nh, uint32_t api_flags, uint32_t api_message); extern int zapi_route_encode(uint8_t, struct stream *, struct zapi_route *); -extern int zapi_route_decode(struct stream *, struct zapi_route *); +extern int zapi_route_decode(struct stream *s, struct zapi_route *api); +extern int zapi_nexthop_decode(struct stream *s, struct zapi_nexthop *api_nh, + uint32_t api_flags, uint32_t api_message); +bool zapi_nhg_notify_decode(struct stream *s, uint32_t *id, + enum zapi_nhg_notify_owner *note); bool zapi_route_notify_decode(struct stream *s, struct prefix *p, uint32_t *tableid, enum zapi_route_notify_owner *note); @@ -872,6 +887,11 @@ bool zapi_ipset_notify_decode(struct stream *s, uint32_t *unique, enum zapi_ipset_notify_owner *note); +extern void zclient_nhg_add(struct zclient *zclient, + uint32_t id, size_t nhops, + struct zapi_nexthop *znh); +extern void zclient_nhg_del(struct zclient *zclient, uint32_t id); + #define ZEBRA_IPSET_NAME_SIZE 32 bool zapi_ipset_entry_notify_decode(struct stream *s, diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 1301a577f2..2d04da5052 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -712,6 +712,34 @@ static int zsend_ipv4_nexthop_lookup_mrib(struct zserv *client, return zserv_send_message(client, s); } +static int nhg_notify(uint16_t type, uint16_t instance, uint16_t id, + enum zapi_nhg_notify_owner note) +{ + struct zserv *client; + struct stream *s; + + client = zserv_find_client(type, instance); + if (!client) { + if (IS_ZEBRA_DEBUG_PACKET) { + zlog_debug("Not Notifying Owner: %u(%u) about %u(%d)", + type, instance, id, note); + } + return 0; + } + + s = stream_new(ZEBRA_MAX_PACKET_SIZ); + stream_reset(s); + + zclient_create_header(s, ZEBRA_NHG_NOTIFY_OWNER, VRF_DEFAULT); + + stream_putw(s, id); + stream_put(s, ¬e, sizeof(note)); + + stream_putw_at(s, 0, stream_get_endp(s)); + + return zserv_send_message(client, s); +} + /* * Common utility send route notification, called from a path using a * route_entry and from a path using a dataplane context. @@ -1431,9 +1459,9 @@ bool zserv_nexthop_num_warn(const char *caller, const struct prefix *p, /* * Create a new nexthop based on a zapi nexthop. */ -static struct nexthop *nexthop_from_zapi(struct route_entry *re, - const struct zapi_nexthop *api_nh, - const struct zapi_route *api) +static struct nexthop *nexthop_from_zapi(const struct zapi_nexthop *api_nh, + uint32_t flags, struct prefix *p, + uint16_t backup_nexthop_num) { struct nexthop *nexthop = NULL; struct ipaddr vtep_ip; @@ -1471,14 +1499,14 @@ static struct nexthop *nexthop_from_zapi(struct route_entry *re, /* Special handling for IPv4 routes sourced from EVPN: * the nexthop and associated MAC need to be installed. */ - if (CHECK_FLAG(api->flags, ZEBRA_FLAG_EVPN_ROUTE)) { + if (CHECK_FLAG(flags, ZEBRA_FLAG_EVPN_ROUTE)) { memset(&vtep_ip, 0, sizeof(struct ipaddr)); vtep_ip.ipa_type = IPADDR_V4; memcpy(&(vtep_ip.ipaddr_v4), &(api_nh->gate.ipv4), sizeof(struct in_addr)); zebra_vxlan_evpn_vrf_route_add( api_nh->vrf_id, &api_nh->rmac, - &vtep_ip, &api->prefix); + &vtep_ip, p); } break; case NEXTHOP_TYPE_IPV6: @@ -1505,14 +1533,14 @@ static struct nexthop *nexthop_from_zapi(struct route_entry *re, /* Special handling for IPv6 routes sourced from EVPN: * the nexthop and associated MAC need to be installed. */ - if (CHECK_FLAG(api->flags, ZEBRA_FLAG_EVPN_ROUTE)) { + if (CHECK_FLAG(flags, ZEBRA_FLAG_EVPN_ROUTE)) { memset(&vtep_ip, 0, sizeof(struct ipaddr)); vtep_ip.ipa_type = IPADDR_V6; memcpy(&vtep_ip.ipaddr_v6, &(api_nh->gate.ipv6), sizeof(struct in6_addr)); zebra_vxlan_evpn_vrf_route_add( api_nh->vrf_id, &api_nh->rmac, - &vtep_ip, &api->prefix); + &vtep_ip, p); } break; case NEXTHOP_TYPE_BLACKHOLE: @@ -1560,7 +1588,7 @@ static struct nexthop *nexthop_from_zapi(struct route_entry *re, for (i = 0; i < api_nh->backup_num; i++) { /* Validate backup index */ - if (api_nh->backup_idx[i] < api->backup_nexthop_num) { + if (api_nh->backup_idx[i] < backup_nexthop_num) { nexthop->backup_idx[i] = api_nh->backup_idx[i]; } else { if (IS_ZEBRA_DEBUG_RECV || IS_ZEBRA_DEBUG_EVENT) @@ -1578,33 +1606,29 @@ done: return nexthop; } -static bool zapi_read_nexthops(struct zserv *client, struct zapi_route *api, - struct route_entry *re, +static bool zapi_read_nexthops(struct zserv *client, struct prefix *p, + struct zapi_nexthop *nhops, uint32_t flags, + uint32_t message, uint16_t nexthop_num, + uint16_t backup_nh_num, struct nexthop_group **png, struct nhg_backup_info **pbnhg) { struct nexthop_group *ng = NULL; struct nhg_backup_info *bnhg = NULL; - uint16_t nexthop_num, i;; - struct zapi_nexthop *nhops; + uint16_t i; struct nexthop *last_nh = NULL; assert(!(png && pbnhg)); - if (png) { + if (png) *png = ng = nexthop_group_new(); - nexthop_num = api->nexthop_num; - nhops = api->nexthops; - } - if (pbnhg && api->backup_nexthop_num > 0) { + if (pbnhg && backup_nh_num > 0) { if (IS_ZEBRA_DEBUG_RECV) - zlog_debug("%s: adding %d backup nexthops", - __func__, api->backup_nexthop_num); + zlog_debug("%s: adding %d backup nexthops", __func__, + backup_nh_num); - nexthop_num = api->backup_nexthop_num; *pbnhg = bnhg = zebra_nhg_backup_alloc(); - nhops = api->backup_nexthops; } /* @@ -1620,7 +1644,7 @@ static bool zapi_read_nexthops(struct zserv *client, struct zapi_route *api, struct zapi_nexthop *api_nh = &nhops[i]; /* Convert zapi nexthop */ - nexthop = nexthop_from_zapi(re, api_nh, api); + nexthop = nexthop_from_zapi(api_nh, flags, p, backup_nh_num); if (!nexthop) { flog_warn( EC_ZEBRA_NEXTHOP_CREATION_FAILED, @@ -1643,7 +1667,7 @@ static bool zapi_read_nexthops(struct zserv *client, struct zapi_route *api, nexthop->backup_num = 0; } - if (CHECK_FLAG(api->message, ZAPI_MESSAGE_SRTE)) { + if (CHECK_FLAG(message, ZAPI_MESSAGE_SRTE)) { SET_FLAG(nexthop->flags, NEXTHOP_FLAG_SRTE); nexthop->srte_color = api_nh->srte_color; } @@ -1705,6 +1729,83 @@ static bool zapi_read_nexthops(struct zserv *client, struct zapi_route *api, return true; } +static void zread_nhg_del(ZAPI_HANDLER_ARGS) +{ + struct stream *s = msg; + uint32_t id; + uint16_t proto; + + STREAM_GETW(s, proto); + STREAM_GETL(s, id); + + /* + * Delete the received nhg id + * id is incremented to make compiler happy right now + * it should be removed in future code work. + */ + nhg_notify(proto, client->instance, id, ZAPI_NHG_REMOVED); + + return; + +stream_failure: + flog_warn(EC_ZEBRA_NEXTHOP_CREATION_FAILED, + "%s: Nexthop group deletion failed", __func__); + return; +} + +static void zread_nhg_reader(ZAPI_HANDLER_ARGS) +{ + struct stream *s; + uint32_t id; + size_t nhops, i; + struct zapi_nexthop zapi_nexthops[MULTIPATH_NUM]; + struct nexthop_group *nhg = NULL; + struct prefix p; + uint16_t proto; + + memset(&p, 0, sizeof(p)); + + s = msg; + + STREAM_GETW(s, proto); + STREAM_GETL(s, id); + STREAM_GETW(s, nhops); + + if (zserv_nexthop_num_warn(__func__, &p, nhops)) + return; + + for (i = 0; i < nhops; i++) { + struct zapi_nexthop *znh = &zapi_nexthops[i]; + + if (zapi_nexthop_decode(s, znh, 0, 0) != 0) { + flog_warn(EC_ZEBRA_NEXTHOP_CREATION_FAILED, + "%s: Nexthop creation failed", + __func__); + return; + } + } + + if (!zapi_read_nexthops(client, &p, zapi_nexthops, 0, 0, nhops, 0, &nhg, + NULL)) { + flog_warn(EC_ZEBRA_NEXTHOP_CREATION_FAILED, + "%s: Nexthop Group Creation failed", + __func__); + return; + } + /* + * Install the nhg + */ + nhg_notify(proto, client->instance, id, ZAPI_NHG_INSTALLED); + + return; + +stream_failure: + flog_warn(EC_ZEBRA_NEXTHOP_CREATION_FAILED, + "%s: Nexthop Group creation failed with some sort of stream read failure", + __func__); + return; +} + static void zread_route_add(ZAPI_HANDLER_ARGS) { struct stream *s; @@ -1770,8 +1871,13 @@ static void zread_route_add(ZAPI_HANDLER_ARGS) zebra_route_string(client->proto), &api.prefix); } - if (!zapi_read_nexthops(client, &api, re, &ng, NULL) || - !zapi_read_nexthops(client, &api, re, NULL, &bnhg)) { + if (!zapi_read_nexthops(client, &api.prefix, api.nexthops, api.flags, + api.message, api.nexthop_num, + api.backup_nexthop_num, &ng, NULL) + || !zapi_read_nexthops(client, &api.prefix, api.backup_nexthops, + api.flags, api.message, + api.backup_nexthop_num, + api.backup_nexthop_num, NULL, &bnhg)) { XFREE(MTYPE_RE, re); return; } @@ -3095,7 +3201,10 @@ void (*const zserv_handlers[])(ZAPI_HANDLER_ARGS) = { [ZEBRA_MLAG_CLIENT_UNREGISTER] = zebra_mlag_client_unregister, [ZEBRA_MLAG_FORWARD_MSG] = zebra_mlag_forward_client_msg, [ZEBRA_CLIENT_CAPABILITIES] = zread_client_capabilities, - [ZEBRA_NEIGH_DISCOVER] = zread_neigh_discover}; + [ZEBRA_NEIGH_DISCOVER] = zread_neigh_discover, + [ZEBRA_NHG_ADD] = zread_nhg_reader, + [ZEBRA_NHG_DEL] = zread_nhg_del, +}; /* * Process a batch of zapi messages. From 27141ea94e4ec770f2df8cca3840ac8c3bc101c1 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Fri, 1 May 2020 17:56:23 -0400 Subject: [PATCH 05/57] lib, zebra: Add ability to send down a nhgid over route install Modify the send down of a route to use the nexthop group id if we have one associated with the route. Signed-off-by: Donald Sharp --- lib/zclient.c | 6 ++++++ lib/zclient.h | 3 +++ zebra/zapi_msg.c | 22 +++++++++++++++++----- 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/lib/zclient.c b/lib/zclient.c index 9ff3a8eea3..17026d13a6 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -1103,6 +1103,9 @@ int zapi_route_encode(uint8_t cmd, struct stream *s, struct zapi_route *api) stream_write(s, (uint8_t *)&api->src_prefix.prefix, psize); } + if (CHECK_FLAG(api->message, ZAPI_MESSAGE_NHG)) + stream_putl(s, api->nhgid); + /* Nexthops. */ if (CHECK_FLAG(api->message, ZAPI_MESSAGE_NEXTHOP)) { /* limit the number of nexthops if necessary */ @@ -1373,6 +1376,9 @@ int zapi_route_decode(struct stream *s, struct zapi_route *api) } } + if (CHECK_FLAG(api->message, ZAPI_MESSAGE_NHG)) + STREAM_GETL(s, api->nhgid); + /* Nexthops. */ if (CHECK_FLAG(api->message, ZAPI_MESSAGE_NEXTHOP)) { STREAM_GETW(s, api->nexthop_num); diff --git a/lib/zclient.h b/lib/zclient.h index 40f345a1be..b7850cdec7 100644 --- a/lib/zclient.h +++ b/lib/zclient.h @@ -374,6 +374,7 @@ struct zclient { #define ZAPI_MESSAGE_SRCPFX 0x20 /* Backup nexthops are present */ #define ZAPI_MESSAGE_BACKUP_NEXTHOPS 0x40 +#define ZAPI_MESSAGE_NHG 0x80 /* * This should only be used by a DAEMON that needs to communicate @@ -518,6 +519,8 @@ struct zapi_route { uint16_t backup_nexthop_num; struct zapi_nexthop backup_nexthops[MULTIPATH_NUM]; + uint32_t nhgid; + uint8_t distance; uint32_t metric; diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 2d04da5052..7c00ae1032 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -1806,6 +1806,16 @@ stream_failure: return; } +static bool zapi_msg_get_nhg(struct zapi_route *api, struct nexthop_group **ng) +{ + if (!CHECK_FLAG(api->message, ZAPI_MESSAGE_NHG)) + return false; + + /* TODO lookup the ng from api->nhgid */ + *ng = NULL; + return true; +} + static void zread_route_add(ZAPI_HANDLER_ARGS) { struct stream *s; @@ -1851,8 +1861,9 @@ static void zread_route_add(ZAPI_HANDLER_ARGS) else re->table = zvrf->table_id; - if (!CHECK_FLAG(api.message, ZAPI_MESSAGE_NEXTHOP) - || api.nexthop_num == 0) { + if (!CHECK_FLAG(api.message, ZAPI_MESSAGE_NHG) + && (!CHECK_FLAG(api.message, ZAPI_MESSAGE_NEXTHOP) + || api.nexthop_num == 0)) { flog_warn(EC_ZEBRA_RX_ROUTE_NO_NEXTHOPS, "%s: received a route without nexthops for prefix %pFX from client %s", __func__, &api.prefix, @@ -1871,9 +1882,10 @@ static void zread_route_add(ZAPI_HANDLER_ARGS) zebra_route_string(client->proto), &api.prefix); } - if (!zapi_read_nexthops(client, &api.prefix, api.nexthops, api.flags, - api.message, api.nexthop_num, - api.backup_nexthop_num, &ng, NULL) + if (zapi_msg_get_nhg(&api, &ng) + || !zapi_read_nexthops(client, &api.prefix, api.nexthops, api.flags, + api.message, api.nexthop_num, + api.backup_nexthop_num, &ng, NULL) || !zapi_read_nexthops(client, &api.prefix, api.backup_nexthops, api.flags, api.message, api.backup_nexthop_num, From 569e87c0c831252f46fc9a8ddcbc5e268bff8327 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 29 Apr 2020 12:32:53 -0400 Subject: [PATCH 06/57] sharpd: add abilty to send a nhg to zebra Modify the sharpd program to have the ability to pass down a NHG and then operate on it for route installation. Signed-off-by: Donald Sharp --- sharpd/sharp_globals.h | 1 + sharpd/sharp_main.c | 3 +- sharpd/sharp_nht.c | 110 +++++++++++++++++++++++++++++++++++++++++ sharpd/sharp_nht.h | 3 ++ sharpd/sharp_vty.c | 7 ++- sharpd/sharp_zebra.c | 48 +++++++++++++++--- sharpd/sharp_zebra.h | 6 ++- 7 files changed, 165 insertions(+), 13 deletions(-) diff --git a/sharpd/sharp_globals.h b/sharpd/sharp_globals.h index 8eba57f4dd..0bd47454a9 100644 --- a/sharpd/sharp_globals.h +++ b/sharpd/sharp_globals.h @@ -31,6 +31,7 @@ struct sharp_routes { /* The nexthop info we are using for installation */ struct nexthop nhop; struct nexthop backup_nhop; + uint32_t nhgid; struct nexthop_group nhop_group; struct nexthop_group backup_nhop_group; diff --git a/sharpd/sharp_main.c b/sharpd/sharp_main.c index ccf34b10dd..4cd92c7f3d 100644 --- a/sharpd/sharp_main.c +++ b/sharpd/sharp_main.c @@ -47,6 +47,7 @@ #include "sharp_zebra.h" #include "sharp_vty.h" #include "sharp_globals.h" +#include "sharp_nht.h" DEFINE_MGROUP(SHARPD, "sharpd") @@ -164,7 +165,7 @@ int main(int argc, char **argv, char **envp) sharp_global_init(); - nexthop_group_init(NULL, NULL, NULL, NULL); + sharp_nhgroup_init(); vrf_init(NULL, NULL, NULL, NULL, NULL); sharp_zebra_init(); diff --git a/sharpd/sharp_nht.c b/sharpd/sharp_nht.c index 174f186863..4148792900 100644 --- a/sharpd/sharp_nht.c +++ b/sharpd/sharp_nht.c @@ -25,11 +25,15 @@ #include "nexthop.h" #include "nexthop_group.h" #include "vty.h" +#include "typesafe.h" +#include "zclient.h" #include "sharp_nht.h" #include "sharp_globals.h" +#include "sharp_zebra.h" DEFINE_MTYPE_STATIC(SHARPD, NH_TRACKER, "Nexthop Tracker") +DEFINE_MTYPE_STATIC(SHARPD, NHG, "Nexthop Group") struct sharp_nh_tracker *sharp_nh_tracker_get(struct prefix *p) { @@ -65,3 +69,109 @@ void sharp_nh_tracker_dump(struct vty *vty) nht->updates); } } + +PREDECL_RBTREE_UNIQ(sharp_nhg_rb); + +struct sharp_nhg { + struct sharp_nhg_rb_item mylistitem; + + uint32_t id; + + char name[256]; +}; + +static uint32_t nhg_id; + +static uint32_t sharp_get_next_nhid(void) +{ + zlog_debug("Id assigned: %u", nhg_id + 1); + return nhg_id++; +} + +struct sharp_nhg_rb_head nhg_head; + +static int sharp_nhg_compare_func(const struct sharp_nhg *a, + const struct sharp_nhg *b) +{ + return strncmp(a->name, b->name, strlen(a->name)); +} + +DECLARE_RBTREE_UNIQ(sharp_nhg_rb, struct sharp_nhg, mylistitem, + sharp_nhg_compare_func); + +static void sharp_nhgroup_add_cb(const char *name) +{ + struct sharp_nhg *snhg; + + snhg = XCALLOC(MTYPE_NHG, sizeof(*snhg)); + snhg->id = sharp_get_next_nhid(); + strncpy(snhg->name, name, sizeof(snhg->name)); + + sharp_nhg_rb_add(&nhg_head, snhg); + return; +} + +static void sharp_nhgroup_add_nexthop_cb(const struct nexthop_group_cmd *nhgc, + const struct nexthop *nhop) +{ + struct sharp_nhg lookup; + struct sharp_nhg *snhg; + + strncpy(lookup.name, nhgc->name, sizeof(lookup.name)); + snhg = sharp_nhg_rb_find(&nhg_head, &lookup); + + nhg_add(snhg->id, &nhgc->nhg); + return; +} + +static void sharp_nhgroup_del_nexthop_cb(const struct nexthop_group_cmd *nhgc, + const struct nexthop *nhop) +{ + struct sharp_nhg lookup; + struct sharp_nhg *snhg; + + strncpy(lookup.name, nhgc->name, sizeof(lookup.name)); + snhg = sharp_nhg_rb_find(&nhg_head, &lookup); + + nhg_add(snhg->id, &nhgc->nhg); + return; +} + +static void sharp_nhgroup_delete_cb(const char *name) +{ + struct sharp_nhg lookup; + struct sharp_nhg *snhg; + + strncpy(lookup.name, name, sizeof(lookup.name)); + snhg = sharp_nhg_rb_find(&nhg_head, &lookup); + if (!snhg) + return; + + nhg_del(snhg->id); + sharp_nhg_rb_del(&nhg_head, snhg); + XFREE(MTYPE_NHG, snhg); + return; +} + +uint32_t sharp_nhgroup_get_id(const char *name) +{ + struct sharp_nhg lookup; + struct sharp_nhg *snhg; + + strncpy(lookup.name, name, sizeof(lookup.name)); + snhg = sharp_nhg_rb_find(&nhg_head, &lookup); + if (!snhg) + return 0; + + return snhg->id; +} + +void sharp_nhgroup_init(void) +{ + sharp_nhg_rb_init(&nhg_head); + nhg_id = zclient_get_nhg_start(ZEBRA_ROUTE_SHARP); + + nexthop_group_init(sharp_nhgroup_add_cb, sharp_nhgroup_add_nexthop_cb, + sharp_nhgroup_del_nexthop_cb, + sharp_nhgroup_delete_cb); +} diff --git a/sharpd/sharp_nht.h b/sharpd/sharp_nht.h index 0b00774a81..27c0104583 100644 --- a/sharpd/sharp_nht.h +++ b/sharpd/sharp_nht.h @@ -35,4 +35,7 @@ struct sharp_nh_tracker { extern struct sharp_nh_tracker *sharp_nh_tracker_get(struct prefix *p); extern void sharp_nh_tracker_dump(struct vty *vty); + +extern uint32_t sharp_nhgroup_get_id(const char *name); +extern void sharp_nhgroup_init(void); #endif diff --git a/sharpd/sharp_vty.c b/sharpd/sharp_vty.c index d390ea8192..2903588c13 100644 --- a/sharpd/sharp_vty.c +++ b/sharpd/sharp_vty.c @@ -192,6 +192,7 @@ DEFPY (install_routes, struct vrf *vrf; struct prefix prefix; uint32_t rts; + uint32_t nhgid = 0; sg.r.total_routes = routes; sg.r.installed_routes = 0; @@ -244,6 +245,8 @@ DEFPY (install_routes, return CMD_WARNING; } + nhgid = sharp_nhgroup_get_id(nexthop_group); + sg.r.nhgid = nhgid; sg.r.nhop_group.nexthop = nhgc->nhg.nexthop; /* Use group's backup nexthop info if present */ @@ -297,8 +300,8 @@ DEFPY (install_routes, sg.r.vrf_id = vrf->vrf_id; rts = routes; sharp_install_routes_helper(&prefix, sg.r.vrf_id, sg.r.inst, - &sg.r.nhop_group, &sg.r.backup_nhop_group, - rts); + nhgid, &sg.r.nhop_group, + &sg.r.backup_nhop_group, rts); return CMD_SUCCESS; } diff --git a/sharpd/sharp_zebra.c b/sharpd/sharp_zebra.c index 08f5a07b7e..8e357f96c9 100644 --- a/sharpd/sharp_zebra.c +++ b/sharpd/sharp_zebra.c @@ -217,7 +217,7 @@ int sharp_install_lsps_helper(bool install_p, bool update_p, } void sharp_install_routes_helper(struct prefix *p, vrf_id_t vrf_id, - uint8_t instance, + uint8_t instance, uint32_t nhgid, const struct nexthop_group *nhg, const struct nexthop_group *backup_nhg, uint32_t routes) @@ -239,7 +239,7 @@ void sharp_install_routes_helper(struct prefix *p, vrf_id_t vrf_id, monotime(&sg.r.t_start); for (i = 0; i < routes; i++) { - route_add(p, vrf_id, (uint8_t)instance, nhg, backup_nhg); + route_add(p, vrf_id, (uint8_t)instance, nhgid, nhg, backup_nhg); if (v4) p->u.prefix4.s_addr = htonl(++temp); else @@ -288,6 +288,7 @@ static void handle_repeated(bool installed) if (!installed) { sg.r.installed_routes = 0; sharp_install_routes_helper(&p, sg.r.vrf_id, sg.r.inst, + sg.r.nhgid, &sg.r.nhop_group, &sg.r.backup_nhop_group, sg.r.total_routes); @@ -357,8 +358,34 @@ void vrf_label_add(vrf_id_t vrf_id, afi_t afi, mpls_label_t label) zclient_send_vrf_label(zclient, vrf_id, afi, label, ZEBRA_LSP_SHARP); } +void nhg_add(uint32_t id, const struct nexthop_group *nhg) +{ + struct zapi_nexthop nh_array[MULTIPATH_NUM]; + struct zapi_nexthop *api_nh; + uint16_t nexthop_num = 0; + struct nexthop *nh; + + for (ALL_NEXTHOPS_PTR(nhg, nh)) { + api_nh = &nh_array[nexthop_num]; + + zapi_nexthop_from_nexthop(api_nh, nh); + nexthop_num++; + } + + zclient_nhg_add(zclient, id, nexthop_num, nh_array); + + zclient_send_message(zclient); +} + +void nhg_del(uint32_t id) +{ + zclient_nhg_del(zclient, id); + zclient_send_message(zclient); +} + void route_add(const struct prefix *p, vrf_id_t vrf_id, - uint8_t instance, const struct nexthop_group *nhg, + uint8_t instance, uint32_t nhgid, + const struct nexthop_group *nhg, const struct nexthop_group *backup_nhg) { struct zapi_route api; @@ -376,14 +403,19 @@ void route_add(const struct prefix *p, vrf_id_t vrf_id, SET_FLAG(api.flags, ZEBRA_FLAG_ALLOW_RECURSION); SET_FLAG(api.message, ZAPI_MESSAGE_NEXTHOP); - for (ALL_NEXTHOPS_PTR(nhg, nh)) { - api_nh = &api.nexthops[i]; + if (nhgid) { + SET_FLAG(api.message, ZAPI_MESSAGE_NHG); + api.nhgid = nhgid; + } else { + for (ALL_NEXTHOPS_PTR(nhg, nh)) { + api_nh = &api.nexthops[i]; - zapi_nexthop_from_nexthop(api_nh, nh); + zapi_nexthop_from_nexthop(api_nh, nh); - i++; + i++; + } + api.nexthop_num = i; } - api.nexthop_num = i; /* Include backup nexthops, if present */ if (backup_nhg && backup_nhg->nexthop) { diff --git a/sharpd/sharp_zebra.h b/sharpd/sharp_zebra.h index 0a44fa694f..69d7343cc4 100644 --- a/sharpd/sharp_zebra.h +++ b/sharpd/sharp_zebra.h @@ -29,15 +29,17 @@ int sharp_zclient_create(uint32_t session_id); int sharp_zclient_delete(uint32_t session_id); extern void vrf_label_add(vrf_id_t vrf_id, afi_t afi, mpls_label_t label); +extern void nhg_add(uint32_t id, const struct nexthop_group *nhg); +extern void nhg_del(uint32_t id); extern void route_add(const struct prefix *p, vrf_id_t, uint8_t instance, - const struct nexthop_group *nhg, + uint32_t nhgid, const struct nexthop_group *nhg, const struct nexthop_group *backup_nhg); extern void route_delete(struct prefix *p, vrf_id_t vrf_id, uint8_t instance); extern void sharp_zebra_nexthop_watch(struct prefix *p, vrf_id_t vrf_id, bool import, bool watch, bool connected); extern void sharp_install_routes_helper(struct prefix *p, vrf_id_t vrf_id, - uint8_t instance, + uint8_t instance, uint32_t nhgid, const struct nexthop_group *nhg, const struct nexthop_group *backup_nhg, uint32_t routes); From 5b27c09d4e7e304845d89fe6de597d29a40d774e Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Thu, 30 Apr 2020 18:35:13 -0400 Subject: [PATCH 07/57] zebra: remove NHG unhashable flag and its code Remove the code for setting a NHG as unhashable. Originally this was to prevent us from attempting to put duplicates from the kernel in our hashtable. Now I think its better to not use them in the hashtable at all and only track them in the ID table. Routes will still be able to use them if they specify the ID explicitly when sending Zebra the route, but 'normal' routes we hash the nexthop group on will not. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 83 +++++------------------------------------------ zebra/zebra_vty.c | 3 -- 2 files changed, 9 insertions(+), 77 deletions(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index aabbd875ec..ac972012b2 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -960,30 +960,6 @@ static struct nhg_ctx *nhg_ctx_init(uint32_t id, struct nexthop *nh, return ctx; } -static bool zebra_nhg_contains_unhashable(struct nhg_hash_entry *nhe) -{ - struct nhg_connected *rb_node_dep = NULL; - - frr_each(nhg_connected_tree, &nhe->nhg_depends, rb_node_dep) { - if (CHECK_FLAG(rb_node_dep->nhe->flags, - NEXTHOP_GROUP_UNHASHABLE)) - return true; - } - - return false; -} - -static void zebra_nhg_set_unhashable(struct nhg_hash_entry *nhe) -{ - SET_FLAG(nhe->flags, NEXTHOP_GROUP_UNHASHABLE); - SET_FLAG(nhe->flags, NEXTHOP_GROUP_INSTALLED); - - flog(LOG_INFO, - EC_ZEBRA_DUPLICATE_NHG_MESSAGE, - "Nexthop Group with ID (%d) is a duplicate, therefore unhashable, ignoring", - nhe->id); -} - static void zebra_nhg_set_valid(struct nhg_hash_entry *nhe) { struct nhg_connected *rb_node_dep; @@ -1038,10 +1014,10 @@ static void zebra_nhg_release(struct nhg_hash_entry *nhe) if_nhg_dependents_del(nhe->ifp, nhe); /* - * If its unhashable, we didn't store it here and have to be + * If its not zebra created, we didn't store it here and have to be * sure we don't clear one thats actually being used. */ - if (!CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_UNHASHABLE)) + if (nhe->type == ZEBRA_ROUTE_NHG) hash_release(zrouter.nhgs, nhe); hash_release(zrouter.nhgs_id, nhe); @@ -1127,54 +1103,7 @@ static int nhg_ctx_process_new(struct nhg_ctx *ctx) nhe = zebra_nhg_find_nexthop(id, nhg_ctx_get_nh(ctx), afi, type); - if (nhe) { - if (id != nhe->id) { - struct nhg_hash_entry *kernel_nhe = NULL; - - /* Duplicate but with different ID from - * the kernel - */ - - /* The kernel allows duplicate nexthops - * as long as they have different IDs. - * We are ignoring those to prevent - * syncing problems with the kernel - * changes. - * - * We maintain them *ONLY* in the ID hash table to - * track them and set the flag to indicated - * their attributes are unhashable. - */ - - kernel_nhe = zebra_nhe_copy(nhe, id); - - if (IS_ZEBRA_DEBUG_NHG_DETAIL) - zlog_debug("%s: copying kernel nhe (%u), dup of %u", - __func__, id, nhe->id); - - zebra_nhg_insert_id(kernel_nhe); - zebra_nhg_set_unhashable(kernel_nhe); - } else if (zebra_nhg_contains_unhashable(nhe)) { - /* The group we got contains an unhashable/duplicated - * depend, so lets mark this group as unhashable as well - * and release it from the non-ID hash. - */ - if (IS_ZEBRA_DEBUG_NHG_DETAIL) - zlog_debug("%s: nhe %p (%u) unhashable", - __func__, nhe, nhe->id); - - hash_release(zrouter.nhgs, nhe); - zebra_nhg_set_unhashable(nhe); - } else { - /* It actually created a new nhe */ - if (IS_ZEBRA_DEBUG_NHG_DETAIL) - zlog_debug("%s: nhe %p (%u) is new", - __func__, nhe, nhe->id); - - SET_FLAG(nhe->flags, NEXTHOP_GROUP_VALID); - SET_FLAG(nhe->flags, NEXTHOP_GROUP_INSTALLED); - } - } else { + if (!nhe) { flog_err( EC_ZEBRA_TABLE_LOOKUP_FAILED, "Zebra failed to find or create a nexthop hash entry for ID (%u)", @@ -1182,6 +1111,12 @@ static int nhg_ctx_process_new(struct nhg_ctx *ctx) return -1; } + if (IS_ZEBRA_DEBUG_NHG_DETAIL) + zlog_debug("%s: nhe %p (%u) is new", __func__, nhe, nhe->id); + + SET_FLAG(nhe->flags, NEXTHOP_GROUP_VALID); + SET_FLAG(nhe->flags, NEXTHOP_GROUP_INSTALLED); + return 0; } diff --git a/zebra/zebra_vty.c b/zebra/zebra_vty.c index 0088b49512..ab96a5cf1f 100644 --- a/zebra/zebra_vty.c +++ b/zebra/zebra_vty.c @@ -1304,9 +1304,6 @@ static void show_nexthop_group_out(struct vty *vty, struct nhg_hash_entry *nhe) vty_out(vty, " RefCnt: %d\n", nhe->refcnt); vty_out(vty, " VRF: %s\n", vrf_id_to_name(nhe->vrf_id)); - if (CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_UNHASHABLE)) - vty_out(vty, " Duplicate - from kernel not hashable\n"); - if (CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_VALID)) { vty_out(vty, " Valid"); if (CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_INSTALLED)) From 0885b1e3d90b1fa4d84c7e7a5fb775ba397c4103 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Tue, 5 May 2020 15:57:35 -0400 Subject: [PATCH 08/57] zebra: implement protocol NHG Add/Del Implement the underlying zebra functionality to Add/Del an internal zebra and kernel NHG. These NHGs are managed by the upperlevel protocols that send them down via zapi messaging. They are not put into the overall zebra NHG hash table and only put into to the ID table. Therefore, different protos cannot and will not share NHGs. The proto is also set appropriately when sent to the kernel. Expand the separation of Zebra hashed/shared/created NHGs and proto created and mangaged NHGs. Signed-off-by: Stephen Worley --- lib/route_types.txt | 2 +- zebra/zapi_msg.c | 38 ++++++++-- zebra/zebra_nhg.c | 174 ++++++++++++++++++++++++++++++++++---------- zebra/zebra_nhg.h | 60 +++++++++++---- zebra/zebra_vty.c | 2 +- 5 files changed, 216 insertions(+), 60 deletions(-) diff --git a/lib/route_types.txt b/lib/route_types.txt index b549c11cfc..37cc2fb590 100644 --- a/lib/route_types.txt +++ b/lib/route_types.txt @@ -84,7 +84,7 @@ ZEBRA_ROUTE_PBR, pbr, pbrd, 'F', 1, 1, 0, "PBR" ZEBRA_ROUTE_BFD, bfd, bfdd, '-', 0, 0, 0, "BFD" ZEBRA_ROUTE_OPENFABRIC, openfabric, fabricd, 'f', 1, 1, 1, "OpenFabric" ZEBRA_ROUTE_VRRP, vrrp, vrrpd, '-', 0, 0, 0, "VRRP" -ZEBRA_ROUTE_NHG, nhg, none, '-', 0, 0, 0, "Nexthop Group" +ZEBRA_ROUTE_NHG, zebra, none, '-', 0, 0, 0, "Nexthop Group" ZEBRA_ROUTE_SRTE, srte, none, '-', 0, 0, 0, "SR-TE" ZEBRA_ROUTE_ALL, wildcard, none, '-', 0, 0, 0, "-" diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 7c00ae1032..dfcab080f1 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -1734,16 +1734,22 @@ static void zread_nhg_del(ZAPI_HANDLER_ARGS) struct stream *s = msg; uint32_t id; uint16_t proto; + struct nhg_hash_entry *nhe; STREAM_GETW(s, proto); STREAM_GETL(s, id); /* * Delete the received nhg id - * id is incremented to make compiler happy right now - * it should be removed in future code work. */ - nhg_notify(proto, client->instance, id, ZAPI_NHG_REMOVED); + + nhe = zebra_nhg_proto_del(id); + + if (nhe) { + zebra_nhg_uninstall_kernel(nhe); + nhg_notify(proto, client->instance, id, ZAPI_NHG_REMOVED); + } else + nhg_notify(proto, client->instance, id, ZAPI_NHG_REMOVE_FAIL); return; @@ -1762,6 +1768,7 @@ static void zread_nhg_reader(ZAPI_HANDLER_ARGS) struct nexthop_group *nhg = NULL; struct prefix p; uint16_t proto; + struct nhg_hash_entry *nhe; memset(&p, 0, sizeof(p)); @@ -1771,8 +1778,9 @@ static void zread_nhg_reader(ZAPI_HANDLER_ARGS) STREAM_GETL(s, id); STREAM_GETW(s, nhops); - if (zserv_nexthop_num_warn(__func__, &p, nhops)) - return; + // TODO: Can't use this without a prefix. + // if (zserv_nexthop_num_warn(__func__, &p, nhops)) + // return; for (i = 0; i < nhops; i++) { struct zapi_nexthop *znh = &zapi_nexthops[i]; @@ -1792,10 +1800,28 @@ static void zread_nhg_reader(ZAPI_HANDLER_ARGS) __func__); return; } + /* * Install the nhg */ - nhg_notify(proto, client->instance, id, ZAPI_NHG_INSTALLED); + + // TODO: Forcing AF_UNSPEC/AF_IP for now + nhe = zebra_nhg_proto_add(id, ZEBRA_ROUTE_BGP, nhg, + ((nhops > 1) ? AFI_UNSPEC : AFI_IP)); + + nexthop_group_delete(&nhg); + + /* + * TODO: + * Assume fully resolved for now and install. + * + * Resolution is going to need some more work. + */ + if (nhe) { + zebra_nhg_install_kernel(nhe); + nhg_notify(proto, client->instance, id, ZAPI_NHG_INSTALLED); + } else + nhg_notify(proto, client->instance, id, ZAPI_NHG_FAIL_INSTALL); return; diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index ac972012b2..59df45420b 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -54,13 +54,13 @@ uint32_t id_counter; /* */ static bool g_nexthops_enabled = true; -static struct nhg_hash_entry *depends_find(const struct nexthop *nh, - afi_t afi); +static struct nhg_hash_entry *depends_find(const struct nexthop *nh, afi_t afi, + int type); static void depends_add(struct nhg_connected_tree_head *head, struct nhg_hash_entry *depend); static struct nhg_hash_entry * depends_find_add(struct nhg_connected_tree_head *head, struct nexthop *nh, - afi_t afi); + afi_t afi, int type); static struct nhg_hash_entry * depends_find_id_add(struct nhg_connected_tree_head *head, uint32_t id); static void depends_decrement_free(struct nhg_connected_tree_head *head); @@ -431,7 +431,6 @@ static void *zebra_nhg_hash_alloc(void *arg) nhe->nhg.nexthop->vrf_id, nhe->id); } - zebra_nhg_insert_id(nhe); return nhe; } @@ -439,17 +438,17 @@ static void *zebra_nhg_hash_alloc(void *arg) uint32_t zebra_nhg_hash_key(const void *arg) { const struct nhg_hash_entry *nhe = arg; - uint32_t val, key = 0x5a351234; + uint32_t key = 0x5a351234; + uint32_t primary = 0; + uint32_t backup = 0; - val = nexthop_group_hash(&(nhe->nhg)); - if (nhe->backup_info) { - val = jhash_2words(val, - nexthop_group_hash( - &(nhe->backup_info->nhe->nhg)), - key); - } + primary = nexthop_group_hash(&(nhe->nhg)); + if (nhe->backup_info) + backup = nexthop_group_hash(&(nhe->backup_info->nhe->nhg)); - key = jhash_3words(nhe->vrf_id, nhe->afi, val, key); + key = jhash_3words(primary, backup, nhe->type, key); + + key = jhash_2words(nhe->vrf_id, nhe->afi, key); return key; } @@ -512,6 +511,9 @@ bool zebra_nhg_hash_equal(const void *arg1, const void *arg2) if (nhe1->id && nhe2->id && (nhe1->id == nhe2->id)) return true; + if (nhe1->type != nhe2->type) + return false; + if (nhe1->vrf_id != nhe2->vrf_id) return false; @@ -611,7 +613,7 @@ static int zebra_nhg_process_grp(struct nexthop_group *nhg, } static void handle_recursive_depend(struct nhg_connected_tree_head *nhg_depends, - struct nexthop *nh, afi_t afi) + struct nexthop *nh, afi_t afi, int type) { struct nhg_hash_entry *depend = NULL; struct nexthop_group resolved_ng = {}; @@ -622,7 +624,7 @@ static void handle_recursive_depend(struct nhg_connected_tree_head *nhg_depends, zlog_debug("%s: head %p, nh %pNHv", __func__, nhg_depends, nh); - depend = zebra_nhg_rib_find(0, &resolved_ng, afi); + depend = zebra_nhg_rib_find(0, &resolved_ng, afi, type); if (IS_ZEBRA_DEBUG_NHG_DETAIL) zlog_debug("%s: nh %pNHv => %p (%u)", @@ -672,7 +674,25 @@ static bool zebra_nhe_find(struct nhg_hash_entry **nhe, /* return value */ */ if (lookup->id == 0) lookup->id = ++id_counter; - newnhe = hash_get(zrouter.nhgs, lookup, zebra_nhg_hash_alloc); + + if (ZEBRA_OWNED(lookup)) { + /* + * This is a zebra hashed/owned NHG. + * + * It goes in HASH and ID table. + */ + newnhe = hash_get(zrouter.nhgs, lookup, zebra_nhg_hash_alloc); + zebra_nhg_insert_id(newnhe); + } else { + /* + * This is upperproto owned NHG and should not be hashed to. + * + * It goes in ID table. + */ + newnhe = + hash_get(zrouter.nhgs_id, lookup, zebra_nhg_hash_alloc); + } + created = true; /* Mail back the new object */ @@ -713,7 +733,8 @@ static bool zebra_nhe_find(struct nhg_hash_entry **nhe, /* return value */ if (CHECK_FLAG(nh->flags, NEXTHOP_FLAG_RECURSIVE)) { /* Single recursive nexthop */ handle_recursive_depend(&newnhe->nhg_depends, - nh->resolved, afi); + nh->resolved, afi, + newnhe->type); recursive = true; } } else { @@ -726,7 +747,8 @@ static bool zebra_nhe_find(struct nhg_hash_entry **nhe, /* return value */ NEXTHOP_FLAG_RECURSIVE) ? "(R)" : ""); - depends_find_add(&newnhe->nhg_depends, nh, afi); + depends_find_add(&newnhe->nhg_depends, nh, afi, + newnhe->type); } } @@ -753,8 +775,8 @@ static bool zebra_nhe_find(struct nhg_hash_entry **nhe, /* return value */ __func__, nh); /* Single recursive nexthop */ - handle_recursive_depend(&backup_nhe->nhg_depends, - nh->resolved, afi); + handle_recursive_depend(&backup_nhe->nhg_depends, nh->resolved, + afi, backup_nhe->type); recursive = true; } else { /* One or more backup NHs */ @@ -766,8 +788,8 @@ static bool zebra_nhe_find(struct nhg_hash_entry **nhe, /* return value */ NEXTHOP_FLAG_RECURSIVE) ? "(R)" : ""); - depends_find_add(&backup_nhe->nhg_depends, - nh, afi); + depends_find_add(&backup_nhe->nhg_depends, nh, afi, + backup_nhe->type); } } @@ -1014,10 +1036,10 @@ static void zebra_nhg_release(struct nhg_hash_entry *nhe) if_nhg_dependents_del(nhe->ifp, nhe); /* - * If its not zebra created, we didn't store it here and have to be + * If its not zebra owned, we didn't store it here and have to be * sure we don't clear one thats actually being used. */ - if (nhe->type == ZEBRA_ROUTE_NHG) + if (ZEBRA_OWNED(nhe)) hash_release(zrouter.nhgs, nhe); hash_release(zrouter.nhgs_id, nhe); @@ -1093,8 +1115,8 @@ static int nhg_ctx_process_new(struct nhg_ctx *ctx) return -ENOENT; } - if (!zebra_nhg_find(&nhe, id, nhg, &nhg_depends, vrf_id, type, - afi)) + if (!zebra_nhg_find(&nhe, id, nhg, &nhg_depends, vrf_id, afi, + type)) depends_decrement_free(&nhg_depends); /* These got copied over in zebra_nhg_alloc() */ @@ -1261,14 +1283,14 @@ int zebra_nhg_kernel_del(uint32_t id, vrf_id_t vrf_id) /* Some dependency helper functions */ static struct nhg_hash_entry *depends_find_recursive(const struct nexthop *nh, - afi_t afi) + afi_t afi, int type) { struct nhg_hash_entry *nhe; struct nexthop *lookup = NULL; lookup = nexthop_dup(nh, NULL); - nhe = zebra_nhg_find_nexthop(0, lookup, afi, 0); + nhe = zebra_nhg_find_nexthop(0, lookup, afi, type); nexthops_free(lookup); @@ -1276,7 +1298,7 @@ static struct nhg_hash_entry *depends_find_recursive(const struct nexthop *nh, } static struct nhg_hash_entry *depends_find_singleton(const struct nexthop *nh, - afi_t afi) + afi_t afi, int type) { struct nhg_hash_entry *nhe; struct nexthop lookup = {}; @@ -1286,7 +1308,7 @@ static struct nhg_hash_entry *depends_find_singleton(const struct nexthop *nh, */ nexthop_copy_no_recurse(&lookup, nh, NULL); - nhe = zebra_nhg_find_nexthop(0, &lookup, afi, 0); + nhe = zebra_nhg_find_nexthop(0, &lookup, afi, type); /* The copy may have allocated labels; free them if necessary. */ nexthop_del_labels(&lookup); @@ -1298,7 +1320,8 @@ static struct nhg_hash_entry *depends_find_singleton(const struct nexthop *nh, return nhe; } -static struct nhg_hash_entry *depends_find(const struct nexthop *nh, afi_t afi) +static struct nhg_hash_entry *depends_find(const struct nexthop *nh, afi_t afi, + int type) { struct nhg_hash_entry *nhe = NULL; @@ -1309,9 +1332,9 @@ static struct nhg_hash_entry *depends_find(const struct nexthop *nh, afi_t afi) * in the non-recursive case (by not alloc/freeing) */ if (CHECK_FLAG(nh->flags, NEXTHOP_FLAG_RECURSIVE)) - nhe = depends_find_recursive(nh, afi); + nhe = depends_find_recursive(nh, afi, type); else - nhe = depends_find_singleton(nh, afi); + nhe = depends_find_singleton(nh, afi, type); if (IS_ZEBRA_DEBUG_NHG_DETAIL) { @@ -1344,11 +1367,11 @@ static void depends_add(struct nhg_connected_tree_head *head, static struct nhg_hash_entry * depends_find_add(struct nhg_connected_tree_head *head, struct nexthop *nh, - afi_t afi) + afi_t afi, int type) { struct nhg_hash_entry *depend = NULL; - depend = depends_find(nh, afi); + depend = depends_find(nh, afi, type); if (IS_ZEBRA_DEBUG_NHG_DETAIL) zlog_debug("%s: nh %pNHv => %p", @@ -1380,8 +1403,9 @@ static void depends_decrement_free(struct nhg_connected_tree_head *head) } /* Find an nhe based on a list of nexthops */ -struct nhg_hash_entry * -zebra_nhg_rib_find(uint32_t id, struct nexthop_group *nhg, afi_t rt_afi) +struct nhg_hash_entry *zebra_nhg_rib_find(uint32_t id, + struct nexthop_group *nhg, + afi_t rt_afi, int type) { struct nhg_hash_entry *nhe = NULL; vrf_id_t vrf_id; @@ -1393,7 +1417,7 @@ zebra_nhg_rib_find(uint32_t id, struct nexthop_group *nhg, afi_t rt_afi) assert(nhg->nexthop); vrf_id = !vrf_is_backend_netns() ? VRF_DEFAULT : nhg->nexthop->vrf_id; - zebra_nhg_find(&nhe, id, nhg, NULL, vrf_id, rt_afi, 0); + zebra_nhg_find(&nhe, id, nhg, NULL, vrf_id, rt_afi, type); if (IS_ZEBRA_DEBUG_NHG_DETAIL) zlog_debug("%s: => nhe %p (%u)", @@ -2478,7 +2502,8 @@ void zebra_nhg_install_kernel(struct nhg_hash_entry *nhe) && !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; + if (!ZEBRA_NHG_CREATED(nhe)) + nhe->type = ZEBRA_ROUTE_NHG; int ret = dplane_nexthop_add(nhe); @@ -2635,3 +2660,74 @@ bool zebra_nhg_kernel_nexthops_enabled(void) { return g_nexthops_enabled; } + +/* Add NHE from upper level proto */ +struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, + struct nexthop_group *nhg, afi_t afi) +{ + struct nhg_hash_entry lookup; + struct nhg_hash_entry *new; + struct nhg_connected *rb_node_dep = NULL; + + zebra_nhe_init(&lookup, afi, nhg->nexthop); + lookup.nhg.nexthop = nhg->nexthop; + lookup.id = id; + lookup.type = type; + + new = zebra_nhg_rib_find_nhe(&lookup, afi); + + if (!new) + return NULL; + + /* TODO: Assuming valid/onlink for now */ + SET_FLAG(new->flags, NEXTHOP_GROUP_VALID); + + if (!zebra_nhg_depends_is_empty(new)) { + frr_each (nhg_connected_tree, &new->nhg_depends, rb_node_dep) + SET_FLAG(rb_node_dep->nhe->flags, NEXTHOP_GROUP_VALID); + } + + if (IS_ZEBRA_DEBUG_NHG_DETAIL) + zlog_debug("%s: added nhe %p (%u), vrf %d, type %s", __func__, + new, new->id, new->vrf_id, + zebra_route_string(new->type)); + + return new; +} + +/* Delete NHE from upper level proto */ +struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id) +{ + struct nhg_hash_entry *nhe; + + nhe = zebra_nhg_lookup_id(id); + + if (!nhe) { + if (IS_ZEBRA_DEBUG_NHG_DETAIL) + zlog_debug("%s: id %u, lookup failed", __func__, id); + + return NULL; + } + + if (nhe->refcnt) { + /* TODO: should be warn? */ + if (IS_ZEBRA_DEBUG_NHG) + zlog_debug("%s: id %u, still being used refcnt %u", + __func__, nhe->id, nhe->refcnt); + return NULL; + } + + if (IS_ZEBRA_DEBUG_NHG_DETAIL) + zlog_debug("%s: deleted nhe %p (%u), vrf %d, type %s", __func__, + nhe, nhe->id, nhe->vrf_id, + zebra_route_string(nhe->type)); + + return nhe; +} + +/* Replace NHE from upper level proto */ +struct nhg_hash_entry * +zebra_nhg_proto_replace(uint32_t id, struct nexthop_group *nhg, afi_t afi) +{ + return NULL; +} diff --git a/zebra/zebra_nhg.h b/zebra/zebra_nhg.h index de5f097472..44d768648f 100644 --- a/zebra/zebra_nhg.h +++ b/zebra/zebra_nhg.h @@ -102,29 +102,25 @@ struct nhg_hash_entry { * Is this a nexthop that is recursively resolved? */ #define NEXTHOP_GROUP_RECURSIVE (1 << 3) -/* - * This is a nexthop group we got from the kernel, it is identical to - * one we already have. (The kernel allows duplicate nexthops, we don't - * since we hash on them). We are only tracking it in our ID table, - * it is unusable by our created routes but may be used by routes we get - * from the kernel. Therefore, it is unhashable. - */ -#define NEXTHOP_GROUP_UNHASHABLE (1 << 4) /* * Backup nexthop support - identify groups that are backups for * another group. */ -#define NEXTHOP_GROUP_BACKUP (1 << 5) +#define NEXTHOP_GROUP_BACKUP (1 << 4) /* * Track FPM installation status.. */ -#define NEXTHOP_GROUP_FPM (1 << 6) +#define NEXTHOP_GROUP_FPM (1 << 5) }; /* Was this one we created, either this session or previously? */ -#define ZEBRA_NHG_CREATED(NHE) ((NHE->type) == ZEBRA_ROUTE_NHG) +#define ZEBRA_NHG_CREATED(NHE) \ + (((NHE->type) <= ZEBRA_ROUTE_MAX) && (NHE->type != ZEBRA_ROUTE_KERNEL)) + +/* Is this an NHE owned by zebra and not an upper level protocol? */ +#define ZEBRA_OWNED(NHE) (NHE->type == ZEBRA_ROUTE_NHG) /* * Backup nexthops: this is a group object itself, so @@ -249,13 +245,51 @@ extern int zebra_nhg_kernel_find(uint32_t id, struct nexthop *nh, extern int zebra_nhg_kernel_del(uint32_t id, vrf_id_t vrf_id); /* Find an nhe based on a nexthop_group */ -extern struct nhg_hash_entry * -zebra_nhg_rib_find(uint32_t id, struct nexthop_group *nhg, afi_t rt_afi); +extern struct nhg_hash_entry *zebra_nhg_rib_find(uint32_t id, + struct nexthop_group *nhg, + afi_t rt_afi, int type); /* Find an nhe based on a route's nhe, used during route creation */ struct nhg_hash_entry * zebra_nhg_rib_find_nhe(struct nhg_hash_entry *rt_nhe, afi_t rt_afi); + +/** + * Functions for Add/Del/Replace via protocol NHG creation. + * + * The NHEs will not be hashed. They will only be present in the + * ID table and therefore not sharable. + * + * It is the owning protocols job to manage these. + */ + +/* + * Add NHE. + * + * Returns allocated NHE on success, otherwise NULL. + */ +struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, + struct nexthop_group *nhg, + afi_t afi); + + +/* + * Del NHE. + * + * Returns deleted NHE on success, otherwise NULL. + * + * Caller must free the NHE. + */ +struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id); + +/* + * Replace NHE. + * + * Returns new NHE on success, otherwise NULL. + */ +struct nhg_hash_entry * +zebra_nhg_proto_replace(uint32_t id, struct nexthop_group *nhg, afi_t afi); + /* Reference counter functions */ extern void zebra_nhg_decrement_ref(struct nhg_hash_entry *nhe); extern void zebra_nhg_increment_ref(struct nhg_hash_entry *nhe); diff --git a/zebra/zebra_vty.c b/zebra/zebra_vty.c index ab96a5cf1f..baf7d2c14d 100644 --- a/zebra/zebra_vty.c +++ b/zebra/zebra_vty.c @@ -1300,7 +1300,7 @@ static void show_nexthop_group_out(struct vty *vty, struct nhg_hash_entry *nhe) struct nhg_connected *rb_node_dep = NULL; struct nexthop_group *backup_nhg; - vty_out(vty, "ID: %u\n", nhe->id); + vty_out(vty, "ID: %u (%s)\n", nhe->id, zebra_route_string(nhe->type)); vty_out(vty, " RefCnt: %d\n", nhe->refcnt); vty_out(vty, " VRF: %s\n", vrf_id_to_name(nhe->vrf_id)); From 6c67f41f9e39bca29416c38eecc74ec729d1d60f Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Wed, 13 May 2020 12:50:14 -0700 Subject: [PATCH 09/57] zebra,lib: command to only install proto-based nexthops Add a command/functionality to only install proto-based nexthops. That is nexthops owned/created by upper level protocols, not ones implicitly created by zebra. There are some scenarios where you would not want zebra to be arbitrarily installing nexthop groups and but you still want to use ones you have control over via lib/nexthop_group config and an upper level protocol. Signed-off-by: Stephen Worley --- lib/zclient.c | 8 ++++++++ lib/zclient.h | 1 + zebra/rt_netlink.c | 40 +++++++++++++++++++++++++++++++++++++++- zebra/zebra_nhg.c | 19 +++++++++++++++++++ zebra/zebra_nhg.h | 4 ++++ zebra/zebra_vty.c | 17 +++++++++++++++++ 6 files changed, 88 insertions(+), 1 deletion(-) diff --git a/lib/zclient.c b/lib/zclient.c index 17026d13a6..deb5f519bd 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -4083,3 +4083,11 @@ uint32_t zclient_get_nhg_start(uint32_t proto) return ZEBRA_NHG_SPACING * proto; } + +/* + * Where do routing protocols IDs start overall (first ID after zebra) + */ +uint32_t zclient_get_nhg_lower_bound() +{ + return ZEBRA_NHG_SPACING * (ZEBRA_ROUTE_CONNECT + 1); +} diff --git a/lib/zclient.h b/lib/zclient.h index b7850cdec7..db5e1ce5b9 100644 --- a/lib/zclient.h +++ b/lib/zclient.h @@ -692,6 +692,7 @@ extern struct zclient_options zclient_options_default; */ #define ZEBRA_NHG_SPACING 50000000 extern uint32_t zclient_get_nhg_start(uint32_t proto); +extern uint32_t zclient_get_nhg_lower_bound(void); extern struct zclient *zclient_new(struct thread_master *m, struct zclient_options *opt); diff --git a/zebra/rt_netlink.c b/zebra/rt_netlink.c index 146650f602..07135b7fc8 100644 --- a/zebra/rt_netlink.c +++ b/zebra/rt_netlink.c @@ -125,6 +125,31 @@ static bool kernel_nexthops_supported(void) && zebra_nhg_kernel_nexthops_enabled()); } +/* + * Some people may only want to use NHGs created by protos and not + * implicitly created by Zebra. This check accounts for that. + */ +static bool proto_nexthops_only(void) +{ + return zebra_nhg_proto_nexthops_only(); +} + +/* Is this a proto created NHG? */ +static bool is_proto_nhg(uint32_t id, int type) +{ + /* If type is available, use it as the source of truth */ + if (type) { + if (type != ZEBRA_ROUTE_NHG) + return true; + return false; + } + + if (id >= zclient_get_nhg_lower_bound()) + return true; + + return false; +} + /* * The ipv4_ll data structure is used for all 5549 * additions to the kernel. Let's figure out the @@ -1748,7 +1773,10 @@ ssize_t netlink_route_multipath_msg_encode(int cmd, nl_attr_nest_end(&req->n, nest); } - if ((!fpm && kernel_nexthops_supported()) || (fpm && force_nhg)) { + if ((!fpm && kernel_nexthops_supported() + && (!proto_nexthops_only() + || is_proto_nhg(dplane_ctx_get_nhe_id(ctx), 0))) + || (fpm && force_nhg)) { /* Kernel supports nexthop objects */ if (IS_ZEBRA_DEBUG_KERNEL) zlog_debug("%s: %pFX nhg_id is %u", __func__, p, @@ -2073,6 +2101,16 @@ ssize_t netlink_nexthop_msg_encode(uint16_t cmd, char label_buf[256]; int num_labels = 0; + /* + * Nothing to do if the kernel doesn't support nexthop objects or + * we dont want to install this type of NHG + */ + if (!kernel_nexthops_supported() + || (proto_nexthops_only() + && !is_proto_nhg(dplane_ctx_get_nhe_id(ctx), + dplane_ctx_get_nhe_type(ctx)))) + return 0; + label_buf[0] = '\0'; if (buflen < sizeof(*req)) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 59df45420b..dbf5adafe1 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -53,6 +53,7 @@ uint32_t id_counter; /* */ static bool g_nexthops_enabled = true; +static bool proto_nexthops_only = false; static struct nhg_hash_entry *depends_find(const struct nexthop *nh, afi_t afi, int type); @@ -2661,6 +2662,24 @@ bool zebra_nhg_kernel_nexthops_enabled(void) return g_nexthops_enabled; } +/* + * Global control to only use kernel nexthops for protocol created NHGs. + * There are some use cases where you may not want zebra to implicitly + * create kernel nexthops for all routes and only create them for NHGs + * passed down by upper level protos. + * + * Default is off. + */ +void zebra_nhg_set_proto_nexthops_only(bool set) +{ + proto_nexthops_only = set; +} + +bool zebra_nhg_proto_nexthops_only(void) +{ + return proto_nexthops_only; +} + /* Add NHE from upper level proto */ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, struct nexthop_group *nhg, afi_t afi) diff --git a/zebra/zebra_nhg.h b/zebra/zebra_nhg.h index 44d768648f..02858779b3 100644 --- a/zebra/zebra_nhg.h +++ b/zebra/zebra_nhg.h @@ -182,6 +182,10 @@ struct nhg_ctx { void zebra_nhg_enable_kernel_nexthops(bool set); bool zebra_nhg_kernel_nexthops_enabled(void); +/* Global control for zebra to only use proto-owned nexthops */ +void zebra_nhg_set_proto_nexthops_only(bool set); +bool zebra_nhg_proto_nexthops_only(void); + /** * NHE abstracted tree functions. * Use these where possible instead of direct access. diff --git a/zebra/zebra_vty.c b/zebra/zebra_vty.c index baf7d2c14d..7e73b3f724 100644 --- a/zebra/zebra_vty.c +++ b/zebra/zebra_vty.c @@ -1566,6 +1566,19 @@ DEFPY_HIDDEN(nexthop_group_use_enable, return CMD_SUCCESS; } +DEFPY_HIDDEN (proto_nexthop_group_only, + proto_nexthop_group_only_cmd, + "[no] zebra nexthop proto only", + NO_STR + ZEBRA_STR + "Nexthop configuration\n" + "Configure exclusive use of proto nexthops\n" + "Only use proto nexthops\n") +{ + zebra_nhg_set_proto_nexthops_only(!no); + return CMD_SUCCESS; +} + DEFUN (no_ip_nht_default_route, no_ip_nht_default_route_cmd, "no ip nht resolve-via-default", @@ -3448,6 +3461,9 @@ static int config_write_protocol(struct vty *vty) if (!zebra_nhg_kernel_nexthops_enabled()) vty_out(vty, "no zebra nexthop kernel enable\n"); + if (zebra_nhg_proto_nexthops_only()) + vty_out(vty, "zebra nexthop proto only\n"); + #ifdef HAVE_NETLINK /* Include netlink info */ netlink_config_write_helper(vty); @@ -3882,6 +3898,7 @@ void zebra_vty_init(void) install_element(CONFIG_NODE, &zebra_packet_process_cmd); install_element(CONFIG_NODE, &no_zebra_packet_process_cmd); install_element(CONFIG_NODE, &nexthop_group_use_enable_cmd); + install_element(CONFIG_NODE, &proto_nexthop_group_only_cmd); install_element(VIEW_NODE, &show_nexthop_group_cmd); install_element(VIEW_NODE, &show_interface_nexthop_group_cmd); From 08da8bbc22f38a352bed729b041abc9bdc3eec6e Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Sun, 10 May 2020 16:34:36 -0400 Subject: [PATCH 10/57] zebra: hash proto-created but zebra ID spaced NHGS To prevent duplication of singleton NHGs, lets hash any zebra-ID spaced NHGs sent from an upper level proto. These would be singleton NHGs anyway and should prevent duplication of dataplane installs. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index dbf5adafe1..e5fa7b425a 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -676,7 +676,7 @@ static bool zebra_nhe_find(struct nhg_hash_entry **nhe, /* return value */ if (lookup->id == 0) lookup->id = ++id_counter; - if (ZEBRA_OWNED(lookup)) { + if (lookup->id < zclient_get_nhg_lower_bound()) { /* * This is a zebra hashed/owned NHG. * From dd1e105fe35c69da2649d72656ef68e1218699ae Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Sun, 10 May 2020 16:36:49 -0400 Subject: [PATCH 11/57] zebra: implement NHG proto replace Implement the ability to replace an NHG sent down from an upper level proto. With proto-owned NHGs, we make the assumption they are ecmp and always treat them as a group to make the replace from 1 -> 2 and 2 -> 1 quite a bit easier. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 43 ++++++++++++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 11 deletions(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index e5fa7b425a..9c45093f21 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -730,7 +730,7 @@ static bool zebra_nhe_find(struct nhg_hash_entry **nhe, /* return value */ if (CHECK_FLAG(nh->flags, NEXTHOP_FLAG_ACTIVE)) SET_FLAG(newnhe->flags, NEXTHOP_GROUP_VALID); - if (nh->next == NULL) { + if (nh->next == NULL && newnhe->id < zclient_get_nhg_lower_bound()) { if (CHECK_FLAG(nh->flags, NEXTHOP_FLAG_RECURSIVE)) { /* Single recursive nexthop */ handle_recursive_depend(&newnhe->nhg_depends, @@ -739,6 +739,7 @@ static bool zebra_nhe_find(struct nhg_hash_entry **nhe, /* return value */ recursive = true; } } else { + /* Proto-owned are groups by default */ /* List of nexthops */ for (nh = newnhe->nhg.nexthop; nh; nh = nh->next) { if (IS_ZEBRA_DEBUG_NHG_DETAIL) @@ -1024,17 +1025,21 @@ done: zebra_nhg_set_invalid(nhe); } +static void zebra_nhg_release_all_deps(struct nhg_hash_entry *nhe) +{ + /* Remove it from any lists it may be on */ + zebra_nhg_depends_release(nhe); + zebra_nhg_dependents_release(nhe); + if (nhe->ifp) + if_nhg_dependents_del(nhe->ifp, nhe); +} static void zebra_nhg_release(struct nhg_hash_entry *nhe) { if (IS_ZEBRA_DEBUG_NHG_DETAIL) zlog_debug("%s: nhe %p (%u)", __func__, nhe, nhe->id); - /* Remove it from any lists it may be on */ - zebra_nhg_depends_release(nhe); - zebra_nhg_dependents_release(nhe); - if (nhe->ifp) - if_nhg_dependents_del(nhe->ifp, nhe); + zebra_nhg_release_all_deps(nhe); /* * If its not zebra owned, we didn't store it here and have to be @@ -2500,7 +2505,8 @@ void zebra_nhg_install_kernel(struct nhg_hash_entry *nhe) } if (CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_VALID) - && !CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_INSTALLED) + && (!CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_INSTALLED) + || nhe->id >= zclient_get_nhg_lower_bound()) && !CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_QUEUED)) { /* Change its type to us since we are installing it */ if (!ZEBRA_NHG_CREATED(nhe)) @@ -2685,7 +2691,7 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, struct nexthop_group *nhg, afi_t afi) { struct nhg_hash_entry lookup; - struct nhg_hash_entry *new; + struct nhg_hash_entry *new, *old; struct nhg_connected *rb_node_dep = NULL; zebra_nhe_init(&lookup, afi, nhg->nexthop); @@ -2693,8 +2699,23 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, lookup.id = id; lookup.type = type; + old = zebra_nhg_lookup_id(id); + + if (old) { + /* + * This is a replace, just release NHE from ID for now, The + * depends/dependents may still be used in the replacement. + */ + hash_release(zrouter.nhgs_id, old); + } + new = zebra_nhg_rib_find_nhe(&lookup, afi); + if (old) { + /* Now release depends/dependents in old one */ + zebra_nhg_release_all_deps(old); + } + if (!new) return NULL; @@ -2707,9 +2728,9 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, } if (IS_ZEBRA_DEBUG_NHG_DETAIL) - zlog_debug("%s: added nhe %p (%u), vrf %d, type %s", __func__, - new, new->id, new->vrf_id, - zebra_route_string(new->type)); + zlog_debug("%s: %s nhe %p (%u), vrf %d, type %s", __func__, + (old ? "replaced" : "added"), new, new->id, + new->vrf_id, zebra_route_string(new->type)); return new; } From 50db3f2f1df221d1e3cdf450f672c4d558be66b6 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Sun, 10 May 2020 17:32:24 -0400 Subject: [PATCH 12/57] zebra: handle zapi routes with NHG ID set Add code to properly handle routes sent with NHG ID rather than a nexthop_group. For now, we separate this from backup nexthop handling since that should probably be added to the nhg_proto_add calls. Signed-off-by: Stephen Worley --- zebra/zapi_msg.c | 31 ++++++++++++++++--------------- zebra/zebra_rib.c | 6 +++--- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index dfcab080f1..1c2a9fb6a7 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -1832,16 +1832,6 @@ stream_failure: return; } -static bool zapi_msg_get_nhg(struct zapi_route *api, struct nexthop_group **ng) -{ - if (!CHECK_FLAG(api->message, ZAPI_MESSAGE_NHG)) - return false; - - /* TODO lookup the ng from api->nhgid */ - *ng = NULL; - return true; -} - static void zread_route_add(ZAPI_HANDLER_ARGS) { struct stream *s; @@ -1908,7 +1898,10 @@ static void zread_route_add(ZAPI_HANDLER_ARGS) zebra_route_string(client->proto), &api.prefix); } - if (zapi_msg_get_nhg(&api, &ng) + if (CHECK_FLAG(api.message, ZAPI_MESSAGE_NHG)) + re->nhe_id = api.nhgid; + + if (!re->nhe_id || !zapi_read_nexthops(client, &api.prefix, api.nexthops, api.flags, api.message, api.nexthop_num, api.backup_nexthop_num, &ng, NULL) @@ -1952,13 +1945,21 @@ static void zread_route_add(ZAPI_HANDLER_ARGS) return; } - /* Include backup info with the route. We use a temporary nhe here; + /* + * If we have an ID, this proto owns the NHG it sent along with the + * route, so we just send the ID into rib code with it. + * + * Havent figured out how to handle backup NHs with this yet, so lets + * keep that separate. + * Include backup info with the route. We use a temporary nhe here; * if this is a new/unknown nhe, a new copy will be allocated * and stored. */ - zebra_nhe_init(&nhe, afi, ng->nexthop); - nhe.nhg.nexthop = ng->nexthop; - nhe.backup_info = bnhg; + if (!re->nhe_id) { + zebra_nhe_init(&nhe, afi, ng->nexthop); + nhe.nhg.nexthop = ng->nexthop; + nhe.backup_info = bnhg; + } ret = rib_add_multipath_nhe(afi, api.safi, &api.prefix, src_p, re, &nhe); diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c index ff30de18a3..5a7967d817 100644 --- a/zebra/zebra_rib.c +++ b/zebra/zebra_rib.c @@ -2906,14 +2906,14 @@ int rib_add_multipath_nhe(afi_t afi, safi_t safi, struct prefix *p, if (!table) return -1; - if (re_nhe->id > 0) { - nhe = zebra_nhg_lookup_id(re_nhe->id); + if (re->nhe_id > 0) { + nhe = zebra_nhg_lookup_id(re->nhe_id); if (!nhe) { flog_err( EC_ZEBRA_TABLE_LOOKUP_FAILED, "Zebra failed to find the nexthop hash entry for id=%u in a route entry", - re_nhe->id); + re->nhe_id); return -1; } From 2c41ef8c1798540c5639f6a2508e72c861d3bede Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Sun, 10 May 2020 17:34:27 -0400 Subject: [PATCH 13/57] zebra: special handling for proto-NHG-based routes For now let's assume proto-NHG-based routes are good to go (we assume they are onlink/interface based anyway) and bypass route resolution altogether. Once we determine how to handle recursive nexthop-resolution for proto-NHGs we will revisit this. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 9c45093f21..617a3def62 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -2291,6 +2291,22 @@ static uint32_t nexthop_list_active_update(struct route_node *rn, return counter; } + +static uint32_t proto_nhg_nexthop_active_update(struct nexthop_group *nhg) +{ + struct nexthop *nh; + uint32_t curr_active = 0; + + /* Assume all active for now */ + + for (nh = nhg->nexthop; nh; nh = nh->next) { + SET_FLAG(nh->flags, NEXTHOP_FLAG_ACTIVE); + curr_active++; + } + + return curr_active; +} + /* * Iterate over all nexthops of the given RIB entry and refresh their * ACTIVE flag. If any nexthop is found to toggle the ACTIVE flag, @@ -2303,6 +2319,9 @@ int nexthop_active_update(struct route_node *rn, struct route_entry *re) struct nhg_hash_entry *curr_nhe; uint32_t curr_active = 0, backup_active = 0; + if (re->nhe->id >= zclient_get_nhg_lower_bound()) + return proto_nhg_nexthop_active_update(&re->nhe->nhg); + afi_t rt_afi = family2afi(rn->p.family); UNSET_FLAG(re->status, ROUTE_ENTRY_CHANGED); From 16b20ad0628ee98537c79ae7e018303ad1de8aca Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Sun, 10 May 2020 17:34:35 -0400 Subject: [PATCH 14/57] zebra: dont update counter if outside of zebra ID range When we receive a NHG from the kernel, we set the ID counter to that to avoid using IDs owned from the kernel. If we get one outside of zebra's range, lets not update it since its probably one we created and never deleted anyway. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 617a3def62..74551e31c4 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -1246,7 +1246,7 @@ int zebra_nhg_kernel_find(uint32_t id, struct nexthop *nh, struct nh_grp *grp, zlog_debug("%s: nh %pNHv, id %u, count %d", __func__, nh, id, (int)count); - if (id > id_counter) + if (id > id_counter && id < zclient_get_nhg_lower_bound()) /* Increase our counter so we don't try to create * an ID that already exists */ From 54c89c9377685e32010ab1f788b2a19c0bc3531b Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Wed, 13 May 2020 14:32:13 -0400 Subject: [PATCH 15/57] zebra: NHG ID bounds macros Determine the NHG ID spacing and lower bound with ZEBRA_ROUTE_MAX in macros. Directly set the upperbound to be the lower 28bits of the uint32_t ID space (the top 4 are reserved for l2-NHGs). Round that number down a bit to make it more even. Convert all former lower_bound calls to just use the macro. Signed-off-by: Stephen Worley --- lib/zclient.c | 10 +--------- lib/zclient.h | 15 +++++++++++---- zebra/rt_netlink.c | 2 +- zebra/zebra_nhg.c | 10 +++++----- 4 files changed, 18 insertions(+), 19 deletions(-) diff --git a/lib/zclient.c b/lib/zclient.c index deb5f519bd..b73dc688fb 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -4081,13 +4081,5 @@ uint32_t zclient_get_nhg_start(uint32_t proto) { assert(proto < ZEBRA_ROUTE_MAX); - return ZEBRA_NHG_SPACING * proto; -} - -/* - * Where do routing protocols IDs start overall (first ID after zebra) - */ -uint32_t zclient_get_nhg_lower_bound() -{ - return ZEBRA_NHG_SPACING * (ZEBRA_ROUTE_CONNECT + 1); + return ZEBRA_NHG_PROTO_SPACING * proto; } diff --git a/lib/zclient.h b/lib/zclient.h index db5e1ce5b9..3ac9ca9456 100644 --- a/lib/zclient.h +++ b/lib/zclient.h @@ -686,13 +686,20 @@ struct zclient_options { extern struct zclient_options zclient_options_default; /* + * We reserve the top 4 bits for l2-NHG, everything else + * is for zebra/proto l3-NHG. + * * Each client is going to get it's own nexthop group space - * and we'll separate them by 50 million, we'll figure out where - * to start based upon the route_types.h + * and we'll separate them, we'll figure out where to start based upon + * the route_types.h */ -#define ZEBRA_NHG_SPACING 50000000 +#define ZEBRA_NHG_PROTO_UPPER \ + ((uint32_t)250000000) /* Bottom 28 bits then rounded down */ +#define ZEBRA_NHG_PROTO_SPACING (ZEBRA_NHG_PROTO_UPPER / ZEBRA_ROUTE_MAX) +#define ZEBRA_NHG_PROTO_LOWER \ + (ZEBRA_NHG_PROTO_SPACING * (ZEBRA_ROUTE_CONNECT + 1)) + extern uint32_t zclient_get_nhg_start(uint32_t proto); -extern uint32_t zclient_get_nhg_lower_bound(void); extern struct zclient *zclient_new(struct thread_master *m, struct zclient_options *opt); diff --git a/zebra/rt_netlink.c b/zebra/rt_netlink.c index 07135b7fc8..34841aa4a5 100644 --- a/zebra/rt_netlink.c +++ b/zebra/rt_netlink.c @@ -144,7 +144,7 @@ static bool is_proto_nhg(uint32_t id, int type) return false; } - if (id >= zclient_get_nhg_lower_bound()) + if (id >= ZEBRA_NHG_PROTO_LOWER) return true; return false; diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 74551e31c4..c078b25ab1 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -676,7 +676,7 @@ static bool zebra_nhe_find(struct nhg_hash_entry **nhe, /* return value */ if (lookup->id == 0) lookup->id = ++id_counter; - if (lookup->id < zclient_get_nhg_lower_bound()) { + if (lookup->id < ZEBRA_NHG_PROTO_LOWER) { /* * This is a zebra hashed/owned NHG. * @@ -730,7 +730,7 @@ static bool zebra_nhe_find(struct nhg_hash_entry **nhe, /* return value */ if (CHECK_FLAG(nh->flags, NEXTHOP_FLAG_ACTIVE)) SET_FLAG(newnhe->flags, NEXTHOP_GROUP_VALID); - if (nh->next == NULL && newnhe->id < zclient_get_nhg_lower_bound()) { + if (nh->next == NULL && newnhe->id < ZEBRA_NHG_PROTO_LOWER) { if (CHECK_FLAG(nh->flags, NEXTHOP_FLAG_RECURSIVE)) { /* Single recursive nexthop */ handle_recursive_depend(&newnhe->nhg_depends, @@ -1246,7 +1246,7 @@ int zebra_nhg_kernel_find(uint32_t id, struct nexthop *nh, struct nh_grp *grp, zlog_debug("%s: nh %pNHv, id %u, count %d", __func__, nh, id, (int)count); - if (id > id_counter && id < zclient_get_nhg_lower_bound()) + if (id > id_counter && id < ZEBRA_NHG_PROTO_LOWER) /* Increase our counter so we don't try to create * an ID that already exists */ @@ -2319,7 +2319,7 @@ int nexthop_active_update(struct route_node *rn, struct route_entry *re) struct nhg_hash_entry *curr_nhe; uint32_t curr_active = 0, backup_active = 0; - if (re->nhe->id >= zclient_get_nhg_lower_bound()) + if (re->nhe->id >= ZEBRA_NHG_PROTO_LOWER) return proto_nhg_nexthop_active_update(&re->nhe->nhg); afi_t rt_afi = family2afi(rn->p.family); @@ -2525,7 +2525,7 @@ void zebra_nhg_install_kernel(struct nhg_hash_entry *nhe) if (CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_VALID) && (!CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_INSTALLED) - || nhe->id >= zclient_get_nhg_lower_bound()) + || nhe->id >= ZEBRA_NHG_PROTO_LOWER) && !CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_QUEUED)) { /* Change its type to us since we are installing it */ if (!ZEBRA_NHG_CREATED(nhe)) From 25645bd6017802cbcb628718a928599c42f72c72 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Wed, 13 May 2020 14:35:25 -0400 Subject: [PATCH 16/57] sharpd: print the correct ID the NHG is using We were incrementing in the output the ID value when we shouldnt be. The value the NHG is assigned is before its incremented. Signed-off-by: Stephen Worley --- sharpd/sharp_nht.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sharpd/sharp_nht.c b/sharpd/sharp_nht.c index 4148792900..731d58e560 100644 --- a/sharpd/sharp_nht.c +++ b/sharpd/sharp_nht.c @@ -84,7 +84,7 @@ static uint32_t nhg_id; static uint32_t sharp_get_next_nhid(void) { - zlog_debug("Id assigned: %u", nhg_id + 1); + zlog_debug("Id assigned: %u", nhg_id); return nhg_id++; } From ac5d1091dcb62484bf432e80023fa55239fc0d84 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Wed, 13 May 2020 17:42:55 -0400 Subject: [PATCH 17/57] zebra: make NHG ID allocation smarter Make NHG ID allocation smarter so it wraps once it hits the lower bound for protos and performs a lookup to make sure we don't already have that ID in use. Its pretty unlikely we would wrap since the ID space is somewhere around 24million for Zebra at this point in time. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index c078b25ab1..6d4a915673 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -69,6 +69,35 @@ static void depends_decrement_free(struct nhg_connected_tree_head *head); static struct nhg_backup_info * nhg_backup_copy(const struct nhg_backup_info *orig); +/* Helper function for getting the next allocatable ID */ +static uint32_t nhg_get_next_id() +{ + while (1) { + id_counter++; + + if (IS_ZEBRA_DEBUG_NHG_DETAIL) + zlog_debug("%s: ID %u checking", __func__, id_counter); + + if (id_counter == ZEBRA_NHG_PROTO_LOWER) { + if (IS_ZEBRA_DEBUG_NHG_DETAIL) + zlog_debug("%s: ID counter wrapped", __func__); + + id_counter = 0; + continue; + } + + if (zebra_nhg_lookup_id(id_counter)) { + if (IS_ZEBRA_DEBUG_NHG_DETAIL) + zlog_debug("%s: ID already exists", __func__); + + continue; + } + + break; + } + + return id_counter; +} static void nhg_connected_free(struct nhg_connected *dep) { @@ -674,7 +703,7 @@ static bool zebra_nhe_find(struct nhg_hash_entry **nhe, /* return value */ * assign the next global id value if necessary. */ if (lookup->id == 0) - lookup->id = ++id_counter; + lookup->id = nhg_get_next_id(); if (lookup->id < ZEBRA_NHG_PROTO_LOWER) { /* From cd53e3a6e6fd4dd9319fad0222b97fc564e9cb4c Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Wed, 13 May 2020 17:55:14 -0400 Subject: [PATCH 18/57] zebra: use the passed proto from zapi We were hard coding proto bgp for use with the NHG creation. Use the actual passed one from zapi now that it exists. Signed-off-by: Stephen Worley --- zebra/zapi_msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 1c2a9fb6a7..91c04893df 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -1806,7 +1806,7 @@ static void zread_nhg_reader(ZAPI_HANDLER_ARGS) */ // TODO: Forcing AF_UNSPEC/AF_IP for now - nhe = zebra_nhg_proto_add(id, ZEBRA_ROUTE_BGP, nhg, + nhe = zebra_nhg_proto_add(id, proto, nhg, ((nhops > 1) ? AFI_UNSPEC : AFI_IP)); nexthop_group_delete(&nhg); From 2b5ecd4ca6b5fb63cd3e2b82c647350a5259a42a Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Thu, 14 May 2020 17:24:46 -0400 Subject: [PATCH 19/57] zebra: fix route validity check with NHG ID Fix check in zread where we determine validity of a route based on reading in nexthops/checking ID is present. We had a bad conditional that was determining a route is bad if its not NHG ID based. Signed-off-by: Stephen Worley --- zebra/zapi_msg.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 91c04893df..4191e1ca39 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -1902,13 +1902,13 @@ static void zread_route_add(ZAPI_HANDLER_ARGS) re->nhe_id = api.nhgid; if (!re->nhe_id - || !zapi_read_nexthops(client, &api.prefix, api.nexthops, api.flags, - api.message, api.nexthop_num, - api.backup_nexthop_num, &ng, NULL) - || !zapi_read_nexthops(client, &api.prefix, api.backup_nexthops, - api.flags, api.message, - api.backup_nexthop_num, - api.backup_nexthop_num, NULL, &bnhg)) { + && (!zapi_read_nexthops(client, &api.prefix, api.nexthops, + api.flags, api.message, api.nexthop_num, + api.backup_nexthop_num, &ng, NULL) + || !zapi_read_nexthops(client, &api.prefix, api.backup_nexthops, + api.flags, api.message, + api.backup_nexthop_num, + api.backup_nexthop_num, NULL, &bnhg))) { XFREE(MTYPE_RE, re); return; } From e90284d77f556c1e94be2d8978b5ebb184b1e93b Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Mon, 18 May 2020 14:22:06 -0400 Subject: [PATCH 20/57] lib: add onlink flag to zapi_nh conversion helper Add setting the onlink flag to the zapi_nh conversion helper function so that we can set the onlink flag with it when passing down NHGs from upper level protos. Signed-off-by: Stephen Worley --- lib/zclient.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/zclient.c b/lib/zclient.c index b73dc688fb..cde2f5e052 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -1649,6 +1649,9 @@ int zapi_nexthop_from_nexthop(struct zapi_nexthop *znh, znh->ifindex = nh->ifindex; znh->gate = nh->gate; + if (CHECK_FLAG(nh->flags, NEXTHOP_FLAG_ONLINK)) + SET_FLAG(znh->flags, ZAPI_NEXTHOP_FLAG_ONLINK); + if (nh->nh_label && (nh->nh_label->num_labels > 0)) { /* Validate */ From 54c6fa8e0ac724aad6f9022f1689ff5c111a0e2e Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Mon, 18 May 2020 14:38:19 -0400 Subject: [PATCH 21/57] lib,doc: add `onlink` flag to nexthop group config Add an `onlink` flag to nexthop group configuration. Signed-off-by: Stephen Worley --- doc/user/pbr.rst | 2 +- lib/nexthop_group.c | 42 +++++++++++++++++++++++++++++------------- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/doc/user/pbr.rst b/doc/user/pbr.rst index 99ef258cb2..c869c6bc45 100644 --- a/doc/user/pbr.rst +++ b/doc/user/pbr.rst @@ -39,7 +39,7 @@ listing of ECMP nexthops used to forward packets for when a pbr-map is matched. sub-mode where you can specify individual nexthops. To exit this mode type exit or end as per normal conventions for leaving a sub-mode. -.. clicmd:: nexthop [A.B.C.D|X:X::X:XX] [interface] [nexthop-vrf NAME] [label LABELS] +.. clicmd:: nexthop [A.B.C.D|X:X::X:XX] [interface [onlink]] [nexthop-vrf NAME] [label LABELS] Create a v4 or v6 nexthop. All normal rules for creating nexthops that you are used to are allowed here. The syntax was intentionally kept the same as diff --git a/lib/nexthop_group.c b/lib/nexthop_group.c index 687cac4062..83905abe43 100644 --- a/lib/nexthop_group.c +++ b/lib/nexthop_group.c @@ -41,6 +41,7 @@ struct nexthop_hold { char *nhvrf_name; union sockunion *addr; char *intf; + bool onlink; char *labels; uint32_t weight; char *backup_str; @@ -560,6 +561,10 @@ static int nhgl_cmp(struct nexthop_hold *nh1, struct nexthop_hold *nh2) if (ret) return ret; + ret = ((int)nh2->onlink) - ((int)nh1->onlink); + if (ret) + return ret; + return nhgc_cmp_helper(nh1->labels, nh2->labels); } @@ -673,8 +678,8 @@ DEFPY(no_nexthop_group_backup, no_nexthop_group_backup_cmd, static void nexthop_group_save_nhop(struct nexthop_group_cmd *nhgc, const char *nhvrf_name, const union sockunion *addr, - const char *intf, const char *labels, - const uint32_t weight, + const char *intf, bool onlink, + const char *labels, const uint32_t weight, const char *backup_str) { struct nexthop_hold *nh; @@ -690,6 +695,8 @@ static void nexthop_group_save_nhop(struct nexthop_group_cmd *nhgc, if (labels) nh->labels = XSTRDUP(MTYPE_TMP, labels); + nh->onlink = onlink; + nh->weight = weight; if (backup_str) @@ -738,9 +745,10 @@ static void nexthop_group_unsave_nhop(struct nexthop_group_cmd *nhgc, */ static bool nexthop_group_parse_nexthop(struct nexthop *nhop, const union sockunion *addr, - const char *intf, const char *name, - const char *labels, int *lbl_ret, - uint32_t weight, const char *backup_str) + const char *intf, bool onlink, + const char *name, const char *labels, + int *lbl_ret, uint32_t weight, + const char *backup_str) { int ret = 0; struct vrf *vrf; @@ -764,6 +772,9 @@ static bool nexthop_group_parse_nexthop(struct nexthop *nhop, return false; } + if (onlink) + SET_FLAG(nhop->flags, NEXTHOP_FLAG_ONLINK); + if (addr) { if (addr->sa.sa_family == AF_INET) { nhop->gate.ipv4.s_addr = addr->sin.sin_addr.s_addr; @@ -820,15 +831,15 @@ static bool nexthop_group_parse_nexthop(struct nexthop *nhop, static bool nexthop_group_parse_nhh(struct nexthop *nhop, const struct nexthop_hold *nhh) { - return (nexthop_group_parse_nexthop(nhop, nhh->addr, nhh->intf, - nhh->nhvrf_name, nhh->labels, NULL, - nhh->weight, nhh->backup_str)); + return (nexthop_group_parse_nexthop( + nhop, nhh->addr, nhh->intf, nhh->onlink, nhh->nhvrf_name, + nhh->labels, NULL, nhh->weight, nhh->backup_str)); } DEFPY(ecmp_nexthops, ecmp_nexthops_cmd, "[no] nexthop\ <\ - $addr [INTERFACE$intf]\ + $addr [INTERFACE$intf [onlink$onlink]]\ |INTERFACE$intf\ >\ [{ \ @@ -842,6 +853,7 @@ DEFPY(ecmp_nexthops, ecmp_nexthops_cmd, "v4 Address\n" "v6 Address\n" "Interface to use\n" + "Treat nexthop as directly attached to the interface\n" "Interface to use\n" "If the nexthop is in a different vrf tell us\n" "The nexthop-vrf Name\n" @@ -870,8 +882,9 @@ DEFPY(ecmp_nexthops, ecmp_nexthops_cmd, } } - legal = nexthop_group_parse_nexthop(&nhop, addr, intf, vrf_name, label, - &lbl_ret, weight, backup_idx); + legal = nexthop_group_parse_nexthop(&nhop, addr, intf, !!onlink, + vrf_name, label, &lbl_ret, weight, + backup_idx); if (nhop.type == NEXTHOP_TYPE_IPV6 && IN6_IS_ADDR_LINKLOCAL(&nhop.gate.ipv6)) { @@ -933,8 +946,8 @@ DEFPY(ecmp_nexthops, ecmp_nexthops_cmd, } /* Save config always */ - nexthop_group_save_nhop(nhgc, vrf_name, addr, intf, label, - weight, backup_idx); + nexthop_group_save_nhop(nhgc, vrf_name, addr, intf, !!onlink, + label, weight, backup_idx); if (legal && nhg_hooks.add_nexthop) nhg_hooks.add_nexthop(nhgc, nh); @@ -1106,6 +1119,9 @@ static void nexthop_group_write_nexthop_internal(struct vty *vty, if (nh->intf) vty_out(vty, " %s", nh->intf); + if (nh->onlink) + vty_out(vty, " onlink"); + if (nh->nhvrf_name) vty_out(vty, " nexthop-vrf %s", nh->nhvrf_name); From df3cef24c596ddb631872f4270097154f50988ef Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sat, 16 May 2020 20:12:30 -0400 Subject: [PATCH 22/57] zebra: Prevent duplicate re-install If we have received a route that the already existing route is exactly the same, just note that it happened and move on. Signed-off-by: Donald Sharp --- zebra/zebra_dplane.c | 27 +++++++++++++++++++++++++++ zebra/zebra_dplane.h | 1 + 2 files changed, 28 insertions(+) diff --git a/zebra/zebra_dplane.c b/zebra/zebra_dplane.c index abd0adb64e..f2725adc54 100644 --- a/zebra/zebra_dplane.c +++ b/zebra/zebra_dplane.c @@ -73,6 +73,7 @@ const uint32_t DPLANE_DEFAULT_NEW_WORK = 100; */ struct dplane_nexthop_info { uint32_t id; + uint32_t old_id; afi_t afi; vrf_id_t vrf_id; int type; @@ -1242,6 +1243,12 @@ uint32_t dplane_ctx_get_nhe_id(const struct zebra_dplane_ctx *ctx) return ctx->u.rinfo.nhe.id; } +uint32_t dplane_ctx_get_old_nhe_id(const struct zebra_dplane_ctx *ctx) +{ + DPLANE_CTX_VALID(ctx); + return ctx->u.rinfo.nhe.old_id; +} + afi_t dplane_ctx_get_nhe_afi(const struct zebra_dplane_ctx *ctx) { DPLANE_CTX_VALID(ctx); @@ -1912,6 +1919,7 @@ int dplane_ctx_route_init(struct zebra_dplane_ctx *ctx, enum dplane_op_e op, struct nhg_hash_entry *nhe = zebra_nhg_resolve(re->nhe); ctx->u.rinfo.nhe.id = nhe->id; + ctx->u.rinfo.nhe.old_id = 0; /* * Check if the nhe is installed/queued before doing anything * with this route. @@ -2328,6 +2336,7 @@ dplane_route_update_internal(struct route_node *rn, ctx->u.rinfo.zd_old_instance = old_re->instance; ctx->u.rinfo.zd_old_distance = old_re->distance; ctx->u.rinfo.zd_old_metric = old_re->metric; + ctx->u.rinfo.nhe.old_id = old_re->nhe->id; #ifndef HAVE_NETLINK /* For bsd, capture previous re's nexthops too, sigh. @@ -2349,6 +2358,24 @@ dplane_route_update_internal(struct route_node *rn, #endif /* !HAVE_NETLINK */ } + /* + * If the old and new context type, and nexthop group id + * are the same there is no need to send down a route replace + * as that we know we have sent a nexthop group replace + * or an upper level protocol has sent us the exact + * same route again. + */ + if ((dplane_ctx_get_type(ctx) == dplane_ctx_get_old_type(ctx)) + && (dplane_ctx_get_nhe_id(ctx) + == dplane_ctx_get_old_nhe_id(ctx))) { + if (IS_ZEBRA_DEBUG_DPLANE) + zlog_debug( + "%s: Ignoring Route exactly the same", + __func__); + + return ZEBRA_DPLANE_REQUEST_SUCCESS; + } + /* Enqueue context for processing */ ret = dplane_update_enqueue(ctx); } diff --git a/zebra/zebra_dplane.h b/zebra/zebra_dplane.h index 1d852b1bac..fd70211f7c 100644 --- a/zebra/zebra_dplane.h +++ b/zebra/zebra_dplane.h @@ -316,6 +316,7 @@ dplane_ctx_get_old_backup_ng(const struct zebra_dplane_ctx *ctx); /* Accessors for nexthop information */ uint32_t dplane_ctx_get_nhe_id(const struct zebra_dplane_ctx *ctx); +uint32_t dplane_ctx_get_old_nhe_id(const struct zebra_dplane_ctx *ctx); afi_t dplane_ctx_get_nhe_afi(const struct zebra_dplane_ctx *ctx); vrf_id_t dplane_ctx_get_nhe_vrf_id(const struct zebra_dplane_ctx *ctx); int dplane_ctx_get_nhe_type(const struct zebra_dplane_ctx *ctx); From 2d8a9c544bd9bdfc6a32910f593ab65fbab2036c Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Wed, 20 May 2020 11:23:36 -0400 Subject: [PATCH 23/57] zebra: remove unneeded nhg repalce boilerplate Remove some leftover boilerplate from the old replace code path. That code ended up in the add API so its no longer needed. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 6 ------ zebra/zebra_nhg.h | 11 +---------- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 6d4a915673..d4d31d3ea2 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -2813,9 +2813,3 @@ struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id) return nhe; } -/* Replace NHE from upper level proto */ -struct nhg_hash_entry * -zebra_nhg_proto_replace(uint32_t id, struct nexthop_group *nhg, afi_t afi) -{ - return NULL; -} diff --git a/zebra/zebra_nhg.h b/zebra/zebra_nhg.h index 02858779b3..16474aada4 100644 --- a/zebra/zebra_nhg.h +++ b/zebra/zebra_nhg.h @@ -268,7 +268,7 @@ zebra_nhg_rib_find_nhe(struct nhg_hash_entry *rt_nhe, afi_t rt_afi); */ /* - * Add NHE. + * Add NHE. If already exists, Replace. * * Returns allocated NHE on success, otherwise NULL. */ @@ -276,7 +276,6 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, struct nexthop_group *nhg, afi_t afi); - /* * Del NHE. * @@ -286,14 +285,6 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, */ struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id); -/* - * Replace NHE. - * - * Returns new NHE on success, otherwise NULL. - */ -struct nhg_hash_entry * -zebra_nhg_proto_replace(uint32_t id, struct nexthop_group *nhg, afi_t afi); - /* Reference counter functions */ extern void zebra_nhg_decrement_ref(struct nhg_hash_entry *nhe); extern void zebra_nhg_increment_ref(struct nhg_hash_entry *nhe); From 6fae63d2bab3e5ee9f83458e32f4292165ef6cb6 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Wed, 20 May 2020 11:26:44 -0400 Subject: [PATCH 24/57] zebra: inc/dec refcount on add/del NHG proto When we add a proto NHG, increment the refcount, when we del a proto NHG, decrement the refcount rather than deleting it explicitly. If the upper level proto is handling it properly, it should get decremented to zero when we receive a NHG del. Signed-off-by: Stephen Worley --- zebra/zapi_msg.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 4191e1ca39..4ee4274657 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -1746,7 +1746,8 @@ static void zread_nhg_del(ZAPI_HANDLER_ARGS) nhe = zebra_nhg_proto_del(id); if (nhe) { - zebra_nhg_uninstall_kernel(nhe); + /* TODO: just decrement for now */ + zebra_nhg_decrement_ref(nhe); nhg_notify(proto, client->instance, id, ZAPI_NHG_REMOVED); } else nhg_notify(proto, client->instance, id, ZAPI_NHG_REMOVE_FAIL); @@ -1818,6 +1819,7 @@ static void zread_nhg_reader(ZAPI_HANDLER_ARGS) * Resolution is going to need some more work. */ if (nhe) { + zebra_nhg_increment_ref(nhe); zebra_nhg_install_kernel(nhe); nhg_notify(proto, client->instance, id, ZAPI_NHG_INSTALLED); } else From 24db1a7b9a289993439a8975558b53d6910adb5c Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Wed, 20 May 2020 15:41:18 -0400 Subject: [PATCH 25/57] zebra: handle proto NHG uninstall client disconnect Add code to handle proto-based NHG uninstalling after the owning client disconnects. This is handled the same way as rib_score_proto() but for now we are ignoring instance. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 37 +++++++++++++++++++++++++++++++++++++ zebra/zebra_nhg.h | 8 ++++++++ zebra/zserv.c | 8 ++++++++ 3 files changed, 53 insertions(+) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index d4d31d3ea2..407af2902d 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -2813,3 +2813,40 @@ struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id) return nhe; } +struct nhg_score_proto_iter { + int type; + unsigned long found; +}; + +static void zebra_nhg_score_proto_entry(struct hash_bucket *bucket, void *arg) +{ + struct nhg_hash_entry *nhe; + struct nhg_score_proto_iter *iter; + + nhe = (struct nhg_hash_entry *)bucket->data; + iter = arg; + + /* Needs to match type and outside zebra ID space */ + if (nhe->type == iter->type && nhe->id >= ZEBRA_NHG_PROTO_LOWER) { + if (IS_ZEBRA_DEBUG_NHG_DETAIL) + zlog_debug( + "%s: found nhe %p (%u), vrf %d, type %s after client disconnect", + __func__, nhe, nhe->id, nhe->vrf_id, + zebra_route_string(nhe->type)); + + /* This should be the last ref if we remove client routes too */ + zebra_nhg_decrement_ref(nhe); + } +} + +/* Remove specific by proto NHGs */ +unsigned long zebra_nhg_score_proto(int type) +{ + struct nhg_score_proto_iter iter = {}; + + iter.type = type; + + hash_iterate(zrouter.nhgs_id, zebra_nhg_score_proto_entry, &iter); + + return iter.found; +} diff --git a/zebra/zebra_nhg.h b/zebra/zebra_nhg.h index 16474aada4..8c33a91917 100644 --- a/zebra/zebra_nhg.h +++ b/zebra/zebra_nhg.h @@ -285,6 +285,14 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, */ struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id); +/* + * Remove specific by proto NHGs. + * + * Called after client disconnect. + * + */ +unsigned long zebra_nhg_score_proto(int type); + /* Reference counter functions */ extern void zebra_nhg_decrement_ref(struct nhg_hash_entry *nhe); extern void zebra_nhg_increment_ref(struct nhg_hash_entry *nhe); diff --git a/zebra/zserv.c b/zebra/zserv.c index 4c8656af0d..44f4641fcf 100644 --- a/zebra/zserv.c +++ b/zebra/zserv.c @@ -590,6 +590,7 @@ static void zserv_client_free(struct zserv *client) /* Close file descriptor. */ if (client->sock) { unsigned long nroutes; + unsigned long nnhgs; close(client->sock); @@ -600,6 +601,13 @@ static void zserv_client_free(struct zserv *client) "client %d disconnected %lu %s routes removed from the rib", client->sock, nroutes, zebra_route_string(client->proto)); + + /* Not worrying about instance for now */ + nnhgs = zebra_nhg_score_proto(client->proto); + zlog_notice( + "client %d disconnected %lu %s nhgs removed from the rib", + client->sock, nnhgs, + zebra_route_string(client->proto)); } client->sock = -1; } From 68671c74390fdb746568413e6d2150cae4c2d4b9 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Wed, 20 May 2020 15:43:23 -0400 Subject: [PATCH 26/57] zebra: warn if zapi NHG add has no nexthops Log a warning and return if we receive a NHG add via zapi that has no nexthops. Signed-off-by: Stephen Worley --- zebra/zapi_msg.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 4ee4274657..4cd733ea35 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -1783,6 +1783,12 @@ static void zread_nhg_reader(ZAPI_HANDLER_ARGS) // if (zserv_nexthop_num_warn(__func__, &p, nhops)) // return; + if (nhops <= 0) { + flog_warn(EC_ZEBRA_NEXTHOP_CREATION_FAILED, + "%s: No nexthops sent", __func__); + return; + } + for (i = 0; i < nhops; i++) { struct zapi_nexthop *znh = &zapi_nexthops[i]; From 1f655680469dc883298348b2bdd4c2ae8bff0fe7 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Wed, 20 May 2020 15:47:12 -0400 Subject: [PATCH 27/57] zebra: fix refcnt/rib issues in NHG replace/delete Fix some reference counting issues seen when replacing a NHG and deleting one. For replacement, we should end with the same refcnt on the new one. For delete, its the caller's job to decrement its ref after its done with it. Further, update routes in the rib with the new pointer after replace. Signed-off-by: Stephen Worley --- zebra/rib.h | 4 +++ zebra/zapi_msg.c | 2 -- zebra/zebra_nhg.c | 68 ++++++++++++++++++++++++++++++++++++----------- zebra/zebra_nhg.h | 2 +- zebra/zebra_rib.c | 25 ++++++++++++++++- 5 files changed, 81 insertions(+), 20 deletions(-) diff --git a/zebra/rib.h b/zebra/rib.h index be680a112f..9ddc35b091 100644 --- a/zebra/rib.h +++ b/zebra/rib.h @@ -333,6 +333,10 @@ extern void route_entry_copy_nexthops(struct route_entry *re, int route_entry_update_nhe(struct route_entry *re, struct nhg_hash_entry *new_nhghe); +/* NHG replace has happend, we have to update route_entry pointers to new one */ +void rib_handle_nhg_replace(struct nhg_hash_entry *old, + struct nhg_hash_entry *new); + #define route_entry_dump(prefix, src, re) _route_entry_dump(__func__, prefix, src, re) extern void _route_entry_dump(const char *func, union prefixconstptr pp, union prefixconstptr src_pp, diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 4cd733ea35..c8220af81e 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -1825,8 +1825,6 @@ static void zread_nhg_reader(ZAPI_HANDLER_ARGS) * Resolution is going to need some more work. */ if (nhe) { - zebra_nhg_increment_ref(nhe); - zebra_nhg_install_kernel(nhe); nhg_notify(proto, client->instance, id, ZAPI_NHG_INSTALLED); } else nhg_notify(proto, client->instance, id, ZAPI_NHG_FAIL_INSTALL); diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 407af2902d..1c9b970008 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -2741,6 +2741,26 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, struct nhg_hash_entry lookup; struct nhg_hash_entry *new, *old; struct nhg_connected *rb_node_dep = NULL; + struct nexthop *newhop; + bool replace = false; + + if (!nhg->nexthop) { + if (IS_ZEBRA_DEBUG_NHG) + zlog_debug("%s: id %u, no nexthops passed to add", + __func__, id); + return NULL; + } + + + /* Set nexthop list as active, since they wont go through rib + * processing. + * + * Assuming valid/onlink for now. + * + * Once resolution is figured out, we won't need this! + */ + for (ALL_NEXTHOPS_PTR(nhg, newhop)) + SET_FLAG(newhop->flags, NEXTHOP_FLAG_ACTIVE); zebra_nhe_init(&lookup, afi, nhg->nexthop); lookup.nhg.nexthop = nhg->nexthop; @@ -2754,36 +2774,51 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, * This is a replace, just release NHE from ID for now, The * depends/dependents may still be used in the replacement. */ + replace = true; hash_release(zrouter.nhgs_id, old); } new = zebra_nhg_rib_find_nhe(&lookup, afi); + zebra_nhg_increment_ref(new); + + zebra_nhg_set_valid_if_active(new); + + zebra_nhg_install_kernel(new); + if (old) { - /* Now release depends/dependents in old one */ + rib_handle_nhg_replace(old, new); + + /* if this != 1 at this point, we have a bug */ + assert(old->refcnt == 1); + + /* We have to decrement its singletons + * because some might not exist in NEW. + */ + if (!zebra_nhg_depends_is_empty(old)) { + frr_each (nhg_connected_tree, &old->nhg_depends, + rb_node_dep) + zebra_nhg_decrement_ref(rb_node_dep->nhe); + } + + /* Free all the things */ zebra_nhg_release_all_deps(old); - } - if (!new) - return NULL; - - /* TODO: Assuming valid/onlink for now */ - SET_FLAG(new->flags, NEXTHOP_GROUP_VALID); - - if (!zebra_nhg_depends_is_empty(new)) { - frr_each (nhg_connected_tree, &new->nhg_depends, rb_node_dep) - SET_FLAG(rb_node_dep->nhe->flags, NEXTHOP_GROUP_VALID); + /* Dont call the dec API, we dont want to uninstall the ID */ + old->refcnt = 0; + zebra_nhg_free(old); + old = NULL; } if (IS_ZEBRA_DEBUG_NHG_DETAIL) zlog_debug("%s: %s nhe %p (%u), vrf %d, type %s", __func__, - (old ? "replaced" : "added"), new, new->id, + (replace ? "replaced" : "added"), new, new->id, new->vrf_id, zebra_route_string(new->type)); return new; } -/* Delete NHE from upper level proto */ +/* Delete NHE from upper level proto, caller must decrement ref */ struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id) { struct nhg_hash_entry *nhe; @@ -2797,11 +2832,12 @@ struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id) return NULL; } - if (nhe->refcnt) { + if (nhe->refcnt > 1) { /* TODO: should be warn? */ if (IS_ZEBRA_DEBUG_NHG) - zlog_debug("%s: id %u, still being used refcnt %u", - __func__, nhe->id, nhe->refcnt); + zlog_debug( + "%s: id %u, still being used by routes refcnt %u", + __func__, nhe->id, nhe->refcnt); return NULL; } diff --git a/zebra/zebra_nhg.h b/zebra/zebra_nhg.h index 8c33a91917..6c9f20d0a4 100644 --- a/zebra/zebra_nhg.h +++ b/zebra/zebra_nhg.h @@ -281,7 +281,7 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, * * Returns deleted NHE on success, otherwise NULL. * - * Caller must free the NHE. + * Caller must decrement ref with zebra_nhg_decrement_ref() when done. */ struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id); diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c index 5a7967d817..aac0e628fe 100644 --- a/zebra/zebra_rib.c +++ b/zebra/zebra_rib.c @@ -235,7 +235,7 @@ int route_entry_update_nhe(struct route_entry *re, goto done; } - if ((re->nhe_id != 0) && (re->nhe_id != new_nhghe->id)) { + if ((re->nhe_id != 0) && re->nhe && (re->nhe != new_nhghe)) { old = re->nhe; route_entry_attach_ref(re, new_nhghe); @@ -250,6 +250,29 @@ done: return ret; } +void rib_handle_nhg_replace(struct nhg_hash_entry *old, + struct nhg_hash_entry *new) +{ + struct zebra_router_table *zrt; + struct route_node *rn; + struct route_entry *re, *next; + + if (IS_ZEBRA_DEBUG_RIB_DETAILED || IS_ZEBRA_DEBUG_NHG_DETAIL) + zlog_debug("%s: replacing routes nhe (%u) OLD %p NEW %p", + __func__, new->id, new, old); + + /* We have to do them ALL */ + RB_FOREACH (zrt, zebra_router_table_head, &zrouter.tables) { + for (rn = route_top(zrt->table); rn; + rn = srcdest_route_next(rn)) { + RNODE_FOREACH_RE_SAFE (rn, re, next) { + if (re->nhe && re->nhe == old) + route_entry_update_nhe(re, new); + } + } + } +} + struct route_entry *rib_match(afi_t afi, safi_t safi, vrf_id_t vrf_id, union g_addr *addr, struct route_node **rn_out) { From f651b708e0921d9610803a6e42414b058dfdfb56 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Wed, 20 May 2020 18:04:53 -0400 Subject: [PATCH 28/57] zebra: increment the nhg proto score iterator Increment the nhg proto score iterator we used to count leftover NHGs after client disconnect and log. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 1c9b970008..517e202e74 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -2864,6 +2864,8 @@ static void zebra_nhg_score_proto_entry(struct hash_bucket *bucket, void *arg) /* Needs to match type and outside zebra ID space */ if (nhe->type == iter->type && nhe->id >= ZEBRA_NHG_PROTO_LOWER) { + iter->found++; + if (IS_ZEBRA_DEBUG_NHG_DETAIL) zlog_debug( "%s: found nhe %p (%u), vrf %d, type %s after client disconnect", From 3bccc0f5ebd71ac269fc7d9723146f5c32a50705 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Sun, 24 May 2020 16:03:01 -0400 Subject: [PATCH 29/57] zebra: fix releasing proto-owned singletons Fix the releasing of proto-owned singletons from the attribute hashed table. Proto-owned singleton nexthops are hashed so they can still be shared therefore they are present in this table and need to be released when the time comes. This check was only matching on zebra proto before. Changed to match IDs in zebra allocated range. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 517e202e74..483b911b31 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -1074,7 +1074,7 @@ static void zebra_nhg_release(struct nhg_hash_entry *nhe) * If its not zebra owned, we didn't store it here and have to be * sure we don't clear one thats actually being used. */ - if (ZEBRA_OWNED(nhe)) + if (nhe->id < ZEBRA_NHG_PROTO_LOWER) hash_release(zrouter.nhgs, nhe); hash_release(zrouter.nhgs_id, nhe); From 9c6c48bc101f8f4b47fa9f373e436c85d04608cf Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Sun, 24 May 2020 16:08:36 -0400 Subject: [PATCH 30/57] zebra: return the proto nhe on del even with refs Return the proto nhe on del even if their are still possible route references. We may get a del before the routes are removed. So we still need to return this to the caller so they can decrement the ref. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 483b911b31..b4d7df0b53 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -2838,7 +2838,7 @@ struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id) zlog_debug( "%s: id %u, still being used by routes refcnt %u", __func__, nhe->id, nhe->refcnt); - return NULL; + return nhe; } if (IS_ZEBRA_DEBUG_NHG_DETAIL) From 0de1db8f3bd8d25be9dbebc3a090f85d60b41b14 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Wed, 27 May 2020 17:39:41 -0400 Subject: [PATCH 31/57] lib,sharpd,pbrd: `set installable` nhg command Add a command `set installable` that allows configured nexthop groups to be treated as separate/installable objects in the RIB. A callback needs to be implemented per daemon to handle installing the NHG into the rib via zapi when this command is set. This patch includes the implementation for sharpd. Signed-off-by: Stephen Worley --- doc/user/pbr.rst | 11 ++++++++++ lib/nexthop_group.c | 53 ++++++++++++++++++++++++++++++++++++++++----- lib/nexthop_group.h | 6 ++++- pbrd/pbr_main.c | 7 +++--- sharpd/sharp_nht.c | 40 ++++++++++++++++++++++++++++++---- 5 files changed, 102 insertions(+), 15 deletions(-) diff --git a/doc/user/pbr.rst b/doc/user/pbr.rst index c869c6bc45..a57a176f3c 100644 --- a/doc/user/pbr.rst +++ b/doc/user/pbr.rst @@ -45,6 +45,17 @@ listing of ECMP nexthops used to forward packets for when a pbr-map is matched. are used to are allowed here. The syntax was intentionally kept the same as creating nexthops as you would for static routes. +.. clicmd:: set installable + + Sets the nexthop group to be installable i.e. treated as a separate object in + the protocol client and zebra's RIB. The proto will send down the object + separately from the route to install into into the RIB and dataplane. + +.. note:: + ``set installable`` is only supported for groups with onlink, interface, and + gateway/interface nexthop types at the moment. Recursive nexthops + (gateway only) are considered undefined behavior. + .. clicmd:: [no] pbr table range (10000-4294966272) (10000-4294966272) Set or unset the range used to assign numeric table ID's to new diff --git a/lib/nexthop_group.c b/lib/nexthop_group.c index 83905abe43..136970cff4 100644 --- a/lib/nexthop_group.c +++ b/lib/nexthop_group.c @@ -54,6 +54,7 @@ struct nexthop_group_hooks { void (*del_nexthop)(const struct nexthop_group_cmd *nhg, const struct nexthop *nhop); void (*delete)(const char *name); + void (*installable)(const struct nexthop_group_cmd *nhg); }; static struct nexthop_group_hooks nhg_hooks; @@ -675,6 +676,37 @@ DEFPY(no_nexthop_group_backup, no_nexthop_group_backup_cmd, return CMD_SUCCESS; } +DEFPY(set_installable, set_installable_cmd, + "set installable", + "Set for nexthop-group\n" + "Install nexthop-group into RIB as separate object\n") +{ + VTY_DECLVAR_CONTEXT(nexthop_group_cmd, nhgc); + + nhgc->installable = true; + + if (nhg_hooks.installable) + nhg_hooks.installable(nhgc); + + return CMD_SUCCESS; +} + +DEFPY(no_set_installable, no_set_installable_cmd, + "no set installable", + NO_STR + "Set for nexthop-group\n" + "Install nexthop-group into RIB as separate object\n") +{ + VTY_DECLVAR_CONTEXT(nexthop_group_cmd, nhgc); + + nhgc->installable = false; + + if (nhg_hooks.installable) + nhg_hooks.installable(nhgc); + + return CMD_SUCCESS; +} + static void nexthop_group_save_nhop(struct nexthop_group_cmd *nhgc, const char *nhvrf_name, const union sockunion *addr, @@ -1147,6 +1179,9 @@ static int nexthop_group_write(struct vty *vty) vty_out(vty, "nexthop-group %s\n", nhgc->name); + if (nhgc->installable) + vty_out(vty, " set installable\n"); + if (nhgc->backup_list_name[0]) vty_out(vty, " backup-group %s\n", nhgc->backup_list_name); @@ -1316,12 +1351,14 @@ static const struct cmd_variable_handler nhg_name_handlers[] = { {.tokenname = "NHGNAME", .completions = nhg_name_autocomplete}, {.completions = NULL}}; -void nexthop_group_init(void (*new)(const char *name), - void (*add_nexthop)(const struct nexthop_group_cmd *nhg, - const struct nexthop *nhop), - void (*del_nexthop)(const struct nexthop_group_cmd *nhg, - const struct nexthop *nhop), - void (*delete)(const char *name)) +void nexthop_group_init( + void (*new)(const char *name), + void (*add_nexthop)(const struct nexthop_group_cmd *nhg, + const struct nexthop *nhop), + void (*del_nexthop)(const struct nexthop_group_cmd *nhg, + const struct nexthop *nhop), + void (*delete)(const char *name), + void (*installable)(const struct nexthop_group_cmd *nhg)) { RB_INIT(nhgc_entry_head, &nhgc_entries); @@ -1334,6 +1371,8 @@ void nexthop_group_init(void (*new)(const char *name), install_default(NH_GROUP_NODE); install_element(NH_GROUP_NODE, &nexthop_group_backup_cmd); install_element(NH_GROUP_NODE, &no_nexthop_group_backup_cmd); + install_element(NH_GROUP_NODE, &set_installable_cmd); + install_element(NH_GROUP_NODE, &no_set_installable_cmd); install_element(NH_GROUP_NODE, &ecmp_nexthops_cmd); memset(&nhg_hooks, 0, sizeof(nhg_hooks)); @@ -1346,4 +1385,6 @@ void nexthop_group_init(void (*new)(const char *name), nhg_hooks.del_nexthop = del_nexthop; if (delete) nhg_hooks.delete = delete; + if (installable) + nhg_hooks.installable = installable; } diff --git a/lib/nexthop_group.h b/lib/nexthop_group.h index 5f7bde0def..e06035ce68 100644 --- a/lib/nexthop_group.h +++ b/lib/nexthop_group.h @@ -97,6 +97,9 @@ struct nexthop_group_cmd { struct list *nhg_list; + /* Install nhg as separate object in RIB */ + bool installable; + QOBJ_FIELDS }; RB_HEAD(nhgc_entry_head, nexthp_group_cmd); @@ -116,7 +119,8 @@ void nexthop_group_init( const struct nexthop *nhop), void (*del_nexthop)(const struct nexthop_group_cmd *nhgc, const struct nexthop *nhop), - void (*destroy)(const char *name)); + void (*destroy)(const char *name), + void (*installable)(const struct nexthop_group_cmd *nhg)); void nexthop_group_enable_vrf(struct vrf *vrf); void nexthop_group_disable_vrf(struct vrf *vrf); diff --git a/pbrd/pbr_main.c b/pbrd/pbr_main.c index 9a9edd79c6..bae8be4f95 100644 --- a/pbrd/pbr_main.c +++ b/pbrd/pbr_main.c @@ -157,10 +157,9 @@ int main(int argc, char **argv, char **envp) pbr_debug_init(); - nexthop_group_init(pbr_nhgroup_add_cb, - pbr_nhgroup_add_nexthop_cb, - pbr_nhgroup_del_nexthop_cb, - pbr_nhgroup_delete_cb); + nexthop_group_init(pbr_nhgroup_add_cb, pbr_nhgroup_add_nexthop_cb, + pbr_nhgroup_del_nexthop_cb, pbr_nhgroup_delete_cb, + NULL); /* * So we safely ignore these commands since diff --git a/sharpd/sharp_nht.c b/sharpd/sharp_nht.c index 731d58e560..e76b1016a8 100644 --- a/sharpd/sharp_nht.c +++ b/sharpd/sharp_nht.c @@ -78,6 +78,8 @@ struct sharp_nhg { uint32_t id; char name[256]; + + bool installable; }; static uint32_t nhg_id; @@ -120,7 +122,9 @@ static void sharp_nhgroup_add_nexthop_cb(const struct nexthop_group_cmd *nhgc, strncpy(lookup.name, nhgc->name, sizeof(lookup.name)); snhg = sharp_nhg_rb_find(&nhg_head, &lookup); - nhg_add(snhg->id, &nhgc->nhg); + if (snhg->installable) + nhg_add(snhg->id, &nhgc->nhg); + return; } @@ -133,7 +137,9 @@ static void sharp_nhgroup_del_nexthop_cb(const struct nexthop_group_cmd *nhgc, strncpy(lookup.name, nhgc->name, sizeof(lookup.name)); snhg = sharp_nhg_rb_find(&nhg_head, &lookup); - nhg_add(snhg->id, &nhgc->nhg); + if (snhg->installable) + nhg_add(snhg->id, &nhgc->nhg); + return; } @@ -147,12 +153,34 @@ static void sharp_nhgroup_delete_cb(const char *name) if (!snhg) return; - nhg_del(snhg->id); + if (snhg->installable) + nhg_del(snhg->id); + sharp_nhg_rb_del(&nhg_head, snhg); XFREE(MTYPE_NHG, snhg); return; } +static void sharp_nhgroup_installable_cb(const struct nexthop_group_cmd *nhgc) +{ + struct sharp_nhg lookup; + struct sharp_nhg *snhg; + + strncpy(lookup.name, nhgc->name, sizeof(lookup.name)); + snhg = sharp_nhg_rb_find(&nhg_head, &lookup); + if (!snhg) + return; + + snhg->installable = nhgc->installable; + + if (snhg->installable) + nhg_add(snhg->id, &nhgc->nhg); + else + nhg_del(snhg->id); + + return; +} + uint32_t sharp_nhgroup_get_id(const char *name) { struct sharp_nhg lookup; @@ -163,6 +191,9 @@ uint32_t sharp_nhgroup_get_id(const char *name) if (!snhg) return 0; + if (!snhg->installable) + return 0; + return snhg->id; } @@ -173,5 +204,6 @@ void sharp_nhgroup_init(void) nexthop_group_init(sharp_nhgroup_add_cb, sharp_nhgroup_add_nexthop_cb, sharp_nhgroup_del_nexthop_cb, - sharp_nhgroup_delete_cb); + sharp_nhgroup_delete_cb, + sharp_nhgroup_installable_cb); } From b36bedd2c3ff98e75383630c2a637339c7f76eb5 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Thu, 28 May 2020 12:08:29 -0400 Subject: [PATCH 32/57] lib: add logging for ZEBRA_NHG_ADD[DEL] Add logging info for the new zapi ZEBRA_NHG_ADD[DEL] message types. With this patch, they are logged properly when debugs are turned on. Signed-off-by: Stephen Worley --- lib/log.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/log.c b/lib/log.c index 4054185019..91d325efb2 100644 --- a/lib/log.c +++ b/lib/log.c @@ -452,7 +452,9 @@ static const struct zebra_desc_table command_types[] = { DESC_ENTRY(ZEBRA_OPAQUE_MESSAGE), DESC_ENTRY(ZEBRA_OPAQUE_REGISTER), DESC_ENTRY(ZEBRA_OPAQUE_UNREGISTER), - DESC_ENTRY(ZEBRA_NEIGH_DISCOVER)}; + DESC_ENTRY(ZEBRA_NEIGH_DISCOVER), + DESC_ENTRY(ZEBRA_NHG_ADD), + DESC_ENTRY(ZEBRA_NHG_DEL)}; #undef DESC_ENTRY static const struct zebra_desc_table unknown = {0, "unknown", '?'}; From 72938edfbc95ee86bccb84633d144951ae2b2b97 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Thu, 28 May 2020 13:22:18 -0400 Subject: [PATCH 33/57] zebra: add logging for NHG ignoring in netlink Add some logging for when we choose to ignore a NHG install for one reason or another. Also, cleanup some of the code using the same accessor functions for the context object. Signed-off-by: Stephen Worley --- zebra/rt_netlink.c | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/zebra/rt_netlink.c b/zebra/rt_netlink.c index 34841aa4a5..46de751f95 100644 --- a/zebra/rt_netlink.c +++ b/zebra/rt_netlink.c @@ -2100,16 +2100,35 @@ ssize_t netlink_nexthop_msg_encode(uint16_t cmd, mpls_lse_t out_lse[MPLS_MAX_LABELS]; char label_buf[256]; int num_labels = 0; + uint32_t id = dplane_ctx_get_nhe_id(ctx); + int type = dplane_ctx_get_nhe_type(ctx); + + if (!id) { + flog_err( + EC_ZEBRA_NHG_FIB_UPDATE, + "Failed trying to update a nexthop group in the kernel that does not have an ID"); + return -1; + } /* * Nothing to do if the kernel doesn't support nexthop objects or * we dont want to install this type of NHG */ - if (!kernel_nexthops_supported() - || (proto_nexthops_only() - && !is_proto_nhg(dplane_ctx_get_nhe_id(ctx), - dplane_ctx_get_nhe_type(ctx)))) + if (!kernel_nexthops_supported()) { + if (IS_ZEBRA_DEBUG_KERNEL || IS_ZEBRA_DEBUG_NHG) + zlog_debug( + "%s: nhg_id %u (%s): kernel nexthops not supported, ignoring", + __func__, id, zebra_route_string(type)); return 0; + } + + if (proto_nexthops_only() && !is_proto_nhg(id, type)) { + if (IS_ZEBRA_DEBUG_KERNEL || IS_ZEBRA_DEBUG_NHG) + zlog_debug( + "%s: nhg_id %u (%s): proto-based nexthops only, ignoring", + __func__, id, zebra_route_string(type)); + return 0; + } label_buf[0] = '\0'; @@ -2130,15 +2149,6 @@ ssize_t netlink_nexthop_msg_encode(uint16_t cmd, req->nhm.nh_family = AF_UNSPEC; /* TODO: Scope? */ - uint32_t id = dplane_ctx_get_nhe_id(ctx); - - if (!id) { - flog_err( - EC_ZEBRA_NHG_FIB_UPDATE, - "Failed trying to update a nexthop group in the kernel that does not have an ID"); - return -1; - } - if (!nl_attr_put32(&req->n, buflen, NHA_ID, id)) return 0; @@ -2259,8 +2269,7 @@ nexthop_done: nh->vrf_id, label_buf); } - req->nhm.nh_protocol = - zebra2proto(dplane_ctx_get_nhe_type(ctx)); + req->nhm.nh_protocol = zebra2proto(type); } else if (cmd != RTM_DELNEXTHOP) { flog_err( From cc6a0d7d80e176ce9cced99cb1d5de9792c6f3f3 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Fri, 24 Jul 2020 17:50:20 -0400 Subject: [PATCH 34/57] Revert "lib,sharpd,pbrd: `set installable` nhg command" This reverts commit 1844f45e30913b27cfd875036f865a0edadcf244. --- doc/user/pbr.rst | 11 ---------- lib/nexthop_group.c | 53 +++++---------------------------------------- lib/nexthop_group.h | 6 +---- pbrd/pbr_main.c | 7 +++--- sharpd/sharp_nht.c | 40 ++++------------------------------ 5 files changed, 15 insertions(+), 102 deletions(-) diff --git a/doc/user/pbr.rst b/doc/user/pbr.rst index a57a176f3c..c869c6bc45 100644 --- a/doc/user/pbr.rst +++ b/doc/user/pbr.rst @@ -45,17 +45,6 @@ listing of ECMP nexthops used to forward packets for when a pbr-map is matched. are used to are allowed here. The syntax was intentionally kept the same as creating nexthops as you would for static routes. -.. clicmd:: set installable - - Sets the nexthop group to be installable i.e. treated as a separate object in - the protocol client and zebra's RIB. The proto will send down the object - separately from the route to install into into the RIB and dataplane. - -.. note:: - ``set installable`` is only supported for groups with onlink, interface, and - gateway/interface nexthop types at the moment. Recursive nexthops - (gateway only) are considered undefined behavior. - .. clicmd:: [no] pbr table range (10000-4294966272) (10000-4294966272) Set or unset the range used to assign numeric table ID's to new diff --git a/lib/nexthop_group.c b/lib/nexthop_group.c index 136970cff4..83905abe43 100644 --- a/lib/nexthop_group.c +++ b/lib/nexthop_group.c @@ -54,7 +54,6 @@ struct nexthop_group_hooks { void (*del_nexthop)(const struct nexthop_group_cmd *nhg, const struct nexthop *nhop); void (*delete)(const char *name); - void (*installable)(const struct nexthop_group_cmd *nhg); }; static struct nexthop_group_hooks nhg_hooks; @@ -676,37 +675,6 @@ DEFPY(no_nexthop_group_backup, no_nexthop_group_backup_cmd, return CMD_SUCCESS; } -DEFPY(set_installable, set_installable_cmd, - "set installable", - "Set for nexthop-group\n" - "Install nexthop-group into RIB as separate object\n") -{ - VTY_DECLVAR_CONTEXT(nexthop_group_cmd, nhgc); - - nhgc->installable = true; - - if (nhg_hooks.installable) - nhg_hooks.installable(nhgc); - - return CMD_SUCCESS; -} - -DEFPY(no_set_installable, no_set_installable_cmd, - "no set installable", - NO_STR - "Set for nexthop-group\n" - "Install nexthop-group into RIB as separate object\n") -{ - VTY_DECLVAR_CONTEXT(nexthop_group_cmd, nhgc); - - nhgc->installable = false; - - if (nhg_hooks.installable) - nhg_hooks.installable(nhgc); - - return CMD_SUCCESS; -} - static void nexthop_group_save_nhop(struct nexthop_group_cmd *nhgc, const char *nhvrf_name, const union sockunion *addr, @@ -1179,9 +1147,6 @@ static int nexthop_group_write(struct vty *vty) vty_out(vty, "nexthop-group %s\n", nhgc->name); - if (nhgc->installable) - vty_out(vty, " set installable\n"); - if (nhgc->backup_list_name[0]) vty_out(vty, " backup-group %s\n", nhgc->backup_list_name); @@ -1351,14 +1316,12 @@ static const struct cmd_variable_handler nhg_name_handlers[] = { {.tokenname = "NHGNAME", .completions = nhg_name_autocomplete}, {.completions = NULL}}; -void nexthop_group_init( - void (*new)(const char *name), - void (*add_nexthop)(const struct nexthop_group_cmd *nhg, - const struct nexthop *nhop), - void (*del_nexthop)(const struct nexthop_group_cmd *nhg, - const struct nexthop *nhop), - void (*delete)(const char *name), - void (*installable)(const struct nexthop_group_cmd *nhg)) +void nexthop_group_init(void (*new)(const char *name), + void (*add_nexthop)(const struct nexthop_group_cmd *nhg, + const struct nexthop *nhop), + void (*del_nexthop)(const struct nexthop_group_cmd *nhg, + const struct nexthop *nhop), + void (*delete)(const char *name)) { RB_INIT(nhgc_entry_head, &nhgc_entries); @@ -1371,8 +1334,6 @@ void nexthop_group_init( install_default(NH_GROUP_NODE); install_element(NH_GROUP_NODE, &nexthop_group_backup_cmd); install_element(NH_GROUP_NODE, &no_nexthop_group_backup_cmd); - install_element(NH_GROUP_NODE, &set_installable_cmd); - install_element(NH_GROUP_NODE, &no_set_installable_cmd); install_element(NH_GROUP_NODE, &ecmp_nexthops_cmd); memset(&nhg_hooks, 0, sizeof(nhg_hooks)); @@ -1385,6 +1346,4 @@ void nexthop_group_init( nhg_hooks.del_nexthop = del_nexthop; if (delete) nhg_hooks.delete = delete; - if (installable) - nhg_hooks.installable = installable; } diff --git a/lib/nexthop_group.h b/lib/nexthop_group.h index e06035ce68..5f7bde0def 100644 --- a/lib/nexthop_group.h +++ b/lib/nexthop_group.h @@ -97,9 +97,6 @@ struct nexthop_group_cmd { struct list *nhg_list; - /* Install nhg as separate object in RIB */ - bool installable; - QOBJ_FIELDS }; RB_HEAD(nhgc_entry_head, nexthp_group_cmd); @@ -119,8 +116,7 @@ void nexthop_group_init( const struct nexthop *nhop), void (*del_nexthop)(const struct nexthop_group_cmd *nhgc, const struct nexthop *nhop), - void (*destroy)(const char *name), - void (*installable)(const struct nexthop_group_cmd *nhg)); + void (*destroy)(const char *name)); void nexthop_group_enable_vrf(struct vrf *vrf); void nexthop_group_disable_vrf(struct vrf *vrf); diff --git a/pbrd/pbr_main.c b/pbrd/pbr_main.c index bae8be4f95..9a9edd79c6 100644 --- a/pbrd/pbr_main.c +++ b/pbrd/pbr_main.c @@ -157,9 +157,10 @@ int main(int argc, char **argv, char **envp) pbr_debug_init(); - nexthop_group_init(pbr_nhgroup_add_cb, pbr_nhgroup_add_nexthop_cb, - pbr_nhgroup_del_nexthop_cb, pbr_nhgroup_delete_cb, - NULL); + nexthop_group_init(pbr_nhgroup_add_cb, + pbr_nhgroup_add_nexthop_cb, + pbr_nhgroup_del_nexthop_cb, + pbr_nhgroup_delete_cb); /* * So we safely ignore these commands since diff --git a/sharpd/sharp_nht.c b/sharpd/sharp_nht.c index e76b1016a8..731d58e560 100644 --- a/sharpd/sharp_nht.c +++ b/sharpd/sharp_nht.c @@ -78,8 +78,6 @@ struct sharp_nhg { uint32_t id; char name[256]; - - bool installable; }; static uint32_t nhg_id; @@ -122,9 +120,7 @@ static void sharp_nhgroup_add_nexthop_cb(const struct nexthop_group_cmd *nhgc, strncpy(lookup.name, nhgc->name, sizeof(lookup.name)); snhg = sharp_nhg_rb_find(&nhg_head, &lookup); - if (snhg->installable) - nhg_add(snhg->id, &nhgc->nhg); - + nhg_add(snhg->id, &nhgc->nhg); return; } @@ -137,9 +133,7 @@ static void sharp_nhgroup_del_nexthop_cb(const struct nexthop_group_cmd *nhgc, strncpy(lookup.name, nhgc->name, sizeof(lookup.name)); snhg = sharp_nhg_rb_find(&nhg_head, &lookup); - if (snhg->installable) - nhg_add(snhg->id, &nhgc->nhg); - + nhg_add(snhg->id, &nhgc->nhg); return; } @@ -153,34 +147,12 @@ static void sharp_nhgroup_delete_cb(const char *name) if (!snhg) return; - if (snhg->installable) - nhg_del(snhg->id); - + nhg_del(snhg->id); sharp_nhg_rb_del(&nhg_head, snhg); XFREE(MTYPE_NHG, snhg); return; } -static void sharp_nhgroup_installable_cb(const struct nexthop_group_cmd *nhgc) -{ - struct sharp_nhg lookup; - struct sharp_nhg *snhg; - - strncpy(lookup.name, nhgc->name, sizeof(lookup.name)); - snhg = sharp_nhg_rb_find(&nhg_head, &lookup); - if (!snhg) - return; - - snhg->installable = nhgc->installable; - - if (snhg->installable) - nhg_add(snhg->id, &nhgc->nhg); - else - nhg_del(snhg->id); - - return; -} - uint32_t sharp_nhgroup_get_id(const char *name) { struct sharp_nhg lookup; @@ -191,9 +163,6 @@ uint32_t sharp_nhgroup_get_id(const char *name) if (!snhg) return 0; - if (!snhg->installable) - return 0; - return snhg->id; } @@ -204,6 +173,5 @@ void sharp_nhgroup_init(void) nexthop_group_init(sharp_nhgroup_add_cb, sharp_nhgroup_add_nexthop_cb, sharp_nhgroup_del_nexthop_cb, - sharp_nhgroup_delete_cb, - sharp_nhgroup_installable_cb); + sharp_nhgroup_delete_cb); } From e3b9c0f2f696956aac4f7c58ad73f46b46264dca Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 11 Jun 2020 07:34:18 -0400 Subject: [PATCH 35/57] zebra: Only install a minimal amount of times The code was installing the nexthop group again using the NLM_F_REPLACE function causing extremely large route installation times. This reduces the time from installing 1 million routes from sharpd with a nhg from > 200 seconds ( where I gave up ) to ~15 seconds on my machine for 32 x ecmp. As a side note 1 million routes using master sharpd takes ~50 seconds to do the same thing. Signed-off-by: Donald Sharp --- zebra/zebra_nhg.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index b4d7df0b53..41754b9f88 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -2553,8 +2553,7 @@ void zebra_nhg_install_kernel(struct nhg_hash_entry *nhe) } if (CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_VALID) - && (!CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_INSTALLED) - || nhe->id >= ZEBRA_NHG_PROTO_LOWER) + && !CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_INSTALLED) && !CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_QUEUED)) { /* Change its type to us since we are installing it */ if (!ZEBRA_NHG_CREATED(nhe)) From 27805e74f02c9e0b4f0086e8727135788cf9fe2b Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 11 Jun 2020 07:34:18 -0400 Subject: [PATCH 36/57] zebra: Properly set NEXTHOP_FLAG_FIB when skipping install When the dataplane detects that we have no need to reinstall the same route, setup the NEXTHOP_FLAG_FIB appropriately. Signed-off-by: Donald Sharp --- zebra/zebra_dplane.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/zebra/zebra_dplane.c b/zebra/zebra_dplane.c index f2725adc54..77abe8bb6d 100644 --- a/zebra/zebra_dplane.c +++ b/zebra/zebra_dplane.c @@ -2368,11 +2368,25 @@ dplane_route_update_internal(struct route_node *rn, if ((dplane_ctx_get_type(ctx) == dplane_ctx_get_old_type(ctx)) && (dplane_ctx_get_nhe_id(ctx) == dplane_ctx_get_old_nhe_id(ctx))) { + struct nexthop *nexthop; + if (IS_ZEBRA_DEBUG_DPLANE) zlog_debug( "%s: Ignoring Route exactly the same", __func__); + for (ALL_NEXTHOPS_PTR(dplane_ctx_get_ng(ctx), + nexthop)) { + if (CHECK_FLAG(nexthop->flags, + NEXTHOP_FLAG_RECURSIVE)) + continue; + + if (CHECK_FLAG(nexthop->flags, + NEXTHOP_FLAG_ACTIVE)) + SET_FLAG(nexthop->flags, + NEXTHOP_FLAG_FIB); + } + return ZEBRA_DPLANE_REQUEST_SUCCESS; } From 2c7819b9d42ab4c1af56946819b6721f27011e9a Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Thu, 11 Jun 2020 13:45:03 -0400 Subject: [PATCH 37/57] lib,zebra: fixup NHG notify zapi messaging Make the message parameters align better with other zapi notifications and change the ID to correctly be a uint32. Signed-off-by: Stephen Worley --- lib/zclient.c | 4 ++-- zebra/zapi_msg.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/zclient.c b/lib/zclient.c index cde2f5e052..2e65e3c9f7 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -1486,10 +1486,10 @@ int zapi_pbr_rule_encode(uint8_t cmd, struct stream *s, struct pbr_rule *zrule) bool zapi_nhg_notify_decode(struct stream *s, uint32_t *id, enum zapi_nhg_notify_owner *note) { - uint16_t read_id; + uint32_t read_id; - STREAM_GETL(s, read_id); STREAM_GET(note, s, sizeof(*note)); + STREAM_GETL(s, read_id); *id = read_id; diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index c8220af81e..21f40ce7c2 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -712,7 +712,7 @@ static int zsend_ipv4_nexthop_lookup_mrib(struct zserv *client, return zserv_send_message(client, s); } -static int nhg_notify(uint16_t type, uint16_t instance, uint16_t id, +static int nhg_notify(uint16_t type, uint16_t instance, uint32_t id, enum zapi_nhg_notify_owner note) { struct zserv *client; @@ -732,8 +732,8 @@ static int nhg_notify(uint16_t type, uint16_t instance, uint16_t id, zclient_create_header(s, ZEBRA_NHG_NOTIFY_OWNER, VRF_DEFAULT); - stream_putw(s, id); stream_put(s, ¬e, sizeof(note)); + stream_putl(s, id); stream_putw_at(s, 0, stream_get_endp(s)); From 70347b7ad6e822ec5a6f22c866c408479887f7b7 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Thu, 11 Jun 2020 13:46:48 -0400 Subject: [PATCH 38/57] zebra: reply fail on NHG add if not ifindex/onlink We currently don't support ADD/DEL/REPLACE with proto-based NHGs that are not already fully resolved and ifindex/onlink based. If we are handed one that doesn't have ifindex set i.e. recursive, gracefully fail and with a notification. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 41754b9f88..3d0835d272 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -2758,8 +2758,16 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, * * Once resolution is figured out, we won't need this! */ - for (ALL_NEXTHOPS_PTR(nhg, newhop)) + for (ALL_NEXTHOPS_PTR(nhg, newhop)) { + if (!newhop->ifindex) { + if (IS_ZEBRA_DEBUG_NHG) + zlog_debug( + "%s: id %u, nexthop without ifindex passed to add", + __func__, id); + return NULL; + } SET_FLAG(newhop->flags, NEXTHOP_FLAG_ACTIVE); + } zebra_nhe_init(&lookup, afi, nhg->nexthop); lookup.nhg.nexthop = nhg->nexthop; From 2053061baeff77c8ce39baf59d6abefa1ee821eb Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Thu, 11 Jun 2020 13:49:25 -0400 Subject: [PATCH 39/57] sharpd: implement NHG notification handling Implement handling of NHG notifications in sharpd so that the routes don't attempt to use an NHG ID that did not successfully get created. If it does not get installed, we fall back to traditional zapi messaging. Signed-off-by: Stephen Worley --- sharpd/sharp_nht.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ sharpd/sharp_nht.h | 3 +++ sharpd/sharp_zebra.c | 31 ++++++++++++++++++++++++++++++- 3 files changed, 77 insertions(+), 1 deletion(-) diff --git a/sharpd/sharp_nht.c b/sharpd/sharp_nht.c index 731d58e560..4c32dc279d 100644 --- a/sharpd/sharp_nht.c +++ b/sharpd/sharp_nht.c @@ -78,6 +78,8 @@ struct sharp_nhg { uint32_t id; char name[256]; + + bool installed; }; static uint32_t nhg_id; @@ -99,6 +101,22 @@ static int sharp_nhg_compare_func(const struct sharp_nhg *a, DECLARE_RBTREE_UNIQ(sharp_nhg_rb, struct sharp_nhg, mylistitem, sharp_nhg_compare_func); +static struct sharp_nhg *sharp_nhgroup_find_id(uint32_t id) +{ + struct sharp_nhg *lookup; + + /* Yea its just a for loop, I don't want add complexity + * to sharpd with another RB tree for just IDs + */ + + frr_each(sharp_nhg_rb, &nhg_head, lookup) { + if (lookup->id == id) + return lookup; + } + + return NULL; +} + static void sharp_nhgroup_add_cb(const char *name) { struct sharp_nhg *snhg; @@ -166,6 +184,32 @@ uint32_t sharp_nhgroup_get_id(const char *name) return snhg->id; } +void sharp_nhgroup_id_set_installed(uint32_t id, bool installed) +{ + struct sharp_nhg *snhg; + + snhg = sharp_nhgroup_find_id(id); + if (!snhg) { + zlog_debug("%s: nhg %u not found", __func__, id); + return; + } + + snhg->installed = installed; +} + +bool sharp_nhgroup_id_is_installed(uint32_t id) +{ + struct sharp_nhg *snhg; + + snhg = sharp_nhgroup_find_id(id); + if (!snhg) { + zlog_debug("%s: nhg %u not found", __func__, id); + return false; + } + + return snhg->installed; +} + void sharp_nhgroup_init(void) { sharp_nhg_rb_init(&nhg_head); diff --git a/sharpd/sharp_nht.h b/sharpd/sharp_nht.h index 27c0104583..da33502878 100644 --- a/sharpd/sharp_nht.h +++ b/sharpd/sharp_nht.h @@ -37,5 +37,8 @@ extern struct sharp_nh_tracker *sharp_nh_tracker_get(struct prefix *p); extern void sharp_nh_tracker_dump(struct vty *vty); extern uint32_t sharp_nhgroup_get_id(const char *name); +extern void sharp_nhgroup_id_set_installed(uint32_t id, bool installed); +extern bool sharp_nhgroup_id_is_installed(uint32_t id); + extern void sharp_nhgroup_init(void); #endif diff --git a/sharpd/sharp_zebra.c b/sharpd/sharp_zebra.c index 8e357f96c9..6be1176db5 100644 --- a/sharpd/sharp_zebra.c +++ b/sharpd/sharp_zebra.c @@ -403,7 +403,8 @@ void route_add(const struct prefix *p, vrf_id_t vrf_id, SET_FLAG(api.flags, ZEBRA_FLAG_ALLOW_RECURSION); SET_FLAG(api.message, ZAPI_MESSAGE_NEXTHOP); - if (nhgid) { + /* Only send via ID if nhgroup has been successfully installed */ + if (nhgid && sharp_nhgroup_id_is_installed(nhgid)) { SET_FLAG(api.message, ZAPI_MESSAGE_NHG); api.nhgid = nhgid; } else { @@ -700,6 +701,33 @@ void sharp_zebra_send_arp(const struct interface *ifp, const struct prefix *p) zclient_send_neigh_discovery_req(zclient, ifp, p); } +static int nhg_notify_owner(ZAPI_CALLBACK_ARGS) +{ + enum zapi_nhg_notify_owner note; + uint32_t id; + + if (!zapi_nhg_notify_decode(zclient->ibuf, &id, ¬e)) + return -1; + + switch (note) { + case ZAPI_NHG_INSTALLED: + sharp_nhgroup_id_set_installed(id, true); + zlog_debug("Installed nhg %u", id); + break; + case ZAPI_NHG_FAIL_INSTALL: + zlog_debug("Failed install of nhg %u", id); + break; + case ZAPI_NHG_REMOVED: + zlog_debug("Removed nhg %u", id); + break; + case ZAPI_NHG_REMOVE_FAIL: + zlog_debug("Failed removal of nhg %u", id); + break; + } + + return 0; +} + void sharp_zebra_init(void) { struct zclient_options opt = {.receive_notify = true}; @@ -716,6 +744,7 @@ void sharp_zebra_init(void) zclient->route_notify_owner = route_notify_owner; zclient->nexthop_update = sharp_nexthop_update; zclient->import_check_update = sharp_nexthop_update; + zclient->nhg_notify_owner = nhg_notify_owner; zclient->redistribute_route_add = sharp_redistribute_route; zclient->redistribute_route_del = sharp_redistribute_route; From 8155e8c592199b27d73b800f471447f27add8826 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Wed, 22 Jul 2020 13:45:47 -0400 Subject: [PATCH 40/57] zebra: add flag track released state of proto NHGS Add a flag to track the released state of a proto-based NHG. This flag is used to know whether the upper level proto has called the *_del API. Typically, the NHG would just get removed and uninstalled at this point but there is a chance we are being sent it while routes are still being owned or we were sent it multiple times. This flag and associated code handles that. Ticket: CM-30369 Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 23 +++++++++++++++++++++-- zebra/zebra_nhg.h | 13 ++++++++++++- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 3d0835d272..6abce30e8e 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -2794,6 +2794,17 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, zebra_nhg_install_kernel(new); if (old) { + /* + * Check to handle recving DEL while routes still in use then + * a replace. + * + * In this case we would have decremented the refcnt already + * but set the FLAG here. Go ahead and increment once to fix + * the misordering we have been sent. + */ + if (CHECK_FLAG(old->flags, NEXTHOP_GROUP_PROTO_RELEASED)) + zebra_nhg_increment_ref(old); + rib_handle_nhg_replace(old, new); /* if this != 1 at this point, we have a bug */ @@ -2833,14 +2844,22 @@ struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id) nhe = zebra_nhg_lookup_id(id); if (!nhe) { - if (IS_ZEBRA_DEBUG_NHG_DETAIL) + if (IS_ZEBRA_DEBUG_NHG) zlog_debug("%s: id %u, lookup failed", __func__, id); return NULL; } + if (CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_PROTO_RELEASED)) { + if (IS_ZEBRA_DEBUG_NHG) + zlog_debug("%s: id %u, already released", __func__, id); + + return NULL; + } + + SET_FLAG(nhe->flags, NEXTHOP_GROUP_PROTO_RELEASED); + if (nhe->refcnt > 1) { - /* TODO: should be warn? */ if (IS_ZEBRA_DEBUG_NHG) zlog_debug( "%s: id %u, still being used by routes refcnt %u", diff --git a/zebra/zebra_nhg.h b/zebra/zebra_nhg.h index 6c9f20d0a4..d8aafbf2ca 100644 --- a/zebra/zebra_nhg.h +++ b/zebra/zebra_nhg.h @@ -109,10 +109,21 @@ struct nhg_hash_entry { */ #define NEXTHOP_GROUP_BACKUP (1 << 4) +/* + * The NHG has been release by an upper level protocol via the + * `zebra_nhg_proto_del()` API. + * + * We use this flag to track this state in case the NHG is still being used + * by routes therefore holding their refcnts as well. Otherwise, the NHG will + * be removed and uninstalled. + * + */ +#define NEXTHOP_GROUP_PROTO_RELEASED (1 << 5) + /* * Track FPM installation status.. */ -#define NEXTHOP_GROUP_FPM (1 << 5) +#define NEXTHOP_GROUP_FPM (1 << 6) }; /* Was this one we created, either this session or previously? */ From e270f004ae4cc2d33108a0931d8c9db649ed47dd Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Wed, 22 Jul 2020 14:02:11 -0400 Subject: [PATCH 41/57] zebra: multipath number checks with NHG proto Get the multipath number checks working with proto-based NHG message decoding in zapi_msg.c Modify the function that checks this for routes to work without being passed a prefix as is the case with NHG creates. Signed-off-by: Stephen Worley --- zebra/zapi_msg.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 21f40ce7c2..ae38b2955f 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -1445,11 +1445,14 @@ bool zserv_nexthop_num_warn(const char *caller, const struct prefix *p, if (nexthop_num > zrouter.multipath_num) { char buff[PREFIX2STR_BUFFER]; - prefix2str(p, buff, sizeof(buff)); + if (p) + prefix2str(p, buff, sizeof(buff)); + flog_warn( EC_ZEBRA_MORE_NH_THAN_MULTIPATH, "%s: Prefix %s has %d nexthops, but we can only use the first %d", - caller, buff, nexthop_num, zrouter.multipath_num); + caller, (p ? buff : "(NULL)"), nexthop_num, + zrouter.multipath_num); return true; } @@ -1767,21 +1770,17 @@ static void zread_nhg_reader(ZAPI_HANDLER_ARGS) size_t nhops, i; struct zapi_nexthop zapi_nexthops[MULTIPATH_NUM]; struct nexthop_group *nhg = NULL; - struct prefix p; uint16_t proto; struct nhg_hash_entry *nhe; - memset(&p, 0, sizeof(p)); - s = msg; STREAM_GETW(s, proto); STREAM_GETL(s, id); STREAM_GETW(s, nhops); - // TODO: Can't use this without a prefix. - // if (zserv_nexthop_num_warn(__func__, &p, nhops)) - // return; + if (zserv_nexthop_num_warn(__func__, NULL, nhops)) + return; if (nhops <= 0) { flog_warn(EC_ZEBRA_NEXTHOP_CREATION_FAILED, @@ -1800,8 +1799,8 @@ static void zread_nhg_reader(ZAPI_HANDLER_ARGS) } } - if (!zapi_read_nexthops(client, &p, zapi_nexthops, 0, 0, nhops, 0, &nhg, - NULL)) { + if (!zapi_read_nexthops(client, NULL, zapi_nexthops, 0, 0, nhops, 0, + &nhg, NULL)) { flog_warn(EC_ZEBRA_NEXTHOP_CREATION_FAILED, "%s: Nexthop Group Creation failed", __func__); From 8b2d3a0fb616dcfba71fa8058d5e811eaf421c48 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Wed, 22 Jul 2020 14:04:07 -0400 Subject: [PATCH 42/57] zebra: clean up the NHG proto zapi code a bit Clean up the function names and remove some TODOs that are no longer needed/hacks we used for testing. Signed-off-by: Stephen Worley --- zebra/zapi_msg.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index ae38b2955f..5bf8fd302d 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -1749,7 +1749,6 @@ static void zread_nhg_del(ZAPI_HANDLER_ARGS) nhe = zebra_nhg_proto_del(id); if (nhe) { - /* TODO: just decrement for now */ zebra_nhg_decrement_ref(nhe); nhg_notify(proto, client->instance, id, ZAPI_NHG_REMOVED); } else @@ -1763,7 +1762,7 @@ stream_failure: return; } -static void zread_nhg_reader(ZAPI_HANDLER_ARGS) +static void zread_nhg_add(ZAPI_HANDLER_ARGS) { struct stream *s; uint32_t id; @@ -1808,12 +1807,9 @@ static void zread_nhg_reader(ZAPI_HANDLER_ARGS) } /* - * Install the nhg + * Create the nhg */ - - // TODO: Forcing AF_UNSPEC/AF_IP for now - nhe = zebra_nhg_proto_add(id, proto, nhg, - ((nhops > 1) ? AFI_UNSPEC : AFI_IP)); + nhe = zebra_nhg_proto_add(id, proto, nhg, 0); nexthop_group_delete(&nhg); @@ -3246,7 +3242,7 @@ void (*const zserv_handlers[])(ZAPI_HANDLER_ARGS) = { [ZEBRA_MLAG_FORWARD_MSG] = zebra_mlag_forward_client_msg, [ZEBRA_CLIENT_CAPABILITIES] = zread_client_capabilities, [ZEBRA_NEIGH_DISCOVER] = zread_neigh_discover, - [ZEBRA_NHG_ADD] = zread_nhg_reader, + [ZEBRA_NHG_ADD] = zread_nhg_add, [ZEBRA_NHG_DEL] = zread_nhg_del, }; From ff9aca4f8d70a45e84c5bdf5503b68bea1f74d13 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Tue, 28 Jul 2020 17:36:51 -0400 Subject: [PATCH 43/57] lib,zebra,sharpd: clang format Clang format for NHG API and sharpd patches. Signed-off-by: Stephen Worley --- lib/zclient.c | 4 +-- lib/zclient.h | 5 ++-- sharpd/sharp_nht.c | 2 +- sharpd/sharp_vty.c | 6 ++--- sharpd/sharp_zebra.c | 8 +++--- zebra/rt_netlink.c | 2 +- zebra/zapi_msg.c | 59 ++++++++++++++++++++++---------------------- zebra/zebra_vty.c | 14 +++++------ 8 files changed, 48 insertions(+), 52 deletions(-) diff --git a/lib/zclient.c b/lib/zclient.c index 2e65e3c9f7..dfb409426d 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -1053,8 +1053,8 @@ static void zclient_nhg_writer(struct stream *s, uint16_t proto, int cmd, stream_putw_at(s, 0, stream_get_endp(s)); } -extern void zclient_nhg_add(struct zclient *zclient, uint32_t id, - size_t nhops, struct zapi_nexthop *znh) +extern void zclient_nhg_add(struct zclient *zclient, uint32_t id, size_t nhops, + struct zapi_nexthop *znh) { struct stream *s = zclient->obuf; diff --git a/lib/zclient.h b/lib/zclient.h index 3ac9ca9456..0fbe1de290 100644 --- a/lib/zclient.h +++ b/lib/zclient.h @@ -374,7 +374,7 @@ struct zclient { #define ZAPI_MESSAGE_SRCPFX 0x20 /* Backup nexthops are present */ #define ZAPI_MESSAGE_BACKUP_NEXTHOPS 0x40 -#define ZAPI_MESSAGE_NHG 0x80 +#define ZAPI_MESSAGE_NHG 0x80 /* * This should only be used by a DAEMON that needs to communicate @@ -898,8 +898,7 @@ bool zapi_ipset_notify_decode(struct stream *s, uint32_t *unique, enum zapi_ipset_notify_owner *note); -extern void zclient_nhg_add(struct zclient *zclient, - uint32_t id, size_t nhops, +extern void zclient_nhg_add(struct zclient *zclient, uint32_t id, size_t nhops, struct zapi_nexthop *znh); extern void zclient_nhg_del(struct zclient *zclient, uint32_t id); diff --git a/sharpd/sharp_nht.c b/sharpd/sharp_nht.c index 4c32dc279d..38de8f217a 100644 --- a/sharpd/sharp_nht.c +++ b/sharpd/sharp_nht.c @@ -109,7 +109,7 @@ static struct sharp_nhg *sharp_nhgroup_find_id(uint32_t id) * to sharpd with another RB tree for just IDs */ - frr_each(sharp_nhg_rb, &nhg_head, lookup) { + frr_each (sharp_nhg_rb, &nhg_head, lookup) { if (lookup->id == id) return lookup; } diff --git a/sharpd/sharp_vty.c b/sharpd/sharp_vty.c index 2903588c13..d062f027ab 100644 --- a/sharpd/sharp_vty.c +++ b/sharpd/sharp_vty.c @@ -299,9 +299,9 @@ DEFPY (install_routes, sg.r.inst = instance; sg.r.vrf_id = vrf->vrf_id; rts = routes; - sharp_install_routes_helper(&prefix, sg.r.vrf_id, sg.r.inst, - nhgid, &sg.r.nhop_group, - &sg.r.backup_nhop_group, rts); + sharp_install_routes_helper(&prefix, sg.r.vrf_id, sg.r.inst, nhgid, + &sg.r.nhop_group, &sg.r.backup_nhop_group, + rts); return CMD_SUCCESS; } diff --git a/sharpd/sharp_zebra.c b/sharpd/sharp_zebra.c index 6be1176db5..03def0d9ba 100644 --- a/sharpd/sharp_zebra.c +++ b/sharpd/sharp_zebra.c @@ -288,8 +288,7 @@ static void handle_repeated(bool installed) if (!installed) { sg.r.installed_routes = 0; sharp_install_routes_helper(&p, sg.r.vrf_id, sg.r.inst, - sg.r.nhgid, - &sg.r.nhop_group, + sg.r.nhgid, &sg.r.nhop_group, &sg.r.backup_nhop_group, sg.r.total_routes); } @@ -383,9 +382,8 @@ void nhg_del(uint32_t id) zclient_send_message(zclient); } -void route_add(const struct prefix *p, vrf_id_t vrf_id, - uint8_t instance, uint32_t nhgid, - const struct nexthop_group *nhg, +void route_add(const struct prefix *p, vrf_id_t vrf_id, uint8_t instance, + uint32_t nhgid, const struct nexthop_group *nhg, const struct nexthop_group *backup_nhg) { struct zapi_route api; diff --git a/zebra/rt_netlink.c b/zebra/rt_netlink.c index 46de751f95..1ce3c435fe 100644 --- a/zebra/rt_netlink.c +++ b/zebra/rt_netlink.c @@ -2269,7 +2269,7 @@ nexthop_done: nh->vrf_id, label_buf); } - req->nhm.nh_protocol = zebra2proto(type); +req->nhm.nh_protocol = zebra2proto(type); } else if (cmd != RTM_DELNEXTHOP) { flog_err( diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 5bf8fd302d..0e4270079c 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -1508,8 +1508,7 @@ static struct nexthop *nexthop_from_zapi(const struct zapi_nexthop *api_nh, memcpy(&(vtep_ip.ipaddr_v4), &(api_nh->gate.ipv4), sizeof(struct in_addr)); zebra_vxlan_evpn_vrf_route_add( - api_nh->vrf_id, &api_nh->rmac, - &vtep_ip, p); + api_nh->vrf_id, &api_nh->rmac, &vtep_ip, p); } break; case NEXTHOP_TYPE_IPV6: @@ -1542,8 +1541,7 @@ static struct nexthop *nexthop_from_zapi(const struct zapi_nexthop *api_nh, memcpy(&vtep_ip.ipaddr_v6, &(api_nh->gate.ipv6), sizeof(struct in6_addr)); zebra_vxlan_evpn_vrf_route_add( - api_nh->vrf_id, &api_nh->rmac, - &vtep_ip, p); + api_nh->vrf_id, &api_nh->rmac, &vtep_ip, p); } break; case NEXTHOP_TYPE_BLACKHOLE: @@ -1660,7 +1658,8 @@ static bool zapi_read_nexthops(struct zserv *client, struct prefix *p, return false; } - if (bnhg && CHECK_FLAG(nexthop->flags, NEXTHOP_FLAG_HAS_BACKUP)) { + if (bnhg + && CHECK_FLAG(nexthop->flags, NEXTHOP_FLAG_HAS_BACKUP)) { if (IS_ZEBRA_DEBUG_RECV) { nexthop2str(nexthop, nhbuf, sizeof(nhbuf)); zlog_debug("%s: backup nh %s with BACKUP flag!", @@ -1707,18 +1706,19 @@ static bool zapi_read_nexthops(struct zserv *client, struct prefix *p, if (png) { /* Add new nexthop to temporary list. This list is - * canonicalized - sorted - so that it can be hashed later - * in route processing. We expect that the sender has sent - * the list sorted, and the zapi client api attempts to enforce - * that, so this should be inexpensive - but it is necessary - * to support shared nexthop-groups. + * canonicalized - sorted - so that it can be hashed + * later in route processing. We expect that the sender + * has sent the list sorted, and the zapi client api + * attempts to enforce that, so this should be + * inexpensive - but it is necessary to support shared + * nexthop-groups. */ nexthop_group_add_sorted(ng, nexthop); } if (bnhg) { - /* Note that the order of the backup nexthops is significant, - * so we don't sort this list as we do the primary nexthops, - * we just append. + /* Note that the order of the backup nexthops is + * significant, so we don't sort this list as we do the + * primary nexthops, we just append. */ if (last_nh) NEXTHOP_APPEND(last_nh, nexthop); @@ -1792,8 +1792,7 @@ static void zread_nhg_add(ZAPI_HANDLER_ARGS) if (zapi_nexthop_decode(s, znh, 0, 0) != 0) { flog_warn(EC_ZEBRA_NEXTHOP_CREATION_FAILED, - "%s: Nexthop creation failed", - __func__); + "%s: Nexthop creation failed", __func__); return; } } @@ -1801,8 +1800,7 @@ static void zread_nhg_add(ZAPI_HANDLER_ARGS) if (!zapi_read_nexthops(client, NULL, zapi_nexthops, 0, 0, nhops, 0, &nhg, NULL)) { flog_warn(EC_ZEBRA_NEXTHOP_CREATION_FAILED, - "%s: Nexthop Group Creation failed", - __func__); + "%s: Nexthop Group Creation failed", __func__); return; } @@ -1827,9 +1825,10 @@ static void zread_nhg_add(ZAPI_HANDLER_ARGS) return; stream_failure: - flog_warn(EC_ZEBRA_NEXTHOP_CREATION_FAILED, - "%s: Nexthop Group creation failed with some sort of stream read failure", - __func__); + flog_warn( + EC_ZEBRA_NEXTHOP_CREATION_FAILED, + "%s: Nexthop Group creation failed with some sort of stream read failure", + __func__); return; } @@ -1881,22 +1880,24 @@ static void zread_route_add(ZAPI_HANDLER_ARGS) if (!CHECK_FLAG(api.message, ZAPI_MESSAGE_NHG) && (!CHECK_FLAG(api.message, ZAPI_MESSAGE_NEXTHOP) || api.nexthop_num == 0)) { - flog_warn(EC_ZEBRA_RX_ROUTE_NO_NEXTHOPS, - "%s: received a route without nexthops for prefix %pFX from client %s", - __func__, &api.prefix, - zebra_route_string(client->proto)); + flog_warn( + EC_ZEBRA_RX_ROUTE_NO_NEXTHOPS, + "%s: received a route without nexthops for prefix %pFX from client %s", + __func__, &api.prefix, + zebra_route_string(client->proto)); XFREE(MTYPE_RE, re); return; } /* Report misuse of the backup flag */ - if (CHECK_FLAG(api.message, ZAPI_MESSAGE_BACKUP_NEXTHOPS) && - api.backup_nexthop_num == 0) { + if (CHECK_FLAG(api.message, ZAPI_MESSAGE_BACKUP_NEXTHOPS) + && api.backup_nexthop_num == 0) { if (IS_ZEBRA_DEBUG_RECV || IS_ZEBRA_DEBUG_EVENT) - zlog_debug("%s: client %s: BACKUP flag set but no backup nexthops, prefix %pFX", - __func__, - zebra_route_string(client->proto), &api.prefix); + zlog_debug( + "%s: client %s: BACKUP flag set but no backup nexthops, prefix %pFX", + __func__, zebra_route_string(client->proto), + &api.prefix); } if (CHECK_FLAG(api.message, ZAPI_MESSAGE_NHG)) diff --git a/zebra/zebra_vty.c b/zebra/zebra_vty.c index 7e73b3f724..6785151705 100644 --- a/zebra/zebra_vty.c +++ b/zebra/zebra_vty.c @@ -1566,14 +1566,12 @@ DEFPY_HIDDEN(nexthop_group_use_enable, return CMD_SUCCESS; } -DEFPY_HIDDEN (proto_nexthop_group_only, - proto_nexthop_group_only_cmd, - "[no] zebra nexthop proto only", - NO_STR - ZEBRA_STR - "Nexthop configuration\n" - "Configure exclusive use of proto nexthops\n" - "Only use proto nexthops\n") +DEFPY_HIDDEN(proto_nexthop_group_only, proto_nexthop_group_only_cmd, + "[no] zebra nexthop proto only", + NO_STR ZEBRA_STR + "Nexthop configuration\n" + "Configure exclusive use of proto nexthops\n" + "Only use proto nexthops\n") { zebra_nhg_set_proto_nexthops_only(!no); return CMD_SUCCESS; From 73937edb73465713c1994202624ec85a306f4e45 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Wed, 29 Jul 2020 13:11:37 -0400 Subject: [PATCH 44/57] zebra,sharpd: checkpatch fixes Check patches fixes for NHG API pathes. Signed-off-by: Stephen Worley --- sharpd/sharp_nht.c | 14 +++++--------- zebra/zapi_msg.c | 4 ++-- zebra/zebra_nhg.c | 4 ++-- 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/sharpd/sharp_nht.c b/sharpd/sharp_nht.c index 38de8f217a..9f31552720 100644 --- a/sharpd/sharp_nht.c +++ b/sharpd/sharp_nht.c @@ -123,10 +123,9 @@ static void sharp_nhgroup_add_cb(const char *name) snhg = XCALLOC(MTYPE_NHG, sizeof(*snhg)); snhg->id = sharp_get_next_nhid(); - strncpy(snhg->name, name, sizeof(snhg->name)); + strlcpy(snhg->name, name, sizeof(snhg->name)); sharp_nhg_rb_add(&nhg_head, snhg); - return; } static void sharp_nhgroup_add_nexthop_cb(const struct nexthop_group_cmd *nhgc, @@ -135,11 +134,10 @@ static void sharp_nhgroup_add_nexthop_cb(const struct nexthop_group_cmd *nhgc, struct sharp_nhg lookup; struct sharp_nhg *snhg; - strncpy(lookup.name, nhgc->name, sizeof(lookup.name)); + strlcpy(lookup.name, nhgc->name, sizeof(lookup.name)); snhg = sharp_nhg_rb_find(&nhg_head, &lookup); nhg_add(snhg->id, &nhgc->nhg); - return; } static void sharp_nhgroup_del_nexthop_cb(const struct nexthop_group_cmd *nhgc, @@ -148,11 +146,10 @@ static void sharp_nhgroup_del_nexthop_cb(const struct nexthop_group_cmd *nhgc, struct sharp_nhg lookup; struct sharp_nhg *snhg; - strncpy(lookup.name, nhgc->name, sizeof(lookup.name)); + strlcpy(lookup.name, nhgc->name, sizeof(lookup.name)); snhg = sharp_nhg_rb_find(&nhg_head, &lookup); nhg_add(snhg->id, &nhgc->nhg); - return; } static void sharp_nhgroup_delete_cb(const char *name) @@ -160,7 +157,7 @@ static void sharp_nhgroup_delete_cb(const char *name) struct sharp_nhg lookup; struct sharp_nhg *snhg; - strncpy(lookup.name, name, sizeof(lookup.name)); + strlcpy(lookup.name, name, sizeof(lookup.name)); snhg = sharp_nhg_rb_find(&nhg_head, &lookup); if (!snhg) return; @@ -168,7 +165,6 @@ static void sharp_nhgroup_delete_cb(const char *name) nhg_del(snhg->id); sharp_nhg_rb_del(&nhg_head, snhg); XFREE(MTYPE_NHG, snhg); - return; } uint32_t sharp_nhgroup_get_id(const char *name) @@ -176,7 +172,7 @@ uint32_t sharp_nhgroup_get_id(const char *name) struct sharp_nhg lookup; struct sharp_nhg *snhg; - strncpy(lookup.name, name, sizeof(lookup.name)); + strlcpy(lookup.name, name, sizeof(lookup.name)); snhg = sharp_nhg_rb_find(&nhg_head, &lookup); if (!snhg) return 0; diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 0e4270079c..6a897166cf 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -1817,9 +1817,9 @@ static void zread_nhg_add(ZAPI_HANDLER_ARGS) * * Resolution is going to need some more work. */ - if (nhe) { + if (nhe) nhg_notify(proto, client->instance, id, ZAPI_NHG_INSTALLED); - } else + else nhg_notify(proto, client->instance, id, ZAPI_NHG_FAIL_INSTALL); return; diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 6abce30e8e..23bd5bb2c0 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -53,7 +53,7 @@ uint32_t id_counter; /* */ static bool g_nexthops_enabled = true; -static bool proto_nexthops_only = false; +static bool proto_nexthops_only; static struct nhg_hash_entry *depends_find(const struct nexthop *nh, afi_t afi, int type); @@ -70,7 +70,7 @@ static struct nhg_backup_info * nhg_backup_copy(const struct nhg_backup_info *orig); /* Helper function for getting the next allocatable ID */ -static uint32_t nhg_get_next_id() +static uint32_t nhg_get_next_id(void) { while (1) { id_counter++; From 70f3cda6c1b56a22dd65eefead5aaa5f393f5e7d Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Mon, 3 Aug 2020 14:34:52 -0400 Subject: [PATCH 45/57] zebra: reject proto NHGs of blackhole/interface Reject proto NHGs of type blackhole/interface for now. We need to think a bit more about how to resolve these given the linux kernel needs to know the Address Family of the routes that will use them and install it with them. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 23bd5bb2c0..cfc5a4f323 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -2759,10 +2759,26 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, * Once resolution is figured out, we won't need this! */ for (ALL_NEXTHOPS_PTR(nhg, newhop)) { + if (newhop->type == NEXTHOP_TYPE_BLACKHOLE) { + if (IS_ZEBRA_DEBUG_NHG) + zlog_debug( + "%s: id %u, blackhole nexthop not supported", + __func__, id); + return NULL; + } + + if (newhop->type == NEXTHOP_TYPE_IFINDEX) { + if (IS_ZEBRA_DEBUG_NHG) + zlog_debug( + "%s: id %u, nexthop without gateway not supported", + __func__, id); + return NULL; + } + if (!newhop->ifindex) { if (IS_ZEBRA_DEBUG_NHG) zlog_debug( - "%s: id %u, nexthop without ifindex passed to add", + "%s: id %u, nexthop without ifindex is not supported", __func__, id); return NULL; } From 8f4d7212f5646d509744a0afc8bf739fd08e5f68 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Mon, 3 Aug 2020 14:43:01 -0400 Subject: [PATCH 46/57] tests: add topotest for NHG Proto APIs Add a topotest for basic NHG Proto Add/Del/Replace. Signed-off-by: Stephen Worley --- .../test_all_protocol_startup.py | 54 ++++++++++++++++--- 1 file changed, 46 insertions(+), 8 deletions(-) diff --git a/tests/topotests/all-protocol-startup/test_all_protocol_startup.py b/tests/topotests/all-protocol-startup/test_all_protocol_startup.py index 2a00398e28..f2af5fdc5c 100755 --- a/tests/topotests/all-protocol-startup/test_all_protocol_startup.py +++ b/tests/topotests/all-protocol-startup/test_all_protocol_startup.py @@ -374,26 +374,36 @@ def route_get_nhg_id(route_str): nhg_id = int(match.group(1)) return nhg_id -def verify_nexthop_group(nhg_id, recursive=False): +def verify_nexthop_group(nhg_id, recursive=False, ecmp=0): # Verify NHG is valid/installed output = net["r1"].cmd('vtysh -c "show nexthop-group rib %d"' % nhg_id) match = re.search(r"Valid", output) assert match is not None, "Nexthop Group ID=%d not marked Valid" % nhg_id - # If recursive, we need to look at its resolved group - if recursive: - match = re.search(r"Depends: \((\d+)\)", output) - resolved_id = int(match.group(1)) - verify_nexthop_group(resolved_id, False) + if ecmp or recursive: + match = re.search(r"Depends:.*\n", output) + assert match is not None, "Nexthop Group ID=%d has no depends" % nhg_id + + # list of IDs in group + depends = re.findall(r"\((\d+)\)", match.group(0)) + + if ecmp: + assert (len(depends) == ecmp), "Nexthop Group ID=%d doesn't match ecmp size" % nhg_id + else: + # If recursive, we need to look at its resolved group + assert (len(depends) == 1), "Nexthop Group ID=%d should only have one recursive depend" % nhg_id + resolved_id = int(depends[0]) + verify_nexthop_group(resolved_id, False) + else: match = re.search(r"Installed", output) assert match is not None, "Nexthop Group ID=%d not marked Installed" % nhg_id -def verify_route_nexthop_group(route_str, recursive=False): +def verify_route_nexthop_group(route_str, recursive=False, ecmp=0): # Verify route and that zebra created NHGs for and they are valid/installed nhg_id = route_get_nhg_id(route_str) - verify_nexthop_group(nhg_id, recursive) + verify_nexthop_group(nhg_id, recursive, ecmp) def test_nexthop_groups(): global fatal_error @@ -1101,6 +1111,34 @@ def test_nexthop_groups_with_route_maps(): net["r1"].cmd('vtysh -c "c t" -c "no route-map NOPE"') net["r1"].cmd('vtysh -c "c t" -c "no ip prefix-list NOPE seq 5 permit %s/32"' % permit_route_str) +def test_nexthop_group_replace(): + global fatal_error + global net + + # Skip if previous fatal error condition is raised + if (fatal_error != ""): + pytest.skip(fatal_error) + + print("\n\n** Verifying Nexthop Groups") + print("******************************************\n") + + ### Nexthop Group Tests + + ## 2-Way ECMP Directly Connected + + net["r1"].cmd('vtysh -c "c t" -c "nexthop-group replace" -c "nexthop 1.1.1.1 r1-eth1 onlink" -c "nexthop 1.1.1.2 r1-eth2 onlink"') + + # Create with sharpd using nexthop-group + net["r1"].cmd('vtysh -c "sharp install routes 3.3.3.1 nexthop-group replace 1"') + + verify_route_nexthop_group("3.3.3.1/32") + + # Change the nexthop group + net["r1"].cmd('vtysh -c "c t" -c "nexthop-group replace" -c "no nexthop 1.1.1.1 r1-eth1 onlink" -c "nexthop 1.1.1.3 r1-eth1 onlink" -c "nexthop 1.1.1.4 r1-eth4 onlink"') + + # Verify it updated. We can just check install and ecmp count here. + verify_route_nexthop_group("3.3.3.1/32", False, 3) + def test_mpls_interfaces(): global fatal_error global net From 391c7a3b18c4892313b793740ca3612a67a633d8 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Fri, 14 Aug 2020 13:32:38 -0400 Subject: [PATCH 47/57] lib: add proto NHG Notif header to log command types Add the proto Nexthop Group Notify Owner header to the log command types for string conversion. Signed-off-by: Stephen Worley --- lib/log.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/log.c b/lib/log.c index 91d325efb2..b629658f75 100644 --- a/lib/log.c +++ b/lib/log.c @@ -454,7 +454,8 @@ static const struct zebra_desc_table command_types[] = { DESC_ENTRY(ZEBRA_OPAQUE_UNREGISTER), DESC_ENTRY(ZEBRA_NEIGH_DISCOVER), DESC_ENTRY(ZEBRA_NHG_ADD), - DESC_ENTRY(ZEBRA_NHG_DEL)}; + DESC_ENTRY(ZEBRA_NHG_DEL), + DESC_ENTRY(ZEBRA_NHG_NOTIFY_OWNER)}; #undef DESC_ENTRY static const struct zebra_desc_table unknown = {0, "unknown", '?'}; From 8f830b8c64622099d4a6d3cc12dbc2f8009b45e2 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Tue, 1 Sep 2020 14:53:09 -0400 Subject: [PATCH 48/57] zebra: use list to mark for removal when scoring In scoring our NHEs during shutdown there is a chance we could release mutliple NHEs at the same time during one iteration. This can cause memory corruption if the two being released are directly next to each other in the hash table. hash_iterate accounts for releasing one during the iteration but not two by setting hbnext before release but if hbnext is also freed, we obviously can have a problem. Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index cfc5a4f323..ac8f4745d6 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -2893,7 +2893,7 @@ struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id) struct nhg_score_proto_iter { int type; - unsigned long found; + struct list *found; }; static void zebra_nhg_score_proto_entry(struct hash_bucket *bucket, void *arg) @@ -2906,27 +2906,42 @@ static void zebra_nhg_score_proto_entry(struct hash_bucket *bucket, void *arg) /* Needs to match type and outside zebra ID space */ if (nhe->type == iter->type && nhe->id >= ZEBRA_NHG_PROTO_LOWER) { - iter->found++; - if (IS_ZEBRA_DEBUG_NHG_DETAIL) zlog_debug( "%s: found nhe %p (%u), vrf %d, type %s after client disconnect", __func__, nhe, nhe->id, nhe->vrf_id, zebra_route_string(nhe->type)); - /* This should be the last ref if we remove client routes too */ - zebra_nhg_decrement_ref(nhe); + /* Add to removal list */ + listnode_add(iter->found, nhe); } } /* Remove specific by proto NHGs */ unsigned long zebra_nhg_score_proto(int type) { + struct nhg_hash_entry *nhe; struct nhg_score_proto_iter iter = {}; + struct listnode *ln; + unsigned long count; iter.type = type; + iter.found = list_new(); + /* Find matching entries to remove */ hash_iterate(zrouter.nhgs_id, zebra_nhg_score_proto_entry, &iter); - return iter.found; + /* Now remove them */ + for (ALL_LIST_ELEMENTS_RO(iter.found, ln, nhe)) { + /* + * This should be the last ref if we remove client routes too, + * and thus should remove and free them. + */ + zebra_nhg_decrement_ref(nhe); + } + + count = iter.found->count; + list_delete(&iter.found); + + return count; } From 3509dd49c0b826b200b68e728ebb9707a91e3079 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Tue, 1 Sep 2020 16:02:12 -0400 Subject: [PATCH 49/57] lib: add doc to clear-up hash_iterate multi deletion Add some header documentation to make it clear that you cannot delete more than one item during each iteration. Doing so could cause memory corruption for next pointer if its also deleted from the table. Signed-off-by: Stephen Worley --- lib/hash.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/hash.h b/lib/hash.h index e7ba3187f5..00953ff3b3 100644 --- a/lib/hash.h +++ b/lib/hash.h @@ -236,7 +236,8 @@ extern void *hash_release(struct hash *hash, void *data); * Iterate over the elements in a hash table. * * It is safe to delete items passed to the iteration function from the hash - * table during iteration. Please note that adding entries to the hash + * table during iteration. More than one item cannot be deleted during each + * iteration. Please note that adding entries to the hash * during the walk will cause undefined behavior in that some new entries * will be walked and some will not. So do not do this. * From 3d3a9dc8a7873f0384492f36a910571da1de142d Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Thu, 3 Sep 2020 13:04:10 -0400 Subject: [PATCH 50/57] zebra: limit no re-install to NHG PROTO using routes Limit the not re-installation of routes with the same NHG ID to routes that are using the new NHG PROTO API. This would only include sharpd and EVPN-MH for now. Signed-off-by: Stephen Worley --- zebra/zebra_dplane.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/zebra/zebra_dplane.c b/zebra/zebra_dplane.c index 77abe8bb6d..d888ceff2c 100644 --- a/zebra/zebra_dplane.c +++ b/zebra/zebra_dplane.c @@ -2367,7 +2367,8 @@ dplane_route_update_internal(struct route_node *rn, */ if ((dplane_ctx_get_type(ctx) == dplane_ctx_get_old_type(ctx)) && (dplane_ctx_get_nhe_id(ctx) - == dplane_ctx_get_old_nhe_id(ctx))) { + == dplane_ctx_get_old_nhe_id(ctx)) + && (dplane_ctx_get_nhe_id(ctx) >= ZEBRA_NHG_PROTO_LOWER)) { struct nexthop *nexthop; if (IS_ZEBRA_DEBUG_DPLANE) From 841f77ff043f31889b32de7373c1710b17290181 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Thu, 3 Sep 2020 13:44:14 -0400 Subject: [PATCH 51/57] zebra: free ctx if we skip replace for NHG PROTO routes Free the ctx if we decide we dont need to do anything with this route update. Signed-off-by: Stephen Worley --- zebra/zebra_dplane.c | 1 + 1 file changed, 1 insertion(+) diff --git a/zebra/zebra_dplane.c b/zebra/zebra_dplane.c index d888ceff2c..76d7d00e4a 100644 --- a/zebra/zebra_dplane.c +++ b/zebra/zebra_dplane.c @@ -2388,6 +2388,7 @@ dplane_route_update_internal(struct route_node *rn, NEXTHOP_FLAG_FIB); } + dplane_ctx_free(&ctx); return ZEBRA_DPLANE_REQUEST_SUCCESS; } From 54a701e4c1d61a7bc72161d501c5e4ee5204c929 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Fri, 11 Sep 2020 17:59:30 -0400 Subject: [PATCH 52/57] sharp: add check for num_nh > multipath Add a check for installing nexthop_group greater than multipath number. Truncate if we hit it and log a warning to the user. Signed-off-by: Stephen Worley --- sharpd/sharp_zebra.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/sharpd/sharp_zebra.c b/sharpd/sharp_zebra.c index 03def0d9ba..edbc7460e0 100644 --- a/sharpd/sharp_zebra.c +++ b/sharpd/sharp_zebra.c @@ -365,6 +365,13 @@ void nhg_add(uint32_t id, const struct nexthop_group *nhg) struct nexthop *nh; for (ALL_NEXTHOPS_PTR(nhg, nh)) { + if (nexthop_num >= MULTIPATH_NUM) { + zlog_warn( + "%s: number of nexthops greater than max multipath size, truncating", + __func__); + break; + } + api_nh = &nh_array[nexthop_num]; zapi_nexthop_from_nexthop(api_nh, nh); From aaa42e056fe2e52c74c8e9241948cc76b61e2ee2 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Tue, 15 Sep 2020 13:42:49 -0400 Subject: [PATCH 53/57] zebra: add type to nhg_prot_del API for sanity check Add type to the nhg_proto_del API params for sanity checking that the types of the route sent by the proto matches the type found with the ID. Signed-off-by: Stephen Worley --- zebra/zapi_msg.c | 2 +- zebra/zebra_nhg.c | 11 ++++++++++- zebra/zebra_nhg.h | 2 +- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 6a897166cf..7ed3a41ae1 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -1746,7 +1746,7 @@ static void zread_nhg_del(ZAPI_HANDLER_ARGS) * Delete the received nhg id */ - nhe = zebra_nhg_proto_del(id); + nhe = zebra_nhg_proto_del(id, proto); if (nhe) { zebra_nhg_decrement_ref(nhe); diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index ac8f4745d6..1ed7ff00e5 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -2853,7 +2853,7 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, } /* Delete NHE from upper level proto, caller must decrement ref */ -struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id) +struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id, int type) { struct nhg_hash_entry *nhe; @@ -2866,6 +2866,15 @@ struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id) return NULL; } + if (type != nhe->type) { + if (IS_ZEBRA_DEBUG_NHG) + zlog_debug( + "%s: id %u, type %s mismatch, sent by %s, ignoring", + __func__, id, zebra_route_string(nhe->type), + zebra_route_string(type)); + return NULL; + } + if (CHECK_FLAG(nhe->flags, NEXTHOP_GROUP_PROTO_RELEASED)) { if (IS_ZEBRA_DEBUG_NHG) zlog_debug("%s: id %u, already released", __func__, id); diff --git a/zebra/zebra_nhg.h b/zebra/zebra_nhg.h index d8aafbf2ca..052fa65d06 100644 --- a/zebra/zebra_nhg.h +++ b/zebra/zebra_nhg.h @@ -294,7 +294,7 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, * * Caller must decrement ref with zebra_nhg_decrement_ref() when done. */ -struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id); +struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id, int type); /* * Remove specific by proto NHGs. From c6ce9334b5387f482af4112ed1ffe6a49debe352 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Tue, 22 Sep 2020 15:27:35 -0400 Subject: [PATCH 54/57] lib,sharpd: align zapi NHG apis a bit Align the zapi NHG apis to be more consistent with the zapi_route apis. Add a struct zapi_nhg to use for encodings as well. Signed-off-by: Stephen Worley --- lib/zclient.c | 55 ++++++++++++++++++++------------------------ lib/zclient.h | 14 ++++++++--- sharpd/sharp_zebra.c | 21 +++++++++-------- 3 files changed, 47 insertions(+), 43 deletions(-) diff --git a/lib/zclient.c b/lib/zclient.c index dfb409426d..13eba4c790 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -1017,49 +1017,44 @@ done: return ret; } -extern void zclient_nhg_del(struct zclient *zclient, uint32_t id) +static int zapi_nhg_encode(struct stream *s, uint16_t proto, int cmd, + struct zapi_nhg *api_nhg) { - struct stream *s = zclient->obuf; + if (cmd != ZEBRA_NHG_DEL && cmd != ZEBRA_NHG_ADD) { + flog_err(EC_LIB_ZAPI_ENCODE, + "%s: Specified zapi NHG command (%d) doesn't exist\n", + __func__, cmd); + return -1; + } stream_reset(s); - zclient_create_header(s, ZEBRA_NHG_DEL, VRF_DEFAULT); - - stream_putw(s, zclient->redist_default); - stream_putl(s, id); - - stream_putw_at(s, 0, stream_get_endp(s)); -} - -static void zclient_nhg_writer(struct stream *s, uint16_t proto, int cmd, - uint32_t id, size_t nhops, - struct zapi_nexthop *znh) -{ - size_t i; - - stream_reset(s); - - zapi_nexthop_group_sort(znh, nhops); - zclient_create_header(s, cmd, VRF_DEFAULT); stream_putw(s, proto); - stream_putl(s, id); - stream_putw(s, nhops); - for (i = 0; i < nhops; i++) { - zapi_nexthop_encode(s, znh, 0, 0); - znh++; + stream_putl(s, api_nhg->id); + + if (cmd == ZEBRA_NHG_ADD) { + zapi_nexthop_group_sort(api_nhg->nexthops, + api_nhg->nexthop_num); + + stream_putw(s, api_nhg->nexthop_num); + + for (int i = 0; i < api_nhg->nexthop_num; i++) + zapi_nexthop_encode(s, &api_nhg->nexthops[i], 0, 0); } stream_putw_at(s, 0, stream_get_endp(s)); + + return 0; } -extern void zclient_nhg_add(struct zclient *zclient, uint32_t id, size_t nhops, - struct zapi_nexthop *znh) +int zclient_nhg_send(struct zclient *zclient, int cmd, struct zapi_nhg *api_nhg) { - struct stream *s = zclient->obuf; + if (zapi_nhg_encode(zclient->obuf, zclient->redist_default, cmd, + api_nhg)) + return -1; - zclient_nhg_writer(s, zclient->redist_default, ZEBRA_NHG_ADD, id, nhops, - znh); + return zclient_send_message(zclient); } int zapi_route_encode(uint8_t cmd, struct stream *s, struct zapi_route *api) diff --git a/lib/zclient.h b/lib/zclient.h index 0fbe1de290..b41f291554 100644 --- a/lib/zclient.h +++ b/lib/zclient.h @@ -438,6 +438,15 @@ struct zapi_nexthop { #define ZAPI_NEXTHOP_FLAG_WEIGHT 0x04 #define ZAPI_NEXTHOP_FLAG_HAS_BACKUP 0x08 /* Nexthop has a backup */ +/* + * ZAPI Nexthop Group. For use with protocol creation of nexthop groups. + */ +struct zapi_nhg { + uint32_t id; + uint16_t nexthop_num; + struct zapi_nexthop nexthops[MULTIPATH_NUM]; +}; + /* * Some of these data structures do not map easily to * a actual data structure size giving different compilers @@ -898,9 +907,8 @@ bool zapi_ipset_notify_decode(struct stream *s, uint32_t *unique, enum zapi_ipset_notify_owner *note); -extern void zclient_nhg_add(struct zclient *zclient, uint32_t id, size_t nhops, - struct zapi_nexthop *znh); -extern void zclient_nhg_del(struct zclient *zclient, uint32_t id); +extern int zclient_nhg_send(struct zclient *zclient, int cmd, + struct zapi_nhg *api_nhg); #define ZEBRA_IPSET_NAME_SIZE 32 diff --git a/sharpd/sharp_zebra.c b/sharpd/sharp_zebra.c index edbc7460e0..50129c2363 100644 --- a/sharpd/sharp_zebra.c +++ b/sharpd/sharp_zebra.c @@ -359,34 +359,35 @@ void vrf_label_add(vrf_id_t vrf_id, afi_t afi, mpls_label_t label) void nhg_add(uint32_t id, const struct nexthop_group *nhg) { - struct zapi_nexthop nh_array[MULTIPATH_NUM]; + struct zapi_nhg api_nhg = {}; struct zapi_nexthop *api_nh; - uint16_t nexthop_num = 0; struct nexthop *nh; + api_nhg.id = id; for (ALL_NEXTHOPS_PTR(nhg, nh)) { - if (nexthop_num >= MULTIPATH_NUM) { + if (api_nhg.nexthop_num >= MULTIPATH_NUM) { zlog_warn( "%s: number of nexthops greater than max multipath size, truncating", __func__); break; } - api_nh = &nh_array[nexthop_num]; + api_nh = &api_nhg.nexthops[api_nhg.nexthop_num]; zapi_nexthop_from_nexthop(api_nh, nh); - nexthop_num++; + api_nhg.nexthop_num++; } - zclient_nhg_add(zclient, id, nexthop_num, nh_array); - - zclient_send_message(zclient); + zclient_nhg_send(zclient, ZEBRA_NHG_ADD, &api_nhg); } void nhg_del(uint32_t id) { - zclient_nhg_del(zclient, id); - zclient_send_message(zclient); + struct zapi_nhg api_nhg = {}; + + api_nhg.id = id; + + zclient_nhg_send(zclient, ZEBRA_NHG_DEL, &api_nhg); } void route_add(const struct prefix *p, vrf_id_t vrf_id, uint8_t instance, From 21735352987e56f11facdc9fb1f4c53602aba9f6 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Fri, 25 Sep 2020 13:48:21 -0400 Subject: [PATCH 55/57] lib,zebra,sharpd: add code for backup proto-NHs but disabled Add the zapi code for encoding/decoding of backup nexthops for when we are ready for it, but disable it for now so that we revert to the old way with them. When zebra gets a proto-NHG with a backup in it, we early fail and tell the upper level proto. In this case sharpd. Sharpd then reverts to the old way of installation with the route. Signed-off-by: Stephen Worley --- lib/zclient.c | 23 +++++-- lib/zclient.h | 8 +++ sharpd/sharp_nht.c | 12 +++- sharpd/sharp_zebra.c | 19 +++++- sharpd/sharp_zebra.h | 3 +- zebra/zapi_msg.c | 152 ++++++++++++++++++++++++++++--------------- zebra/zebra_nhg.c | 8 +++ 7 files changed, 162 insertions(+), 63 deletions(-) diff --git a/lib/zclient.c b/lib/zclient.c index 13eba4c790..b7d240b4e8 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -1017,9 +1017,10 @@ done: return ret; } -static int zapi_nhg_encode(struct stream *s, uint16_t proto, int cmd, - struct zapi_nhg *api_nhg) +int zapi_nhg_encode(struct stream *s, int cmd, struct zapi_nhg *api_nhg) { + int i; + if (cmd != ZEBRA_NHG_DEL && cmd != ZEBRA_NHG_ADD) { flog_err(EC_LIB_ZAPI_ENCODE, "%s: Specified zapi NHG command (%d) doesn't exist\n", @@ -1030,17 +1031,26 @@ static int zapi_nhg_encode(struct stream *s, uint16_t proto, int cmd, stream_reset(s); zclient_create_header(s, cmd, VRF_DEFAULT); - stream_putw(s, proto); + stream_putw(s, api_nhg->proto); stream_putl(s, api_nhg->id); if (cmd == ZEBRA_NHG_ADD) { + /* Nexthops */ zapi_nexthop_group_sort(api_nhg->nexthops, api_nhg->nexthop_num); stream_putw(s, api_nhg->nexthop_num); - for (int i = 0; i < api_nhg->nexthop_num; i++) + for (i = 0; i < api_nhg->nexthop_num; i++) zapi_nexthop_encode(s, &api_nhg->nexthops[i], 0, 0); + + /* Backup nexthops */ + + stream_putw(s, api_nhg->backup_nexthop_num); + + for (i = 0; i < api_nhg->backup_nexthop_num; i++) + zapi_nexthop_encode(s, &api_nhg->backup_nexthops[i], 0, + 0); } stream_putw_at(s, 0, stream_get_endp(s)); @@ -1050,8 +1060,9 @@ static int zapi_nhg_encode(struct stream *s, uint16_t proto, int cmd, int zclient_nhg_send(struct zclient *zclient, int cmd, struct zapi_nhg *api_nhg) { - if (zapi_nhg_encode(zclient->obuf, zclient->redist_default, cmd, - api_nhg)) + api_nhg->proto = zclient->redist_default; + + if (zapi_nhg_encode(zclient->obuf, cmd, api_nhg)) return -1; return zclient_send_message(zclient); diff --git a/lib/zclient.h b/lib/zclient.h index b41f291554..959a101395 100644 --- a/lib/zclient.h +++ b/lib/zclient.h @@ -442,9 +442,14 @@ struct zapi_nexthop { * ZAPI Nexthop Group. For use with protocol creation of nexthop groups. */ struct zapi_nhg { + uint16_t proto; uint32_t id; + uint16_t nexthop_num; struct zapi_nexthop nexthops[MULTIPATH_NUM]; + + uint16_t backup_nexthop_num; + struct zapi_nexthop backup_nexthops[MULTIPATH_NUM]; }; /* @@ -907,6 +912,9 @@ bool zapi_ipset_notify_decode(struct stream *s, uint32_t *unique, enum zapi_ipset_notify_owner *note); + +extern int zapi_nhg_encode(struct stream *s, int cmd, struct zapi_nhg *api_nhg); +extern int zapi_nhg_decode(struct stream *s, int cmd, struct zapi_nhg *api_nhg); extern int zclient_nhg_send(struct zclient *zclient, int cmd, struct zapi_nhg *api_nhg); diff --git a/sharpd/sharp_nht.c b/sharpd/sharp_nht.c index 9f31552720..5b3fe2583e 100644 --- a/sharpd/sharp_nht.c +++ b/sharpd/sharp_nht.c @@ -133,11 +133,15 @@ static void sharp_nhgroup_add_nexthop_cb(const struct nexthop_group_cmd *nhgc, { struct sharp_nhg lookup; struct sharp_nhg *snhg; + struct nexthop_group_cmd *bnhgc = NULL; strlcpy(lookup.name, nhgc->name, sizeof(lookup.name)); snhg = sharp_nhg_rb_find(&nhg_head, &lookup); - nhg_add(snhg->id, &nhgc->nhg); + if (nhgc->backup_list_name[0]) + bnhgc = nhgc_find(nhgc->backup_list_name); + + nhg_add(snhg->id, &nhgc->nhg, (bnhgc ? &bnhgc->nhg : NULL)); } static void sharp_nhgroup_del_nexthop_cb(const struct nexthop_group_cmd *nhgc, @@ -145,11 +149,15 @@ static void sharp_nhgroup_del_nexthop_cb(const struct nexthop_group_cmd *nhgc, { struct sharp_nhg lookup; struct sharp_nhg *snhg; + struct nexthop_group_cmd *bnhgc = NULL; strlcpy(lookup.name, nhgc->name, sizeof(lookup.name)); snhg = sharp_nhg_rb_find(&nhg_head, &lookup); - nhg_add(snhg->id, &nhgc->nhg); + if (nhgc->backup_list_name[0]) + bnhgc = nhgc_find(nhgc->backup_list_name); + + nhg_add(snhg->id, &nhgc->nhg, (bnhgc ? &bnhgc->nhg : NULL)); } static void sharp_nhgroup_delete_cb(const char *name) diff --git a/sharpd/sharp_zebra.c b/sharpd/sharp_zebra.c index 50129c2363..d167e8e277 100644 --- a/sharpd/sharp_zebra.c +++ b/sharpd/sharp_zebra.c @@ -357,7 +357,8 @@ void vrf_label_add(vrf_id_t vrf_id, afi_t afi, mpls_label_t label) zclient_send_vrf_label(zclient, vrf_id, afi, label, ZEBRA_LSP_SHARP); } -void nhg_add(uint32_t id, const struct nexthop_group *nhg) +void nhg_add(uint32_t id, const struct nexthop_group *nhg, + const struct nexthop_group *backup_nhg) { struct zapi_nhg api_nhg = {}; struct zapi_nexthop *api_nh; @@ -378,6 +379,22 @@ void nhg_add(uint32_t id, const struct nexthop_group *nhg) api_nhg.nexthop_num++; } + if (backup_nhg) { + for (ALL_NEXTHOPS_PTR(backup_nhg, nh)) { + if (api_nhg.backup_nexthop_num >= MULTIPATH_NUM) { + zlog_warn( + "%s: number of backup nexthops greater than max multipath size, truncating", + __func__); + break; + } + api_nh = &api_nhg.backup_nexthops + [api_nhg.backup_nexthop_num]; + + zapi_backup_nexthop_from_nexthop(api_nh, nh); + api_nhg.backup_nexthop_num++; + } + } + zclient_nhg_send(zclient, ZEBRA_NHG_ADD, &api_nhg); } diff --git a/sharpd/sharp_zebra.h b/sharpd/sharp_zebra.h index 69d7343cc4..4a767ababf 100644 --- a/sharpd/sharp_zebra.h +++ b/sharpd/sharp_zebra.h @@ -29,7 +29,8 @@ int sharp_zclient_create(uint32_t session_id); int sharp_zclient_delete(uint32_t session_id); extern void vrf_label_add(vrf_id_t vrf_id, afi_t afi, mpls_label_t label); -extern void nhg_add(uint32_t id, const struct nexthop_group *nhg); +extern void nhg_add(uint32_t id, const struct nexthop_group *nhg, + const struct nexthop_group *backup_nhg); extern void nhg_del(uint32_t id); extern void route_add(const struct prefix *p, vrf_id_t, uint8_t instance, uint32_t nhgid, const struct nexthop_group *nhg, diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 7ed3a41ae1..1d8c5c7cc0 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -1732,84 +1732,134 @@ static bool zapi_read_nexthops(struct zserv *client, struct prefix *p, return true; } +int zapi_nhg_decode(struct stream *s, int cmd, struct zapi_nhg *api_nhg) +{ + uint16_t i; + struct zapi_nexthop *znh; + + STREAM_GETW(s, api_nhg->proto); + STREAM_GETL(s, api_nhg->id); + + if (cmd == ZEBRA_NHG_DEL) + goto done; + + /* Nexthops */ + STREAM_GETW(s, api_nhg->nexthop_num); + + if (zserv_nexthop_num_warn(__func__, NULL, api_nhg->nexthop_num)) + return -1; + + if (api_nhg->nexthop_num <= 0) { + flog_warn(EC_ZEBRA_NEXTHOP_CREATION_FAILED, + "%s: No nexthops sent", __func__); + return -1; + } + + for (i = 0; i < api_nhg->nexthop_num; i++) { + znh = &((api_nhg->nexthops)[i]); + + if (zapi_nexthop_decode(s, znh, 0, 0) != 0) { + flog_warn(EC_ZEBRA_NEXTHOP_CREATION_FAILED, + "%s: Nexthop creation failed", __func__); + return -1; + } + } + + /* Backup Nexthops */ + STREAM_GETW(s, api_nhg->backup_nexthop_num); + + if (zserv_nexthop_num_warn(__func__, NULL, api_nhg->backup_nexthop_num)) + return -1; + + for (i = 0; i < api_nhg->backup_nexthop_num; i++) { + znh = &((api_nhg->backup_nexthops)[i]); + + if (zapi_nexthop_decode(s, znh, 0, 0) != 0) { + flog_warn(EC_ZEBRA_NEXTHOP_CREATION_FAILED, + "%s: Backup Nexthop creation failed", + __func__); + return -1; + } + } + +done: + return 0; + +stream_failure: + flog_warn( + EC_ZEBRA_NEXTHOP_CREATION_FAILED, + "%s: Nexthop Group decode failed with some sort of stream read failure", + __func__); + return -1; +} + static void zread_nhg_del(ZAPI_HANDLER_ARGS) { - struct stream *s = msg; - uint32_t id; - uint16_t proto; + struct stream *s; + struct zapi_nhg api_nhg = {}; struct nhg_hash_entry *nhe; - STREAM_GETW(s, proto); - STREAM_GETL(s, id); + s = msg; + if (zapi_nhg_decode(s, hdr->command, &api_nhg) < 0) { + if (IS_ZEBRA_DEBUG_RECV) + zlog_debug("%s: Unable to decode zapi_nhg sent", + __func__); + return; + } /* * Delete the received nhg id */ - nhe = zebra_nhg_proto_del(id, proto); + nhe = zebra_nhg_proto_del(api_nhg.id, api_nhg.proto); if (nhe) { zebra_nhg_decrement_ref(nhe); - nhg_notify(proto, client->instance, id, ZAPI_NHG_REMOVED); + nhg_notify(api_nhg.proto, client->instance, api_nhg.id, + ZAPI_NHG_REMOVED); } else - nhg_notify(proto, client->instance, id, ZAPI_NHG_REMOVE_FAIL); - - return; - -stream_failure: - flog_warn(EC_ZEBRA_NEXTHOP_CREATION_FAILED, - "%s: Nexthop group deletion failed", __func__); - return; + nhg_notify(api_nhg.proto, client->instance, api_nhg.id, + ZAPI_NHG_REMOVE_FAIL); } static void zread_nhg_add(ZAPI_HANDLER_ARGS) { struct stream *s; - uint32_t id; - size_t nhops, i; - struct zapi_nexthop zapi_nexthops[MULTIPATH_NUM]; + struct zapi_nhg api_nhg = {}; struct nexthop_group *nhg = NULL; - uint16_t proto; + struct nhg_backup_info *bnhg = NULL; struct nhg_hash_entry *nhe; s = msg; - - STREAM_GETW(s, proto); - STREAM_GETL(s, id); - STREAM_GETW(s, nhops); - - if (zserv_nexthop_num_warn(__func__, NULL, nhops)) - return; - - if (nhops <= 0) { - flog_warn(EC_ZEBRA_NEXTHOP_CREATION_FAILED, - "%s: No nexthops sent", __func__); + if (zapi_nhg_decode(s, hdr->command, &api_nhg) < 0) { + if (IS_ZEBRA_DEBUG_RECV) + zlog_debug("%s: Unable to decode zapi_nhg sent", + __func__); return; } - for (i = 0; i < nhops; i++) { - struct zapi_nexthop *znh = &zapi_nexthops[i]; + if ((!zapi_read_nexthops(client, NULL, api_nhg.nexthops, 0, 0, + api_nhg.nexthop_num, + api_nhg.backup_nexthop_num, &nhg, NULL)) + || (!zapi_read_nexthops(client, NULL, api_nhg.backup_nexthops, 0, 0, + api_nhg.backup_nexthop_num, + api_nhg.backup_nexthop_num, NULL, &bnhg))) { - if (zapi_nexthop_decode(s, znh, 0, 0) != 0) { - flog_warn(EC_ZEBRA_NEXTHOP_CREATION_FAILED, - "%s: Nexthop creation failed", __func__); - return; - } - } - - if (!zapi_read_nexthops(client, NULL, zapi_nexthops, 0, 0, nhops, 0, - &nhg, NULL)) { flog_warn(EC_ZEBRA_NEXTHOP_CREATION_FAILED, "%s: Nexthop Group Creation failed", __func__); + + nexthop_group_delete(&nhg); + zebra_nhg_backup_free(&bnhg); return; } /* * Create the nhg */ - nhe = zebra_nhg_proto_add(id, proto, nhg, 0); + nhe = zebra_nhg_proto_add(api_nhg.id, api_nhg.proto, nhg, 0); nexthop_group_delete(&nhg); + zebra_nhg_backup_free(&bnhg); /* * TODO: @@ -1818,18 +1868,11 @@ static void zread_nhg_add(ZAPI_HANDLER_ARGS) * Resolution is going to need some more work. */ if (nhe) - nhg_notify(proto, client->instance, id, ZAPI_NHG_INSTALLED); + nhg_notify(api_nhg.proto, client->instance, api_nhg.id, + ZAPI_NHG_INSTALLED); else - nhg_notify(proto, client->instance, id, ZAPI_NHG_FAIL_INSTALL); - - return; - -stream_failure: - flog_warn( - EC_ZEBRA_NEXTHOP_CREATION_FAILED, - "%s: Nexthop Group creation failed with some sort of stream read failure", - __func__); - return; + nhg_notify(api_nhg.proto, client->instance, api_nhg.id, + ZAPI_NHG_FAIL_INSTALL); } static void zread_route_add(ZAPI_HANDLER_ARGS) @@ -1911,6 +1954,9 @@ static void zread_route_add(ZAPI_HANDLER_ARGS) api.flags, api.message, api.backup_nexthop_num, api.backup_nexthop_num, NULL, &bnhg))) { + + nexthop_group_delete(&ng); + zebra_nhg_backup_free(&bnhg); XFREE(MTYPE_RE, re); return; } diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 1ed7ff00e5..6aa9ba0ebc 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -2759,6 +2759,14 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type, * Once resolution is figured out, we won't need this! */ for (ALL_NEXTHOPS_PTR(nhg, newhop)) { + if (CHECK_FLAG(newhop->flags, NEXTHOP_FLAG_HAS_BACKUP)) { + if (IS_ZEBRA_DEBUG_NHG) + zlog_debug( + "%s: id %u, backup nexthops not supported", + __func__, id); + return NULL; + } + if (newhop->type == NEXTHOP_TYPE_BLACKHOLE) { if (IS_ZEBRA_DEBUG_NHG) zlog_debug( From 612fcc5e8f33249929c3e9130885b9f2f6420532 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Mon, 28 Sep 2020 11:37:56 -0400 Subject: [PATCH 56/57] sharpd: make id log more specific Make ID log more specific as to the ID being assigned here. Signed-off-by: Stephen Worley --- sharpd/sharp_nht.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sharpd/sharp_nht.c b/sharpd/sharp_nht.c index 5b3fe2583e..7484dd3b06 100644 --- a/sharpd/sharp_nht.c +++ b/sharpd/sharp_nht.c @@ -86,7 +86,7 @@ static uint32_t nhg_id; static uint32_t sharp_get_next_nhid(void) { - zlog_debug("Id assigned: %u", nhg_id); + zlog_debug("NHG ID assigned: %u", nhg_id); return nhg_id++; } From 66c28560bac4035a92ea992f531b1d8e5250fab7 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Mon, 28 Sep 2020 12:39:22 -0400 Subject: [PATCH 57/57] zebra: set NHG/backup NHG pointers on success zapi read Only set the NHG/backup NHG pointers of the caller if the read of the nexthops was successfull. Otherwise, we might free when not neccessary or double free. Signed-off-by: Stephen Worley --- zebra/zapi_msg.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c index 1d8c5c7cc0..c33bca3d86 100644 --- a/zebra/zapi_msg.c +++ b/zebra/zapi_msg.c @@ -1622,14 +1622,14 @@ static bool zapi_read_nexthops(struct zserv *client, struct prefix *p, assert(!(png && pbnhg)); if (png) - *png = ng = nexthop_group_new(); + ng = nexthop_group_new(); if (pbnhg && backup_nh_num > 0) { if (IS_ZEBRA_DEBUG_RECV) zlog_debug("%s: adding %d backup nexthops", __func__, backup_nh_num); - *pbnhg = bnhg = zebra_nhg_backup_alloc(); + bnhg = zebra_nhg_backup_alloc(); } /* @@ -1704,7 +1704,7 @@ static bool zapi_read_nexthops(struct zserv *client, struct prefix *p, __func__, nhbuf, api_nh->vrf_id, labelbuf); } - if (png) { + if (ng) { /* Add new nexthop to temporary list. This list is * canonicalized - sorted - so that it can be hashed * later in route processing. We expect that the sender @@ -1729,6 +1729,14 @@ static bool zapi_read_nexthops(struct zserv *client, struct prefix *p, } } + + /* succesfully read, set caller pointers now */ + if (png) + *png = ng; + + if (pbnhg) + *pbnhg = bnhg; + return true; } @@ -1847,9 +1855,6 @@ static void zread_nhg_add(ZAPI_HANDLER_ARGS) flog_warn(EC_ZEBRA_NEXTHOP_CREATION_FAILED, "%s: Nexthop Group Creation failed", __func__); - - nexthop_group_delete(&nhg); - zebra_nhg_backup_free(&bnhg); return; }