bgpd: allow batch handling of peer shutdown/failure

[mjstapp]

When a peer connection fails or is closed, bgp does cleanup processing on a per-peer basis. At scale, this can become a problem - bgp can be forced to make a complete rib walk to clean up for each peer involved. This PR makes peer error-handling more visible at the bgp object level, and then adds a batching path if there are multiple peers who need cleanup/clearing processing at the same time.

• Replace the per-peer connection error with a per-bgp event and a list. The io pthread enqueues peers per-bgp-instance, and the error-handing code can process multiple peers if there have been multiple failures.
• When peer connections encounter errors, attempt to batch some of the clearing processing that occurs. Add a new batch object, add multiple peers to it, if possible. Do one rib walk for the batch, rather than one walk per peer. Use a handler callback per batch to check and remove peers' path-infos, rather than a work-queue and callback per peer. The original clearing code remains; it's used for single peers.

[ton31337]

Very nice improvement ahead!

[ton31337]

Can we switch to frr.conf (unified config)?

[mjstapp]

Pushed to try to clean up the build problem

[riw777]

looks good ... waiting on @ton31337 's one comment

[donaldsharp]

diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index 2f21cfd76d..14c280b7ca 100644
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -6203,7 +6203,7 @@ static void bgp_clear_batch_dests_task(struct event *event)
 {
        struct bgp_clearing_info *cinfo = EVENT_ARG(event);
        struct bgp_dest *dest;
-       struct bgp_path_info *pi;
+       struct bgp_path_info *pi, *next;
        struct bgp_table *table;
        struct bgp *bgp;
        afi_t afi;
@@ -6225,7 +6225,8 @@ next_dest:
        /* Have to check every path: it is possible that we have multiple paths
         * for a prefix from a peer if that peer is using AddPath.
         */
-       for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next) {
+       for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = next) {
+               next = pi ? pi->next : NULL;
                if (!bgp_clearing_batch_check_peer(cinfo, pi->peer))
                        continue;
 

[donaldsharp]

the above patch will fix the infinite loops we get stuck in sometimes with this code. Effectively when you call bgp_process the pi->next pointer can be reset.

[donaldsharp]

there is also a crash that I am chasing down w/ Mark that I am seeing locally

[mjstapp]

rebased to apply a couple of fixes - let's see how CI looks

[mjstapp]

CI:rerun

[donaldsharp]

Spoke w/ Mark he's going to make a change so that all peer clearing events go through his batching

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.