From ef4a9215b912c885498715614ee01b43dc861c1a Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 5 Dec 2024 09:51:05 +0200 Subject: [PATCH 1/4] bgpd: Reuse defined constants for BGP timers Signed-off-by: Donatas Abraitis --- bgpd/bgp_vty.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index bb0c69ca56ee..e76631130a9f 100644 --- 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", }, From 44f4a8ec4049c54e8d25465a735174bd0f15f81e Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 5 Dec 2024 10:18:02 +0200 Subject: [PATCH 2/4] bgpd: Reduce the default connect retry timer to 30 seconds RFC 4271 recommends this 120 seconds, but most of the implementations use 60. For a datacenter profile we set it to 10 seconds. Let's roll with 30 and increase up to the maximum 120. Signed-off-by: Donatas Abraitis --- bgpd/bgpd.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index bb56fd355a05..fafd20711042 100644 --- a/bgpd/bgpd.h +++ b/bgpd/bgpd.h @@ -2105,7 +2105,7 @@ 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_DEFAULT_EBGP_ROUTEADV 0 #define BGP_DEFAULT_IBGP_ROUTEADV 0 From ab3535fbcf37b59ec02332fa021142c5b7d6dd3e Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 5 Dec 2024 10:49:52 +0200 Subject: [PATCH 3/4] bgpd: Implement connect retry backoff Instead of starting with a fairly high value of retry, let's try with a lower and increase with a backoff to reach what was a default value (120s). Signed-off-by: Donatas Abraitis --- bgpd/bgp_fsm.c | 14 +++++++++++--- bgpd/bgpd.h | 1 + 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c index cadef3997423..d135ba809993 100644 --- 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/bgpd.h b/bgpd/bgpd.h index fafd20711042..69048f1f6526 100644 --- a/bgpd/bgpd.h +++ b/bgpd/bgpd.h @@ -2106,6 +2106,7 @@ struct bgp_nlri { #define BGP_DEFAULT_HOLDTIME 180 #define BGP_DEFAULT_KEEPALIVE 60 #define BGP_DEFAULT_CONNECT_RETRY 30 +#define BGP_MAX_CONNECT_RETRY 120 #define BGP_DEFAULT_EBGP_ROUTEADV 0 #define BGP_DEFAULT_IBGP_ROUTEADV 0 From 4ac77b199e0547c8457dbf7e94f159c1fbe57d06 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Wed, 11 Dec 2024 15:40:07 +0200 Subject: [PATCH 4/4] tests: Wait 30 seconds for a notification check (minimum hold-time) Signed-off-by: Donatas Abraitis --- .../topotests/bgp_minimum_holdtime/test_bgp_minimum_holdtime.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/topotests/bgp_minimum_holdtime/test_bgp_minimum_holdtime.py b/tests/topotests/bgp_minimum_holdtime/test_bgp_minimum_holdtime.py index c9ff2ffc7e7c..d2d6a40ae884 100755 --- 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"