Merge pull request #4731 from mjstapp/fix_redist_update

zebra: redistribute deletes when updating selected route
FRRouting · Sep 18, 2019 · 16296be · 16296be
2 parents 312fbe4 + 40f321c
commit 16296be
Show file tree

Hide file tree

Showing 5 changed files with 100 additions and 35 deletions.
diff --git a/zebra/redistribute.c b/zebra/redistribute.c
@@ -149,7 +149,8 @@ static void zebra_redistribute(struct zserv *client, int type,
 /* Either advertise a route for redistribution to registered clients or */
 /* withdraw redistribution if add cannot be done for client */
 void redistribute_update(const struct prefix *p, const struct prefix *src_p,
-			 struct route_entry *re, struct route_entry *prev_re)
+			 const struct route_entry *re,
+			 const struct route_entry *prev_re)
 {
 	struct listnode *node, *nnode;
 	struct zserv *client;
@@ -226,54 +227,99 @@ void redistribute_update(const struct prefix *p, const struct prefix *src_p,
 	}
 }
 
+/*
+ * During a route delete, where 'new_re' is NULL, redist a delete to all
+ * clients registered for the type of 'old_re'.
+ * During a route update, redist a delete to any clients who will not see
+ * an update when the new route is installed. There are cases when a client
+ * may have seen a redist for 'old_re', but will not see
+ * the redist for 'new_re'.
+ */
 void redistribute_delete(const struct prefix *p, const struct prefix *src_p,
-			 struct route_entry *re)
+			 const struct route_entry *old_re,
+			 const struct route_entry *new_re)
 {
 	struct listnode *node, *nnode;
 	struct zserv *client;
-	char buf[INET6_ADDRSTRLEN];
 	int afi;
+	char buf[PREFIX_STRLEN];
+	vrf_id_t vrfid;
+
+	if (old_re)
+		vrfid = old_re->vrf_id;
+	else if (new_re)
+		vrfid = new_re->vrf_id;
+	else
+		return;
 
 	if (IS_ZEBRA_DEBUG_RIB) {
-		inet_ntop(p->family, &p->u.prefix, buf, INET6_ADDRSTRLEN);
-		zlog_debug("%u:%s/%d: Redist delete re %p (%s)",
-			   re->vrf_id, buf, p->prefixlen, re,
-			   zebra_route_string(re->type));
+		zlog_debug(
+			"%u:%s: Redist del: re %p (%s), new re %p (%s)",
+			vrfid, prefix2str(p, buf, sizeof(buf)),
+			old_re,
+			old_re ? zebra_route_string(old_re->type) : "None",
+			new_re,
+			new_re ? zebra_route_string(new_re->type) : "None");
 	}
 
 	/* Add DISTANCE_INFINITY check. */
-	if (re->distance == DISTANCE_INFINITY)
+	if (old_re && (old_re->distance == DISTANCE_INFINITY))
 		return;
 
 	afi = family2afi(p->family);
 	if (!afi) {
 		flog_warn(EC_ZEBRA_REDISTRIBUTE_UNKNOWN_AF,
 			  "%s: Unknown AFI/SAFI prefix received\n",
-			  __FUNCTION__);
+			  __func__);
 		return;
 	}
 
+	/* Skip invalid (e.g. linklocal) prefix */
 	if (!zebra_check_addr(p)) {
-		if (IS_ZEBRA_DEBUG_RIB)
-			zlog_debug("Redist delete filter prefix %s",
-				   prefix2str(p, buf, sizeof(buf)));
+		if (IS_ZEBRA_DEBUG_RIB) {
+			zlog_debug(
+				"%u:%s: Redist del old: skipping invalid prefix",
+				vrfid, prefix2str(p, buf, sizeof(buf)));
+		}
 		return;
 	}
 
 	for (ALL_LIST_ELEMENTS(zrouter.client_list, node, nnode, client)) {
-		if ((is_default_prefix(p)
-		     && vrf_bitmap_check(client->redist_default[afi],
-					 re->vrf_id))
-		    || vrf_bitmap_check(client->redist[afi][ZEBRA_ROUTE_ALL],
-					re->vrf_id)
-		    || (re->instance
-			&& redist_check_instance(
-				   &client->mi_redist[afi][re->type],
-				   re->instance))
-		    || vrf_bitmap_check(client->redist[afi][re->type],
-					re->vrf_id)) {
+		if (new_re) {
+			/* Skip this client if it will receive an update for the
+			 * 'new' re
+			 */
+			if (is_default_prefix(p)
+			    && vrf_bitmap_check(client->redist_default[afi],
+						new_re->vrf_id))
+				continue;
+			else if (vrf_bitmap_check(
+					 client->redist[afi][ZEBRA_ROUTE_ALL],
+					 new_re->vrf_id))
+				continue;
+			else if (new_re->instance
+				 && redist_check_instance(
+					 &client->mi_redist[afi][new_re->type],
+					 new_re->instance))
+				continue;
+			else if (vrf_bitmap_check(
+					 client->redist[afi][new_re->type],
+					 new_re->vrf_id))
+				continue;
+		}
+
+		/* Send a delete for the 'old' re to any subscribed client. */
+		if (old_re
+		    && ((old_re->instance
+			 && redist_check_instance(
+				 &client->mi_redist[afi]
+				 [old_re->type],
+				 old_re->instance))
+			|| vrf_bitmap_check(
+				client->redist[afi][old_re->type],
+				old_re->vrf_id))) {
 			zsend_redistribute_route(ZEBRA_REDISTRIBUTE_ROUTE_DEL,
-						 client, p, src_p, re);
+						 client, p, src_p, old_re);
 		}
 	}
 }

diff --git a/zebra/redistribute.h b/zebra/redistribute.h
@@ -42,11 +42,19 @@ extern void zebra_redistribute_default_delete(ZAPI_HANDLER_ARGS);
 
 extern void redistribute_update(const struct prefix *p,
 				const struct prefix *src_p,
-				struct route_entry *re,
-				struct route_entry *prev_re);
-extern void redistribute_delete(const struct prefix *p,
-				const struct prefix *src_p,
-				struct route_entry *re);
+				const struct route_entry *re,
+				const struct route_entry *prev_re);
+/*
+ * During a route delete, where 'new_re' is NULL, redist a delete to all
+ * clients registered for the type of 'old_re'.
+ * During a route update, redist a delete to any clients who will not see
+ * an update when the new route is installed. There are cases when a client
+ * may have seen a redist for 'old_re', but will not see
+ * the redist for 'new_re'.
+ */
+void redistribute_delete(const struct prefix *p, const struct prefix *src_p,
+			 const struct route_entry *old_re,
+			 const struct route_entry *new_re);
 
 extern void zebra_interface_up_update(struct interface *);
 extern void zebra_interface_down_update(struct interface *);

diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c
@@ -516,7 +516,8 @@ int zsend_interface_update(int cmd, struct zserv *client, struct interface *ifp)
 
 int zsend_redistribute_route(int cmd, struct zserv *client,
 			     const struct prefix *p,
-			     const struct prefix *src_p, struct route_entry *re)
+			     const struct prefix *src_p,
+			     const struct route_entry *re)
 {
 	struct zapi_route api;
 	struct zapi_nexthop *api_nh;

diff --git a/zebra/zapi_msg.h b/zebra/zapi_msg.h
@@ -65,7 +65,8 @@ extern int zsend_interface_update(int cmd, struct zserv *client,
 extern int zsend_redistribute_route(int cmd, struct zserv *zclient,
 				    const struct prefix *p,
 				    const struct prefix *src_p,
-				    struct route_entry *re);
+				    const struct route_entry *re);
+
 extern int zsend_router_id_update(struct zserv *zclient, struct prefix *p,
 				  vrf_id_t vrf_id);
 extern int zsend_interface_vrf_update(struct zserv *zclient,

diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c
@@ -691,7 +691,7 @@ static void rib_uninstall(struct route_node *rn, struct route_entry *re)
 
 		srcdest_rnode_prefixes(rn, &p, &src_p);
 
-		redistribute_delete(p, src_p, re);
+		redistribute_delete(p, src_p, re, NULL);
 		UNSET_FLAG(re->flags, ZEBRA_FLAG_SELECTED);
 	}
 }
@@ -1248,8 +1248,17 @@ static void rib_process(struct route_node *rn)
 			SET_FLAG(new_selected->flags, ZEBRA_FLAG_SELECTED);
 
 		if (old_selected) {
-			if (!new_selected)
-				redistribute_delete(p, src_p, old_selected);
+			/*
+			 * If we're removing the old entry, we should tell
+			 * redist subscribers about that *if* they aren't
+			 * going to see a redist for the new entry.
+			 */
+			if (!new_selected || CHECK_FLAG(old_selected->status,
+							ROUTE_ENTRY_REMOVED))
+				redistribute_delete(p, src_p,
+						    old_selected,
+						    new_selected);
+
 			if (old_selected != new_selected)
 				UNSET_FLAG(old_selected->flags,
 					   ZEBRA_FLAG_SELECTED);
@@ -2026,7 +2035,7 @@ static void rib_process_dplane_notify(struct zebra_dplane_ctx *ctx)
 		dplane_route_notif_update(rn, re, DPLANE_OP_ROUTE_DELETE, ctx);
 
 		/* Redistribute, lsp, and nht update */
-		redistribute_delete(dest_pfx, src_pfx, re);
+		redistribute_delete(dest_pfx, src_pfx, re, NULL);
 
 		zebra_rib_evaluate_rn_nexthops(
 			rn, zebra_router_get_next_sequence());