-
Notifications
You must be signed in to change notification settings - Fork 1.4k
bgpd: Additional commits for graceful restart #20239
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
base: master
Are you sure you want to change the base?
Conversation
76f9b80 to
d79eadc
Compare
| if (peer_immediate_announce(peer, paf->afi, paf->safi)) { | ||
| update_subgroup_split_peer(paf, updgrp); | ||
| subgrp = paf->subgroup; | ||
| subgrp->v_coalesce = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we stop subgrp->t_coalesce (in case this is very large and running yet)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it
bgpd/bgpd.c
Outdated
|
|
||
| bgp_peer_gr_flags_update(peer); | ||
| /* Initialize per peer bgp GR FSM */ | ||
| bgp_peer_gr_init(peer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this commit really part of the coalesce timer foobar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is separate commit, related to GR. All commits in this MR are improvements in GR
bgpd/bgp_updgrp.c
Outdated
| if (old_subgrp) { | ||
| /* | ||
| * If we need to announce immediately, put peer in its | ||
| * own group and set its coalsece timer to 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo coalsece?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it
bgpd/bgp_route.c
Outdated
| bgp_zebra_route_install(dest, new_select, bgp, true, | ||
| NULL, false); | ||
| if (CHECK_FLAG(bgp->gr_info[afi][safi].flags, BGP_GR_SKIP_BP)) | ||
| bgp_zebra_announce_actual(dest, old_select, bgp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? Install the old_select?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it
When acting as a Helper router for a Restarting router, skip running the coalesce timer after the peering comes up, but immediately announce the routes. This is to facilitate the Restarting router immediately get the latest set of routes and start its path selection. Likewise, skip an extra run of the coalesce timer on the Restarting router. Routes will only be announced after deferred path selection. Signed-off-by: Vivek Venkatraman <[email protected]>
d79eadc to
f48118a
Compare
when the bgp warmboot is trigerred, all the subgroups from the peer paf are deleted. but the corresponding router annouce timer are not stopped. If this timer get scheduled in mean time, we see the crash. Fix: stop timer before clearing the subgoup from paf. Signed-off-by: Vijayalaxmi Basavaraj <[email protected]>
Decoupling GR and backpressure i.e. in case GR is kicked in, the deferred best path selection will continue its normal flow of BGP installing/uninstalling routes directly into zebra rather than applying backpressure logic of processing it later. Signed-off-by: Rajasekar Raja <[email protected]>
f48118a to
2da9f0f
Compare
| bool route_sync; | ||
| bool select_defer_over; | ||
| uint8_t flags; | ||
| #define BGP_GR_SKIP_BP (1 << 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does it mean (extended version)?
Please refer to individual commits