Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bgpd: allow batch handling of peer shutdown/failure #17505

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mjstapp
Copy link
Contributor

@mjstapp mjstapp commented Nov 25, 2024

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.

Mark Stapp added 2 commits November 25, 2024 14:13
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.

Signed-off-by: Mark Stapp <mjs@cisco.com>
Remove a couple of apis that don't exist.

Signed-off-by: Mark Stapp <mjs@cisco.com>
@frrbot frrbot bot added bgp tests Topotests, make check, etc zebra labels Nov 25, 2024
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice improvement ahead!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mjstapp
Copy link
Contributor Author

mjstapp commented Nov 26, 2024

Pushed to try to clean up the build problem

Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@donaldsharp
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

@github-actions github-actions bot added the rebase PR needs rebase label Dec 2, 2024
@mjstapp
Copy link
Contributor Author

mjstapp commented Dec 2, 2024

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

Mark Stapp added 3 commits December 3, 2024 14:23
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.

Signed-off-by: Mark Stapp <mjs@cisco.com>
Move the peer connection error list to the peer_connection
struct; that seems to line up better with the way that struct
works.

Signed-off-by: Mark Stapp <mjs@cisco.com>
Add a simple topotest using multiple bgp peers; based on the
ecmp_topo1 test.

Signed-off-by: Mark Stapp <mjs@cisco.com>
@mjstapp
Copy link
Contributor Author

mjstapp commented Dec 4, 2024

CI:rerun

@donaldsharp
Copy link
Member

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

Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants