Merge pull request #17501 from opensourcerouting/fix/gr_peer-groups

bgpd: Fix Graceful-Restart for peer-groups

bgpd/bgp_fsm.c

@@ -2726,33 +2726,55 @@ static void bgp_gr_update_mode_of_all_peers(struct bgp *bgp,
 	struct listnode *node = {0};
 	struct listnode *nnode = {0};
 	enum peer_mode peer_old_state = PEER_INVALID;
-
+	struct peer_group *group;
-	/* TODO: Need to handle peer-groups. */
+	struct peer *member;
 
 	for (ALL_LIST_ELEMENTS(bgp->peer, node, nnode, peer)) {
-		peer_old_state = bgp_peer_gr_mode_get(peer);
+		if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
-		if (peer_old_state != PEER_GLOBAL_INHERIT)
+			peer_old_state = bgp_peer_gr_mode_get(peer);
-			continue;
+			if (peer_old_state != PEER_GLOBAL_INHERIT)
+				continue;
 
-		bgp_peer_inherit_global_gr_mode(peer, global_new_state);
+			bgp_peer_inherit_global_gr_mode(peer, global_new_state);
-		bgp_peer_gr_flags_update(peer);
+			bgp_peer_gr_flags_update(peer);
 
-		if (BGP_DEBUG(graceful_restart, GRACEFUL_RESTART))
+			if (BGP_DEBUG(graceful_restart, GRACEFUL_RESTART))
-			zlog_debug("%pBP: Inherited Global GR mode, GR flags 0x%x peer flags 0x%" PRIx64
+				zlog_debug("%pBP: Inherited Global GR mode, GR flags 0x%x peer flags 0x%" PRIx64
-				   "...resetting session",
+					   "...resetting session",
-				   peer, peer->peer_gr_new_status_flag,
+					   peer, peer->peer_gr_new_status_flag, peer->flags);
-				   peer->flags);
 
-		peer->last_reset = PEER_DOWN_CAPABILITY_CHANGE;
+			peer->last_reset = PEER_DOWN_CAPABILITY_CHANGE;
 
-		/* Reset session to match with behavior for other peer
+			if (BGP_IS_VALID_STATE_FOR_NOTIF(peer->connection->status))
-		 * configs that require the session to be re-setup.
+				bgp_notify_send(peer->connection, BGP_NOTIFY_CEASE,
-		 */
+						BGP_NOTIFY_CEASE_CONFIG_CHANGE);
-		if (BGP_IS_VALID_STATE_FOR_NOTIF(peer->connection->status))
+			else
-			bgp_notify_send(peer->connection, BGP_NOTIFY_CEASE,
+				bgp_session_reset_safe(peer, &nnode);
-					BGP_NOTIFY_CEASE_CONFIG_CHANGE);
+		} else {
-		else
+			group = peer->group;
-			bgp_session_reset_safe(peer, &nnode);
+			for (ALL_LIST_ELEMENTS(group->peer, node, nnode, member)) {
+				peer_old_state = bgp_peer_gr_mode_get(member);
+				if (peer_old_state != PEER_GLOBAL_INHERIT)
+					continue;
+
+				bgp_peer_inherit_global_gr_mode(member, global_new_state);
+				bgp_peer_gr_flags_update(member);
+
+				if (BGP_DEBUG(graceful_restart, GRACEFUL_RESTART))
+					zlog_debug("%pBP: Inherited Global GR mode, GR flags 0x%x peer flags 0x%" PRIx64
+						   "...resetting session",
+						   member, member->peer_gr_new_status_flag,
+						   member->flags);
+
+				member->last_reset = PEER_DOWN_CAPABILITY_CHANGE;
+
+				if (BGP_IS_VALID_STATE_FOR_NOTIF(member->connection->status))
+					bgp_notify_send(member->connection, BGP_NOTIFY_CEASE,
+							BGP_NOTIFY_CEASE_CONFIG_CHANGE);
+				else
+					bgp_session_reset(member);
+			}
+		}
 	}
 }
 
