Merge pull request #17599 from opensourcerouting/fix/reduce_default_c…

…onnect_timer bgpd: Connect retry timer backoff
FRRouting · Dec 18, 2024 · f78b178 · f78b178
2 parents 6c3e1e4 + 4ac77b1
commit f78b178
Show file tree

Hide file tree

Showing 4 changed files with 17 additions and 8 deletions.
diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c
@@ -491,11 +491,14 @@ static void bgp_connect_timer(struct event *thread)
 	assert(!connection->t_read);
 
 	if (bgp_debug_neighbor_events(peer))
-		zlog_debug("%s [FSM] Timer (connect timer expire)", peer->host);
+		zlog_debug("%s [FSM] Timer (connect timer (%us) expire)", peer->host,
+			   peer->v_connect);
 
 	if (CHECK_FLAG(peer->sflags, PEER_STATUS_ACCEPT_PEER))
 		bgp_stop(connection);
 	else {
+		if (!peer->connect)
+			peer->v_connect = MIN(BGP_MAX_CONNECT_RETRY, peer->v_connect * 2);
 		EVENT_VAL(thread) = ConnectRetry_timer_expired;
 		bgp_event(thread); /* bgp_event unlocks peer */
 	}
@@ -1224,9 +1227,14 @@ void bgp_fsm_change_status(struct peer_connection *connection,
 
 	peer_count = bgp->established_peers;
 
-	if (status == Established)
+	if (status == Established) {
 		bgp->established_peers++;
-	else if ((peer_established(connection)) && (status != Established))
+		/* Reset the retry timer if we already established */
+		if (peer->connect)
+			peer->v_connect = peer->connect;
+		else
+			peer->v_connect = peer->bgp->default_connect_retry;
+	} else if ((peer_established(connection)) && (status != Established))
 		bgp->established_peers--;
 
 	if (bgp_debug_neighbor_events(peer)) {

diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c
@@ -95,15 +95,15 @@ FRR_CFG_DEFAULT_BOOL(BGP_DETERMINISTIC_MED,
 );
 FRR_CFG_DEFAULT_ULONG(BGP_CONNECT_RETRY,
 	{ .val_ulong = 10, .match_profile = "datacenter", },
-	{ .val_ulong = 120 },
+	{ .val_ulong = BGP_DEFAULT_CONNECT_RETRY },
 );
 FRR_CFG_DEFAULT_ULONG(BGP_HOLDTIME,
 	{ .val_ulong = 9, .match_profile = "datacenter", },
-	{ .val_ulong = 180 },
+	{ .val_ulong = BGP_DEFAULT_KEEPALIVE },
 );
 FRR_CFG_DEFAULT_ULONG(BGP_KEEPALIVE,
 	{ .val_ulong = 3, .match_profile = "datacenter", },
-	{ .val_ulong = 60 },
+	{ .val_ulong = BGP_DEFAULT_KEEPALIVE },
 );
 FRR_CFG_DEFAULT_BOOL(BGP_EBGP_REQUIRES_POLICY,
 	{ .val_bool = false, .match_profile = "datacenter", },

diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h
@@ -2121,7 +2121,8 @@ struct bgp_nlri {
  */
 #define BGP_DEFAULT_HOLDTIME                   180
 #define BGP_DEFAULT_KEEPALIVE                   60
-#define BGP_DEFAULT_CONNECT_RETRY              120
+#define BGP_DEFAULT_CONNECT_RETRY               30
+#define BGP_MAX_CONNECT_RETRY                  120
 
 #define BGP_DEFAULT_EBGP_ROUTEADV                0
 #define BGP_DEFAULT_IBGP_ROUTEADV                0

diff --git a/tests/topotests/bgp_minimum_holdtime/test_bgp_minimum_holdtime.py b/tests/topotests/bgp_minimum_holdtime/test_bgp_minimum_holdtime.py
@@ -76,7 +76,7 @@ def _bgp_neighbor_check_if_notification_sent():
         return topotest.json_cmp(output, expected)
 
     test_func = functools.partial(_bgp_neighbor_check_if_notification_sent)
-    _, result = topotest.run_and_expect(test_func, None, count=40, wait=0.5)
+    _, result = topotest.run_and_expect(test_func, None, count=30, wait=1)
     assert result is None, "Failed to send notification message\n"