bgpd: Re-use TX Addpath IDs where possible

The motivation for this patch is to address a concerning behavior of
tx-addpath-bestpath-per-AS. Prior to this patch, all paths' TX ID was
pre-determined as the path was received from a peer. However, this meant
that any time the path selected as best from an AS changed, bgpd had no
choice but to withdraw the previous best path, and advertise the new
best-path under a new TX ID. This could cause significant network
disruption, especially for the subset of prefixes coming from only one
AS that were also communicated over a bestpath-per-AS session.

The patch's general approach is best illustrated by
txaddpath_update_ids. After a bestpath run (required for best-per-AS to
know what will and will not be sent as addpaths) ID numbers will be
stripped from paths that no longer need to be sent, and held in a pool.
Then, paths that will be sent as addpaths and do not already have ID
numbers will allocate new ID numbers, pulling first from that pool.
Finally, anything left in the pool will be returned to the allocator.

In order for this to work, ID numbers had to be split by strategy. The
tx-addpath-All strategy would keep every ID number "in use" constantly,
preventing IDs from being transferred to different paths. Rather than
create two variables for ID, this patch create a more generic array that
will easily enable more addpath strategies to be implemented. The
previously described ID manipulations will happen per addpath strategy,
and will only be run for strategies that are enabled on at least one
peer.

Finally, the ID numbers are allocated from an allocator that tracks per
AFI/SAFI/Addpath Strategy which IDs are in use. Though it would be very
improbable, there was the possibility with the free-running counter
approach for rollover to cause two paths on the same prefix to get
assigned the same TX ID. As remote as the possibility is, we prefer to
not leave it to chance.

This ID re-use method is not perfect. In some cases you could still get
withdraw-then-add behaviors where not strictly necessary. In the case of
bestpath-per-AS this requires one AS to advertise a prefix for the first
time, then a second AS withdraws that prefix, all within the space of an
already pending MRAI timer. In those situations a withdraw-then-add is
more forgivable, and fixing it would probably require a much more
significant effort, as IDs would need to be moved to ADVs instead of
paths.

Signed-off-by Mitchell Skiba <mskiba@amazon.com>

bgpd/bgp_addpath.h

@@ -0,0 +1,72 @@
+/*
+ * Addpath TX ID selection, and related utilities
+ * Copyright (C) 2018  Amazon.com, Inc. or its affiliates
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; see the file COPYING; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#ifndef _QUAGGA_BGPD_TX_ADDPATH_H
+#define _QUAGGA_BGPD_TX_ADDPATH_H
+
+#include <stdint.h>
+#include <zebra.h>
+
+#include "bgpd/bgp_addpath_types.h"
+#include "bgpd/bgp_route.h"
+#include "bgpd/bgp_table.h"
+#include "lib/json.h"
+
+#define BGP_ADDPATH_TX_ID_FOR_DEFAULT_ORIGINATE 1
+
+void bgp_addpath_init_bgp_data(struct bgp_addpath_bgp_data *d);
+
+int bgp_addpath_is_addpath_used(struct bgp_addpath_bgp_data *d, afi_t afi,
+			      safi_t safi);
+
+void bgp_addpath_free_node_data(struct bgp_addpath_bgp_data *bd,
+			      struct bgp_addpath_node_data *nd,
+			      afi_t afi, safi_t safi);
+
+void bgp_addpath_free_info_data(struct bgp_addpath_info_data *d,
+			      struct bgp_addpath_node_data *nd);
+
+
+int bgp_addpath_info_has_ids(struct bgp_addpath_info_data *d);
+
+uint32_t bgp_addpath_id_for_peer(struct peer *peer, afi_t afi, safi_t safi,
+				struct bgp_addpath_info_data *d);
+
+struct bgp_addpath_strategy_names *
+bgp_addpath_names(enum bgp_addpath_strat strat);
+
+int bgp_addpath_dmed_required(int strategy);
+
+/*
+ * Return true if this is a path we should advertise due to a configured
+ * addpath-tx knob
+ */
+int bgp_addpath_tx_path(enum bgp_addpath_strat strat,
+			    struct bgp_path_info *pi);
+/*
+ * Change the type of addpath used for a peer.
+ */
+void bgp_addpath_set_peer_type(struct peer *peer, afi_t afi, safi_t safi,
+			      enum bgp_addpath_strat addpath_type);
+
+void bgp_addpath_update_ids(struct bgp *bgp, struct bgp_node *bn, afi_t afi,
+			    safi_t safi);
+
+void bgp_addpath_type_changed(struct bgp *bgp);
+#endif

bgpd/bgp_addpath_types.h

@@ -0,0 +1,55 @@
+/*
+ * Addpath TX ID selection, and related utilities
+ * Copyright (C) 2018  Amazon.com, Inc. or its affiliates
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; see the file COPYING; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#ifndef _QUAGGA_BGPD_TX_ADDPATH_DATA_H
+#define _QUAGGA_BGPD_TX_ADDPATH_DATA_H
+#include "lib/id_alloc.h"
+#include <stdint.h>
+
+enum bgp_addpath_strat {
+	BGP_ADDPATH_ALL = 0,
+	BGP_ADDPATH_BEST_PER_AS,
+	BGP_ADDPATH_MAX,
+	BGP_ADDPATH_NONE,
+};
+
+/* TX Addpath structures */
+struct bgp_addpath_bgp_data {
+	unsigned int peercount[AFI_MAX][SAFI_MAX][BGP_ADDPATH_MAX];
+	unsigned int total_peercount[AFI_MAX][SAFI_MAX];
+	struct id_alloc *id_allocators[AFI_MAX][SAFI_MAX][BGP_ADDPATH_MAX];
+};
+
+struct bgp_addpath_node_data {
+	struct id_alloc_pool *free_ids[BGP_ADDPATH_MAX];
+};
+
+struct bgp_addpath_info_data {
+	uint32_t addpath_tx_id[BGP_ADDPATH_MAX];
+};
+
+struct bgp_addpath_strategy_names {
+	const char *config_name;
+	const char *human_name;	       /* path detail non-json */
+	const char *human_description; /* non-json peer descriptions */
+	const char *type_json_name;    /* json peer listings */
+	const char *id_json_name;      /* path json output for tx ID# */
+};
+
+#endif

bgpd/bgp_evpn.c

@@ -47,6 +47,7 @@
 #include "bgpd/bgp_aspath.h"
 #include "bgpd/bgp_zebra.h"
 #include "bgpd/bgp_nexthop.h"
+#include "bgpd/bgp_addpath.h"
 
 /*
  * Definitions and external declarations.
@@ -1059,7 +1060,7 @@ static int evpn_es_route_select_install(struct bgp *bgp,
 	    && old_select->sub_type == BGP_ROUTE_IMPORTED
 	    && !CHECK_FLAG(rn->flags, BGP_NODE_USER_CLEAR)
 	    && !CHECK_FLAG(old_select->flags, BGP_PATH_ATTR_CHANGED)
-	    && !bgp->addpath_tx_used[afi][safi]) {
+	    && !bgp_addpath_is_addpath_used(&bgp->tx_addpath, afi, safi)) {
 		if (bgp_zebra_has_route_changed(rn, old_select)) {
 			ret = evpn_es_install_vtep(bgp, es,
 						   (struct prefix_evpn *)&rn->p,
@@ -1142,7 +1143,7 @@ static int evpn_route_select_install(struct bgp *bgp, struct bgpevpn *vpn,
 	    && old_select->sub_type == BGP_ROUTE_IMPORTED
 	    && !CHECK_FLAG(rn->flags, BGP_NODE_USER_CLEAR)
 	    && !CHECK_FLAG(old_select->flags, BGP_PATH_ATTR_CHANGED)
-	    && !bgp->addpath_tx_used[afi][safi]) {
+	    && !bgp_addpath_is_addpath_used(&bgp->tx_addpath, afi, safi)) {
 		if (bgp_zebra_has_route_changed(rn, old_select)) {
 			if (old_select->attr->sticky)
 				SET_FLAG(flags, ZEBRA_MACIP_TYPE_STICKY);

bgpd/bgp_open.c

@@ -1387,10 +1387,7 @@ void bgp_open_capability(struct stream *s, struct peer *peer)
 			/* Only advertise addpath TX if a feature that
 			 * will use it is
 			 * configured */
-			if (CHECK_FLAG(peer->af_flags[afi][safi],
-				       PEER_FLAG_ADDPATH_TX_ALL_PATHS)
-			    || CHECK_FLAG(peer->af_flags[afi][safi],
-					  PEER_FLAG_ADDPATH_TX_BESTPATH_PER_AS))
+			if (peer->addpath_type[afi][safi] != BGP_ADDPATH_NONE)
 				adv_addpath_tx = 1;
 		}
 	}

bgpd/bgp_route.c

@@ -65,6 +65,7 @@
 #include "bgpd/bgp_nht.h"
 #include "bgpd/bgp_updgrp.h"
 #include "bgpd/bgp_label.h"
+#include "bgpd/bgp_addpath.h"
 
 #if ENABLE_BGP_VNC
 #include "bgpd/rfapi/rfapi_backend.h"
@@ -247,6 +248,8 @@ static void bgp_path_info_free(struct bgp_path_info *path)
 	bgp_unlink_nexthop(path);
 	bgp_path_info_extra_free(&path->extra);
 	bgp_path_info_mpath_free(&path->mpath);
+	bgp_addpath_free_info_data(&path->tx_addpath,
+				 path->net ? &path->net->tx_addpath : NULL);
 
 	peer_unlock(path->peer); /* bgp_path_info peer reference */
 
@@ -1472,7 +1475,7 @@ int subgroup_announce_check(struct bgp_node *rn, struct bgp_path_info *pi,
 	 * addpath
 	 * feature that requires us to advertise it */
 	if (!CHECK_FLAG(pi->flags, BGP_PATH_SELECTED)) {
-		if (!bgp_addpath_tx_path(peer, afi, safi, pi)) {
+		if (!bgp_addpath_tx_path(peer->addpath_type[afi][safi], pi)) {
 			return 0;
 		}
 	}
@@ -2078,6 +2081,8 @@ void bgp_best_selection(struct bgp *bgp, struct bgp_node *rn,
 	bgp_path_info_mpath_aggregate_update(new_select, old_select);
 	bgp_mp_list_clear(&mp_list);
 
+	bgp_addpath_update_ids(bgp, rn, afi, safi);
+
 	result->old = old_select;
 	result->new = new_select;
 
@@ -2127,7 +2132,7 @@ int subgroup_process_announce_selected(struct update_subgroup *subgrp,
 			bgp_adj_out_set_subgroup(rn, subgrp, &attr, selected);
 		else
 			bgp_adj_out_unset_subgroup(rn, subgrp, 1,
-						   selected->addpath_tx_id);
+						   addpath_tx_id);
 	}
 
 	/* If selected is NULL we must withdraw the path using addpath_tx_id */
@@ -2303,7 +2308,7 @@ static void bgp_process_main_one(struct bgp *bgp, struct bgp_node *rn,
 	if (old_select && old_select == new_select
 	    && !CHECK_FLAG(rn->flags, BGP_NODE_USER_CLEAR)
 	    && !CHECK_FLAG(old_select->flags, BGP_PATH_ATTR_CHANGED)
-	    && !bgp->addpath_tx_used[afi][safi]) {
+	    && !bgp_addpath_is_addpath_used(&bgp->tx_addpath, afi, safi)) {
 		if (bgp_zebra_has_route_changed(rn, old_select)) {
 #if ENABLE_BGP_VNC
 			vnc_import_bgp_add_route(bgp, p, old_select);
@@ -2776,7 +2781,6 @@ struct bgp_path_info *info_make(int type, int sub_type, unsigned short instance,
 	new->attr = attr;
 	new->uptime = bgp_clock();
 	new->net = rn;
-	new->addpath_tx_id = ++peer->bgp->addpath_tx_id;
 	return new;
 }
 
@@ -7486,6 +7490,18 @@ static void route_vty_out_advertised_to(struct vty *vty, struct peer *peer,
 	}
 }
 
+static void route_vty_out_tx_ids(struct vty *vty,
+				 struct bgp_addpath_info_data *d)
+{
+	int i;
+
+	for (i = 0; i < BGP_ADDPATH_MAX; i++) {
+		vty_out(vty, "TX-%s %u%s", bgp_addpath_names(i)->human_name,
+			d->addpath_tx_id[i],
+			i < BGP_ADDPATH_MAX - 1 ? " " : "\n");
+	}
+}
+
 void route_vty_out_detail(struct vty *vty, struct bgp *bgp, struct prefix *p,
 			  struct bgp_path_info *path, afi_t afi, safi_t safi,
 			  json_object *json_paths)
@@ -7517,6 +7533,7 @@ void route_vty_out_detail(struct vty *vty, struct bgp *bgp, struct prefix *p,
 	unsigned int first_as;
 	bool nexthop_self =
 		CHECK_FLAG(path->flags, BGP_PATH_ANNC_NH_SELF) ? true : false;
+	int i;
 
 	if (json_paths) {
 		json_path = json_object_new_object();
@@ -8228,29 +8245,53 @@ void route_vty_out_detail(struct vty *vty, struct bgp *bgp, struct prefix *p,
 		}
 
 		/* Line 8 display Addpath IDs */
-		if (path->addpath_rx_id || path->addpath_tx_id) {
+		if (path->addpath_rx_id
+		    || bgp_addpath_info_has_ids(&path->tx_addpath)) {
 			if (json_paths) {
 				json_object_int_add(json_path, "addpathRxId",
 						    path->addpath_rx_id);
-				json_object_int_add(json_path, "addpathTxId",
-						    path->addpath_tx_id);
+
+				/* Keep backwards compatibility with the old API
+				 * by putting TX All's ID in the old field
+				 */
+				json_object_int_add(
+					json_path, "addpathTxId",
+					path->tx_addpath.addpath_tx_id
+						[BGP_ADDPATH_ALL]);
+
+				/* ... but create a specific field for each
+				 * strategy
+				 */
+				for (i = 0; i < BGP_ADDPATH_MAX; i++) {
+					json_object_int_add(
+						json_path,
+						bgp_addpath_names(i)
+							->id_json_name,
+						path->tx_addpath
+							.addpath_tx_id[i]);
+				}
 			} else {
-				vty_out(vty, "      AddPath ID: RX %u, TX %u\n",
-					path->addpath_rx_id,
-					path->addpath_tx_id);
+				vty_out(vty, "      AddPath ID: RX %u, ",
+					path->addpath_rx_id);
+
+				route_vty_out_tx_ids(vty, &path->tx_addpath);
 			}
 		}
 
 		/* If we used addpath to TX a non-bestpath we need to display
-		 * "Advertised to" on a path-by-path basis */
-		if (bgp->addpath_tx_used[afi][safi]) {
+		 * "Advertised to" on a path-by-path basis
+		 */
+		if (bgp_addpath_is_addpath_used(&bgp->tx_addpath, afi, safi)) {
 			first = 1;
 
 			for (ALL_LIST_ELEMENTS(bgp->peer, node, nnode, peer)) {
 				addpath_capable =
 					bgp_addpath_encode_tx(peer, afi, safi);
 				has_adj = bgp_adj_out_lookup(
-					peer, path->net, path->addpath_tx_id);
+					peer, path->net,
+					bgp_addpath_id_for_peer(
+						peer, afi, safi,
+						&path->tx_addpath));
 
 				if ((addpath_capable && has_adj)
 				    || (!addpath_capable && has_adj
@@ -8958,7 +8999,7 @@ void route_vty_out_detail_header(struct vty *vty, struct bgp *bgp,
 	 * show what peers we advertised the bestpath to.  If we are using
 	 * addpath
 	 * though then we must display Advertised to on a path-by-path basis. */
-	if (!bgp->addpath_tx_used[afi][safi]) {
+	if (!bgp_addpath_is_addpath_used(&bgp->tx_addpath, afi, safi)) {
 		for (ALL_LIST_ELEMENTS(bgp->peer, node, nnode, peer)) {
 			if (bgp_adj_out_lookup(peer, rn, 0)) {
 				if (json && !json_adv_to)

bgpd/bgp_route.h

@@ -24,6 +24,7 @@
 #include "queue.h"
 #include "nexthop.h"
 #include "bgp_table.h"
+#include "bgp_addpath_types.h"
 
 struct bgp_nexthop_cache;
 struct bgp_route_evpn;
@@ -220,7 +221,7 @@ struct bgp_path_info {
 
 	/* Addpath identifiers */
 	uint32_t addpath_rx_id;
-	uint32_t addpath_tx_id;
+	struct bgp_addpath_info_data tx_addpath;
 };
 
 /* Structure used in BGP path selection */

bgpd/bgp_table.c

@@ -29,6 +29,7 @@
 
 #include "bgpd/bgpd.h"
 #include "bgpd/bgp_table.h"
+#include "bgp_addpath.h"
 
 void bgp_table_lock(struct bgp_table *rt)
 {
@@ -76,7 +77,16 @@ static void bgp_node_destroy(route_table_delegate_t *delegate,
 			     struct route_table *table, struct route_node *node)
 {
 	struct bgp_node *bgp_node;
+	struct bgp_table *rt;
 	bgp_node = bgp_node_from_rnode(node);
+	rt = table->info;
+
+	if (rt->bgp) {
+		bgp_addpath_free_node_data(&rt->bgp->tx_addpath,
+					 &bgp_node->tx_addpath,
+					 rt->afi, rt->safi);
+	}
+
 	XFREE(MTYPE_BGP_NODE, bgp_node);
 }
 

bgpd/bgp_table.h

@@ -25,6 +25,7 @@
 #include "table.h"
 #include "queue.h"
 #include "linklist.h"
+#include "bgpd.h"
 
 struct bgp_table {
 	/* table belongs to this instance */
@@ -67,6 +68,8 @@ struct bgp_node {
 #define BGP_NODE_USER_CLEAR             (1 << 1)
 #define BGP_NODE_LABEL_CHANGED          (1 << 2)
 #define BGP_NODE_REGISTERED_FOR_LABEL   (1 << 3)
+
+	struct bgp_addpath_node_data tx_addpath;
 };
 
 /*