bgpd: prevent from sending useless ROUTE_ADD messages when NHG is used

When using BGP nexthop-groups with route, it is not needed to reinstall routes when only the nexthop needs to be updated for any reason (IGP, or same incoming BGP update received with nexthop modification). Fix this by introducing a flag at node level, to know if a route update is needed or not. This feature works if 'bgp suppress-fib-enabled' functionality is used. Without that, the BGP service does not know if a route has already been installed or not. Signed-off-by: Philippe Guibert <[email protected]>
FRRouting · Mar 5, 2024 · 2404364 · 2404364
1 parent b83c8bc
commit 2404364
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 7 deletions.
diff --git a/bgpd/bgp_table.h b/bgpd/bgp_table.h
@@ -91,6 +91,9 @@ struct bgp_dest {
 #define BGP_NODE_LABEL_REQUESTED        (1 << 7)
 #define BGP_NODE_SOFT_RECONFIG (1 << 8)
 #define BGP_NODE_PROCESS_CLEAR (1 << 9)
+#define BGP_NODE_FIB_UPDATE_UNNEEDED (1 << 10)
+/* this flag overrides previous one, when present */
+#define BGP_NODE_FIB_UPDATE_OVERRIDE (1 << 11)
 
 	struct bgp_addpath_node_data tx_addpath;
 

diff --git a/bgpd/bgp_zebra.c b/bgpd/bgp_zebra.c
@@ -1292,6 +1292,7 @@ void bgp_zebra_announce_parse_nexthop(struct bgp_path_info *info,
 	bool creation = true;
 	struct bgp_path_info *p_mpinfo[MULTIPATH_NUM] = { 0 };
 	char nexthop_buf[1024];
+	bool route_need_update;
 
 	/* Determine if we're doing weighted ECMP or not */
 	do_wt_ecmp = bgp_path_info_mpath_chkwtd(bgp, info);
@@ -1594,6 +1595,7 @@ void bgp_zebra_announce_parse_nexthop(struct bgp_path_info *info,
 				    sizeof(nexthop_buf));
 
 	/* unlink parent nhg from p_mpinfo[i], except for info */
+	route_need_update = false;
 	for (i = 0; i < *valid_nh_count; i++) {
 		if (p_mpinfo[i]->bgp_nhg != p_nhg_parent) {
 			if (p_mpinfo[i]->bgp_nhg) {
@@ -1615,10 +1617,18 @@ void bgp_zebra_announce_parse_nexthop(struct bgp_path_info *info,
 					 nhg_cache_thread);
 			p_mpinfo[i]->bgp_nhg = p_nhg_parent;
 			p_nhg_parent->path_count++;
-		} else
+			route_need_update = true;
+		} else if (BGP_DEBUG(nexthop_group, NEXTHOP_GROUP))
 			zlog_debug("parse_nexthop: %pFX, NHG %u already attached (%s)",
 				   p, p_nhg_parent->id, nexthop_buf);
 	}
+
+	if (!CHECK_FLAG(p_nhg_parent->state, BGP_NHG_STATE_INSTALLED))
+		SET_FLAG(info->net->flags, BGP_NODE_FIB_UPDATE_OVERRIDE);
+	else if (route_need_update == false
+		 && !CHECK_FLAG(info->net->flags, BGP_NODE_FIB_UPDATE_OVERRIDE))
+		SET_FLAG(info->net->flags, BGP_NODE_FIB_UPDATE_UNNEEDED);
+
 	p_nhg_parent->last_update = monotime(NULL);
 
 	*nhg_id = p_nhg_parent->id;
@@ -1835,14 +1845,28 @@ void bgp_zebra_announce(struct bgp_dest *dest, const struct prefix *p,
 		api.distance = distance;
 	}
 
-	if (nhg_id && info->bgp_nhg &&
-	    !CHECK_FLAG(info->bgp_nhg->state, BGP_NHG_STATE_INSTALLED)) {
-		if (BGP_DEBUG(nexthop_group, NEXTHOP_GROUP))
-			zlog_debug("NHG %u, BGP %s: ID not installed, postpone prefix %pFX install",
-				   info->bgp_nhg->id, bgp->name_pretty, p);
-		return;
+	if (nhg_id && info->bgp_nhg) {
+		if (!CHECK_FLAG(info->bgp_nhg->state, BGP_NHG_STATE_INSTALLED)) {
+			if (BGP_DEBUG(nexthop_group, NEXTHOP_GROUP))
+				zlog_debug("NHG %u, BGP %s: ID not installed, postpone prefix %pFX install",
+					   info->bgp_nhg->id, bgp->name_pretty,
+					   p);
+			return;
+		}
+		if (CHECK_FLAG(dest->flags, BGP_NODE_FIB_INSTALLED) &&
+		    !CHECK_FLAG(dest->flags, BGP_NODE_FIB_UPDATE_OVERRIDE) &&
+		    CHECK_FLAG(dest->flags, BGP_NODE_FIB_UPDATE_UNNEEDED)) {
+			if (BGP_DEBUG(nexthop_group, NEXTHOP_GROUP))
+				zlog_debug("NHG %u, BGP %s: ID and route already installed, no re-install %pFX",
+					   info->bgp_nhg->id, bgp->name_pretty,
+					   p);
+			UNSET_FLAG(dest->flags, BGP_NODE_FIB_UPDATE_UNNEEDED);
+			return;
+		}
 	}
 
+	UNSET_FLAG(dest->flags, BGP_NODE_FIB_UPDATE_OVERRIDE);
+
 	if (bgp_debug_zebra(p)) {
 		zlog_debug(
 			"Tx route %s VRF %u %pFX metric %u tag %" ROUTE_TAG_PRI