@@ -2911,6 +2933,9 @@ unsigned int bgp_peer_gr_action(struct peer *peer, enum peer_mode old_state,
 {
 	enum global_mode global_gr_mode;
 	bool session_reset = true;
+	struct peer_group *group;
+	struct peer *member;
+	struct listnode *node, *nnode;
 
 	if (old_state == new_state)
 		return BGP_GR_NO_OPERATION;
@@ -2945,16 +2970,27 @@ unsigned int bgp_peer_gr_action(struct peer *peer, enum peer_mode old_state,
 	bgp_peer_move_to_gr_mode(peer, new_state);
 
 	if (session_reset) {
-		peer->last_reset = PEER_DOWN_CAPABILITY_CHANGE;
+		if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
+			peer->last_reset = PEER_DOWN_CAPABILITY_CHANGE;
 
-		/* Reset session to match with behavior for other peer
+			if (BGP_IS_VALID_STATE_FOR_NOTIF(peer->connection->status))
-		 * configs that require the session to be re-setup.
+				bgp_notify_send(peer->connection, BGP_NOTIFY_CEASE,
-		 */
+						BGP_NOTIFY_CEASE_CONFIG_CHANGE);
-		if (BGP_IS_VALID_STATE_FOR_NOTIF(peer->connection->status))
+			else
-			bgp_notify_send(peer->connection, BGP_NOTIFY_CEASE,
+				bgp_session_reset(peer);
-					BGP_NOTIFY_CEASE_CONFIG_CHANGE);
+		} else {
-		else
+			group = peer->group;
-			bgp_session_reset(peer);
+			for (ALL_LIST_ELEMENTS(group->peer, node, nnode, member)) {
+				member->last_reset = PEER_DOWN_CAPABILITY_CHANGE;
+				bgp_peer_move_to_gr_mode(member, new_state);
+
+				if (BGP_IS_VALID_STATE_FOR_NOTIF(member->connection->status))
+					bgp_notify_send(member->connection, BGP_NOTIFY_CEASE,
+							BGP_NOTIFY_CEASE_CONFIG_CHANGE);
+				else
+					bgp_session_reset(member);
+			}
+		}
 	}
 
 	return BGP_GR_SUCCESS;

bgpd/bgp_vty.c

@@ -3519,11 +3519,6 @@ DEFUN (bgp_neighbor_graceful_restart_set,
 	peer = peer_and_group_lookup_vty(vty, argv[idx_peer]->arg);
 	if (!peer)
 		return CMD_WARNING_CONFIG_FAILED;
-	if (CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
-		vty_out(vty,
-			"Per peer-group graceful-restart configuration is not yet supported\n");
-		return CMD_WARNING_CONFIG_FAILED;
-	}
 
 	result = bgp_neighbor_graceful_restart(peer, PEER_GR_CMD);
 	if (result == BGP_GR_SUCCESS) {
@@ -3554,11 +3549,6 @@ DEFUN (no_bgp_neighbor_graceful_restart,
 	peer = peer_and_group_lookup_vty(vty, argv[idx_peer]->arg);
 	if (!peer)
 		return CMD_WARNING_CONFIG_FAILED;
-	if (CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
-		vty_out(vty,
-			"Per peer-group graceful-restart configuration is not yet supported\n");
-		return CMD_WARNING_CONFIG_FAILED;
-	}
 
 	result = bgp_neighbor_graceful_restart(peer, NO_PEER_GR_CMD);
 	if (ret == BGP_GR_SUCCESS) {
@@ -3588,11 +3578,6 @@ DEFUN (bgp_neighbor_graceful_restart_helper_set,
 	peer = peer_and_group_lookup_vty(vty, argv[idx_peer]->arg);
 	if (!peer)
 		return CMD_WARNING_CONFIG_FAILED;
-	if (CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
-		vty_out(vty,
-			"Per peer-group graceful-restart configuration is not yet supported\n");
-		return CMD_WARNING_CONFIG_FAILED;
-	}
 
 	ret = bgp_neighbor_graceful_restart(peer, PEER_HELPER_CMD);
 	if (ret == BGP_GR_SUCCESS) {
@@ -3623,11 +3608,6 @@ DEFUN (no_bgp_neighbor_graceful_restart_helper,
 	peer = peer_and_group_lookup_vty(vty, argv[idx_peer]->arg);
 	if (!peer)
 		return CMD_WARNING_CONFIG_FAILED;
-	if (CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
-		vty_out(vty,
-			"Per peer-group graceful-restart configuration is not yet supported\n");
-		return CMD_WARNING_CONFIG_FAILED;
-	}
 
 	ret = bgp_neighbor_graceful_restart(peer, NO_PEER_HELPER_CMD);
 	if (ret == BGP_GR_SUCCESS) {
@@ -3657,11 +3637,6 @@ DEFUN (bgp_neighbor_graceful_restart_disable_set,
 	peer = peer_and_group_lookup_vty(vty, argv[idx_peer]->arg);
 	if (!peer)
 		return CMD_WARNING_CONFIG_FAILED;
-	if (CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
-		vty_out(vty,
-			"Per peer-group graceful-restart configuration is not yet supported\n");
-		return CMD_WARNING_CONFIG_FAILED;
-	}
 
 	ret = bgp_neighbor_graceful_restart(peer, PEER_DISABLE_CMD);
 	if (ret == BGP_GR_SUCCESS) {
@@ -3693,11 +3668,6 @@ DEFUN (no_bgp_neighbor_graceful_restart_disable,
 	peer = peer_and_group_lookup_vty(vty, argv[idx_peer]->arg);
 	if (!peer)
 		return CMD_WARNING_CONFIG_FAILED;
-	if (CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) {
-		vty_out(vty,
-			"Per peer-group graceful-restart configuration is not yet supported\n");
-		return CMD_WARNING_CONFIG_FAILED;
-	}
 
 	ret = bgp_neighbor_graceful_restart(peer, NO_PEER_DISABLE_CMD);
 	if (ret == BGP_GR_SUCCESS) {
@@ -14927,22 +14897,31 @@ static void bgp_show_peer(struct vty *vty, struct peer *p, bool use_json,
 			if (CHECK_FLAG(p->cap, PEER_CAP_RESTART_RCV) ||
 			    CHECK_FLAG(p->cap, PEER_CAP_RESTART_ADV)) {
 				if (CHECK_FLAG(p->cap, PEER_CAP_RESTART_ADV) &&
-				    CHECK_FLAG(p->cap, PEER_CAP_RESTART_RCV))
+				    CHECK_FLAG(p->cap, PEER_CAP_RESTART_RCV)) {
 					json_object_string_add(
 						json_cap, "gracefulRestart",
 						"advertisedAndReceived");
-				else if (CHECK_FLAG(p->cap,
+				} else if (CHECK_FLAG(p->cap, PEER_CAP_RESTART_ADV)) {
-						    PEER_CAP_RESTART_ADV))
+					json_object_string_add(json_cap, "gracefulRestart",
+							       "advertised");
+#if CONFDATE > 20250525
+CPP_NOTICE("Remove `gracefulRestartCapability` JSON field")
+#endif
 					json_object_string_add(
 						json_cap,
 						"gracefulRestartCapability",
 						"advertised");
-				else if (CHECK_FLAG(p->cap,
+				} else if (CHECK_FLAG(p->cap, PEER_CAP_RESTART_RCV)) {
-						    PEER_CAP_RESTART_RCV))
+					json_object_string_add(json_cap, "gracefulRestart",
+							       "received");
+#if CONFDATE > 20250525
+CPP_NOTICE("Remove `gracefulRestartCapability` JSON field")
+#endif
 					json_object_string_add(
 						json_cap,
 						"gracefulRestartCapability",
 						"received");
+				}
 
 				if (CHECK_FLAG(p->cap, PEER_CAP_RESTART_RCV)) {
 					int restart_af_count = 0;

bgpd/bgpd.c

@@ -3022,6 +3022,7 @@ static void peer_group2peer_config_copy(struct peer_group *group,
 	PEER_ATTR_INHERIT(peer, group, local_role);
 
 	/* Update GR flags for the peer. */
+	PEER_ATTR_INHERIT(peer, group, peer_gr_new_status_flag);
 	bgp_peer_gr_flags_update(peer);
 
 	/* Apply BFD settings from group to peer if it exists. */

tests/topotests/bgp_peer_group/r1/bgpd.conf

@@ -1,12 +0,0 @@
-!
-router bgp 65001
-  neighbor PG peer-group
-  neighbor PG remote-as external
-  neighbor PG timers 3 10
-  neighbor 192.168.255.3 peer-group PG
-  neighbor r1-eth0 interface peer-group PG
-  neighbor PG1 peer-group
-  neighbor PG1 remote-as external
-  neighbor PG1 timers 3 20
-  neighbor 192.168.251.2 peer-group PG1
-!

tests/topotests/bgp_peer_group/r1/frr.conf

@@ -0,0 +1,21 @@
+!
+interface r1-eth0
+ ip address 192.168.255.1/24
+!
+interface r1-eth1
+ ip address 192.168.251.1/30
+!
+ip forwarding
+!
+router bgp 65001
+ neighbor PG peer-group
+ neighbor PG remote-as external
+ neighbor PG timers 3 10
+ neighbor 192.168.255.3 peer-group PG
+ neighbor r1-eth0 interface peer-group PG
+ neighbor PG1 peer-group
+ neighbor PG1 remote-as external
+ neighbor PG1 timers 3 20
+ neighbor PG1 graceful-restart-disable
+ neighbor 192.168.251.2 peer-group PG1
+!

tests/topotests/bgp_peer_group/r1/zebra.conf

@@ -1,9 +0,0 @@
-!
-interface r1-eth0
- ip address 192.168.255.1/24
-!
-interface r1-eth1
- ip address 192.168.251.1/30
-!
-ip forwarding
-!

tests/topotests/bgp_peer_group/r2/bgpd.conf

@@ -1,11 +0,0 @@
-!
-router bgp 65002
-  neighbor PG peer-group
-  neighbor PG remote-as external
-  neighbor PG timers 3 10
-  neighbor r2-eth0 interface peer-group PG
-  neighbor PG1 peer-group
-  neighbor PG1 remote-as external
-  neighbor PG1 timers 3 20
-  neighbor 192.168.251.1 peer-group PG1
-!

tests/topotests/bgp_peer_group/r2/frr.conf

@@ -0,0 +1,19 @@
+!
+interface r2-eth0
+ ip address 192.168.255.2/24
+!
+interface r2-eth1
+ ip address 192.168.251.2/30
+!
+ip forwarding
+!
+router bgp 65002
+ neighbor PG peer-group
+ neighbor PG remote-as external
+ neighbor PG timers 3 10
+ neighbor r2-eth0 interface peer-group PG
+ neighbor PG1 peer-group
+ neighbor PG1 remote-as external
+ neighbor PG1 timers 3 20
+ neighbor 192.168.251.1 peer-group PG1
+!

tests/topotests/bgp_peer_group/r2/zebra.conf

@@ -1,9 +0,0 @@
-!
-interface r2-eth0
- ip address 192.168.255.2/24
-!
-interface r2-eth1
- ip address 192.168.251.2/30
-!
-ip forwarding
-!

tests/topotests/bgp_peer_group/r3/frr.conf

@@ -1,4 +1,9 @@
 !
+interface r3-eth0
+ ip address 192.168.255.3/24
+!
+ip forwarding
+!
 router bgp 65003
  no bgp ebgp-requires-policy
  neighbor PG peer-group

tests/topotests/bgp_peer_group/r3/zebra.conf

@@ -1,6 +0,0 @@
-!
-interface r3-eth0
- ip address 192.168.255.3/24
-!
-ip forwarding
-!

tests/topotests/bgp_peer_group/test_bgp_peer-group.py

@@ -2,12 +2,14 @@
 # SPDX-License-Identifier: ISC
 
 #
-# Copyright (c) 2021 by
+# Copyright (c) 2021-2024 by
 # Donatas Abraitis <donatas.abraitis@gmail.com>
+# Donatas Abraitis <donatas@opensourcerouting.org>
 #
 
 """
-Test if peer-group works for numbered and unnumbered configurations.
+Test if various random settings with peer-group works for
+numbered and unnumbered configurations.
 """
 
 import os
@@ -21,7 +23,7 @@ sys.path.append(os.path.join(CWD, "../"))
 
 # pylint: disable=C0413
 from lib import topotest
-from lib.topogen import Topogen, TopoRouter, get_topogen
+from lib.topogen import Topogen, get_topogen
 from lib.topolog import logger
 
 pytestmark = [pytest.mark.bgpd]
@@ -48,12 +50,7 @@ def setup_module(mod):
     router_list = tgen.routers()
 
     for _, (rname, router) in enumerate(router_list.items(), 1):
-        router.load_config(
+        router.load_frr_config(os.path.join(CWD, "{}/frr.conf".format(rname)))
-            TopoRouter.RD_ZEBRA, os.path.join(CWD, "{}/zebra.conf".format(rname))
-        )
-        router.load_config(
-            TopoRouter.RD_BGP, os.path.join(CWD, "{}/bgpd.conf".format(rname))
-        )
 
     tgen.start_router()
 
@@ -72,14 +69,26 @@ def test_bgp_peer_group():
     def _bgp_peer_group_configured():
         output = json.loads(tgen.gears["r1"].vtysh_cmd("show ip bgp neighbor json"))
         expected = {
-            "r1-eth0": {"peerGroup": "PG", "bgpState": "Established"},
+            "r1-eth0": {
-            "192.168.255.3": {"peerGroup": "PG", "bgpState": "Established"},
+                "peerGroup": "PG",
-            "192.168.251.2": {"peerGroup": "PG1", "bgpState": "Established"},
+                "bgpState": "Established",
+                "neighborCapabilities": {"gracefulRestart": "advertisedAndReceived"},
+            },
+            "192.168.255.3": {
+                "peerGroup": "PG",
+                "bgpState": "Established",
+                "neighborCapabilities": {"gracefulRestart": "advertisedAndReceived"},
+            },
+            "192.168.251.2": {
+                "peerGroup": "PG1",
+                "bgpState": "Established",
+                "neighborCapabilities": {"gracefulRestart": "received"},
+            },
         }
         return topotest.json_cmp(output, expected)
 
     test_func = functools.partial(_bgp_peer_group_configured)
-    _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
+    _, result = topotest.run_and_expect(test_func, None, count=30, wait=1)
     assert result is None, "Failed bgp convergence in r1"
 
     def _bgp_peer_group_check_advertised_routes():
@@ -97,7 +106,7 @@ def test_bgp_peer_group():
         return topotest.json_cmp(output, expected)
 
     test_func = functools.partial(_bgp_peer_group_check_advertised_routes)
-    _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
+    _, result = topotest.run_and_expect(test_func, None, count=30, wait=1)
     assert result is None, "Failed checking advertised routes from r3"
 
 
@@ -122,7 +131,7 @@ def test_bgp_peer_group_remote_as_del_readd():
         return topotest.json_cmp(output, expected)
 
     test_func = functools.partial(_bgp_peer_group_remoteas_del)
-    _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
+    _, result = topotest.run_and_expect(test_func, None, count=30, wait=1)
     assert result is None, "Failed bgp convergence in r1"
 
     logger.info("Re-add bgp peer-group PG1 remote-as neighbor should be established")
@@ -139,7 +148,7 @@ def test_bgp_peer_group_remote_as_del_readd():
         return topotest.json_cmp(output, expected)
 
     test_func = functools.partial(_bgp_peer_group_remoteas_add)
-    _, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
+    _, result = topotest.run_and_expect(test_func, None, count=30, wait=1)
     assert result is None, "Failed bgp convergence in r1"