Merge pull request #17734 from opensourcerouting/fix/show_discarded_withdrawn_routes

bgpd: Show prefix-related stats per neighbor

7 files changed, 80 insertions, 8 deletions

diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
index 2280aa9097..d349922c52 100644
--- a/bgpd/bgp_attr.c
+++ b/bgpd/bgp_attr.c
@@ -5406,7 +5406,14 @@ enum bgp_attr_parse_ret bgp_attr_ignore(struct peer *peer, uint8_t type)
 			   lookup_msg(attr_str, type, NULL),
 			   withdraw ? "treat-as-withdraw" : "discard");
 
-	return withdraw ? BGP_ATTR_PARSE_WITHDRAW : BGP_ATTR_PARSE_PROCEED;
+	/* We don't increment stat_pfx_withdraw here, because it's done in
+	 * bgp_update_receive().
+	 */
+	if (withdraw)
+		return BGP_ATTR_PARSE_WITHDRAW;
+
+	peer->stat_pfx_discard++;
+	return BGP_ATTR_PARSE_PROCEED;
 }
 
 bool route_matches_soo(struct bgp_path_info *pi, struct ecommunity *soo)
diff --git a/bgpd/bgp_bmp.c b/bgpd/bgp_bmp.c
index 5264bea9af..acc49cac94 100644
--- a/bgpd/bgp_bmp.c
+++ b/bgpd/bgp_bmp.c
@@ -1773,8 +1773,7 @@ static void bmp_stats(struct event *thread)
 				peer->stat_pfx_cluster_loop);
 		bmp_stat_put_u32(s, &count, BMP_STATS_PFX_DUP_WITHDRAW,
 				peer->stat_pfx_dup_withdraw);
-		bmp_stat_put_u32(s, &count, BMP_STATS_UPD_7606_WITHDRAW,
-				peer->stat_upd_7606);
+		bmp_stat_put_u32(s, &count, BMP_STATS_UPD_7606_WITHDRAW, peer->stat_pfx_withdraw);
 		if (bt->stats_send_experimental)
 			bmp_stat_put_u32(s, &count, BMP_STATS_FRR_NH_INVALID,
 					 peer->stat_pfx_nh_invalid);
diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c
index e9cc52449b..c5e390b045 100644
--- a/bgpd/bgp_packet.c
+++ b/bgpd/bgp_packet.c
@@ -2411,7 +2411,7 @@ static int bgp_update_receive(struct peer_connection *connection,
 				    sizeof(peer->rcvd_attr_str));
 
 		if (attr_parse_ret == BGP_ATTR_PARSE_WITHDRAW) {
-			peer->stat_upd_7606++;
+			peer->stat_pfx_withdraw++;
 			flog_err(
 				EC_BGP_UPDATE_RCV,
 				"%pBP rcvd UPDATE with errors in attr(s)!! Withdrawing route.",
diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c
index e336b54eb4..550adf93db 100644
--- a/bgpd/bgp_vty.c
+++ b/bgpd/bgp_vty.c
@@ -15464,9 +15464,12 @@ CPP_NOTICE("Remove `gracefulRestartCapability` JSON field")
 
 	if (use_json) {
 		json_object *json_stat = NULL;
+		json_object *json_pfx_stat = NULL;
+
 		json_stat = json_object_new_object();
-		/* Packet counts. */
+		json_pfx_stat = json_object_new_object();
 
+		/* Packet counts. */
 		atomic_size_t outq_count, inq_count;
 		outq_count = atomic_load_explicit(&p->connection->obuf->count,
 						  memory_order_relaxed);
@@ -15516,6 +15519,16 @@ CPP_NOTICE("Remove `gracefulRestartCapability` JSON field")
 		json_object_int_add(json_stat, "totalSent", PEER_TOTAL_TX(p));
 		json_object_int_add(json_stat, "totalRecv", PEER_TOTAL_RX(p));
 		json_object_object_add(json_neigh, "messageStats", json_stat);
+
+		/* Prefix statistics */
+		json_object_int_add(json_pfx_stat, "inboundFiltered", p->stat_pfx_filter);
+		json_object_int_add(json_pfx_stat, "aspathLoop", p->stat_pfx_aspath_loop);
+		json_object_int_add(json_pfx_stat, "originatorLoop", p->stat_pfx_originator_loop);
+		json_object_int_add(json_pfx_stat, "clusterLoop", p->stat_pfx_cluster_loop);
+		json_object_int_add(json_pfx_stat, "invalidNextHop", p->stat_pfx_nh_invalid);
+		json_object_int_add(json_pfx_stat, "withdrawn", p->stat_pfx_withdraw);
+		json_object_int_add(json_pfx_stat, "attributesDiscarded", p->stat_pfx_discard);
+		json_object_object_add(json_neigh, "prefixStats", json_pfx_stat);
 	} else {
 		atomic_size_t outq_count, inq_count, open_out, open_in,
 			notify_out, notify_in, update_out, update_in,
@@ -15567,8 +15580,18 @@ CPP_NOTICE("Remove `gracefulRestartCapability` JSON field")
 			refresh_in);
 		vty_out(vty, "    Capability:    %10zu %10zu\n",
 			dynamic_cap_out, dynamic_cap_in);
-		vty_out(vty, "    Total:         %10u %10u\n",
-			(uint32_t)PEER_TOTAL_TX(p), (uint32_t)PEER_TOTAL_RX(p));
+		vty_out(vty, "    Total:         %10u %10u\n\n", (uint32_t)PEER_TOTAL_TX(p),
+			(uint32_t)PEER_TOTAL_RX(p));
+
+		/* Prefix statistics */
+		vty_out(vty, "  Prefix statistics:\n");
+		vty_out(vty, "    Inbound filtered: %u\n", p->stat_pfx_filter);
+		vty_out(vty, "    AS-PATH loop: %u\n", p->stat_pfx_aspath_loop);
+		vty_out(vty, "    Originator loop: %u\n", p->stat_pfx_originator_loop);
+		vty_out(vty, "    Cluster loop: %u\n", p->stat_pfx_cluster_loop);
+		vty_out(vty, "    Invalid next-hop: %u\n", p->stat_pfx_nh_invalid);
+		vty_out(vty, "    Withdrawn: %u\n", p->stat_pfx_withdraw);
+		vty_out(vty, "    Attributes discarded: %u\n\n", p->stat_pfx_discard);
 	}
 
 	if (use_json) {
diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
index 2d7396ad6c..47214e52e5 100644
--- a/bgpd/bgpd.h
+++ b/bgpd/bgpd.h
@@ -1728,7 +1728,8 @@ struct peer {
 	uint32_t stat_pfx_cluster_loop;
 	uint32_t stat_pfx_nh_invalid;
 	uint32_t stat_pfx_dup_withdraw;
-	uint32_t stat_upd_7606;  /* RFC7606: treat-as-withdraw */
+	uint32_t stat_pfx_withdraw; /* RFC7606: treat-as-withdraw */
+	uint32_t stat_pfx_discard;  /* The number of prefixes with discarded attributes */
 	uint64_t stat_pfx_loc_rib; /* RFC7854 : Number of routes in Loc-RIB */
 	uint64_t stat_pfx_adj_rib_in; /* RFC7854 : Number of routes in Adj-RIBs-In */
 
diff --git a/tests/topotests/bgp_path_attribute_discard/test_bgp_path_attribute_discard.py b/tests/topotests/bgp_path_attribute_discard/test_bgp_path_attribute_discard.py
index adc92f59fe..c6f1b6193b 100644
--- a/tests/topotests/bgp_path_attribute_discard/test_bgp_path_attribute_discard.py
+++ b/tests/topotests/bgp_path_attribute_discard/test_bgp_path_attribute_discard.py
@@ -142,6 +142,27 @@ def test_bgp_path_attribute_discard():
         result is None
     ), "Failed to discard path attributes (atomic-aggregate, community)"
 
+    def _bgp_check_attributes_discarded_stats():
+        output = json.loads(r1.vtysh_cmd("show bgp neighbor json"))
+        expected = {
+            "10.0.0.254": {
+                "prefixStats": {
+                    "inboundFiltered": 0,
+                    "aspathLoop": 0,
+                    "originatorLoop": 0,
+                    "clusterLoop": 0,
+                    "invalidNextHop": 0,
+                    "withdrawn": 0,
+                    "attributesDiscarded": 3,
+                }
+            }
+        }
+        return topotest.json_cmp(output, expected)
+
+    test_func = functools.partial(_bgp_check_attributes_discarded_stats)
+    _, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5)
+    assert result is None, "Discarded path attributes count is not as expected"
+
     def _bgp_check_if_aigp_invalid_attribute_discarded():
         output = json.loads(r2.vtysh_cmd("show bgp ipv4 unicast json detail"))
         expected = {
diff --git a/tests/topotests/bgp_path_attribute_treat_as_withdraw/test_bgp_path_attribute_treat_as_withdraw.py b/tests/topotests/bgp_path_attribute_treat_as_withdraw/test_bgp_path_attribute_treat_as_withdraw.py
index a9d678a42d..4f6472f3c5 100644
--- a/tests/topotests/bgp_path_attribute_treat_as_withdraw/test_bgp_path_attribute_treat_as_withdraw.py
+++ b/tests/topotests/bgp_path_attribute_treat_as_withdraw/test_bgp_path_attribute_treat_as_withdraw.py
@@ -134,6 +134,27 @@ def test_bgp_path_attribute_treat_as_withdraw():
     _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
     assert result is None, "Failed to withdraw prefixes with atomic-aggregate attribute"
 
+    def _bgp_check_attributes_withdrawn_stats():
+        output = json.loads(r2.vtysh_cmd("show bgp neighbor json"))
+        expected = {
+            "10.0.0.1": {
+                "prefixStats": {
+                    "inboundFiltered": 0,
+                    "aspathLoop": 0,
+                    "originatorLoop": 0,
+                    "clusterLoop": 0,
+                    "invalidNextHop": 0,
+                    "withdrawn": 1,
+                    "attributesDiscarded": 0,
+                }
+            }
+        }
+        return topotest.json_cmp(output, expected)
+
+    test_func = functools.partial(_bgp_check_attributes_withdrawn_stats)
+    _, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5)
+    assert result is None, "Withdrawn prefix count is not as expected"
+
 
 def test_memory_leak():
     "Run the memory leak test and report results."