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

Implement BGP-wide configuration for graceful restart #16099

Merged
merged 9 commits into from
Jul 2, 2024

Conversation

Pdoijode
Copy link
Contributor

@Pdoijode Pdoijode commented May 28, 2024

Commit 1:
Add support for a BGP-wide setting for graceful restart modes and
parameters. This setting will apply to all BGP peers across all BGP
instances, but per-neighbor configuration can override it.
Per-instance configuration is disallowed if the BGP-wide setting
is in effect.

Commit2:
Streamline the BGP graceful-restart configuration at the global and
peer level some more. Similar to many other neighbor capability
parameters like MP and ENHE, reset the session immediately upon a
change to the configuration. This will be more aligned with the
transactional UI model also and will not require a separate 'clear'
command to be executed.

Note: Peer-group graceful-restart configuration is not yet supported.

Commit3:
bgpd: Refine restarter operation - R-bit & F-bit

Introduce BGP-wide flags to denote if BGP has started gracefully
and GR is in progress or not. Use this for setting of the R-bit in
the GR capability, and not a timer which is set for any new
instance creation. Mark graceful restart is complete when the
deferred path selection has been done and route sync with zebra as
well as deferred EOR advertisement has been initiated.

Introduce a function to check on F-bit setting rather than just
base it on configuration.

Subsequent commits will extend these functionalities.

Commit 4:
bgpd: Refine OPEN debug logs for graceful restart

This also fixes Rx F-bit log which was incorrect.

@frrbot frrbot bot added the bgp label May 28, 2024
@Pdoijode Pdoijode force-pushed the pdoijode/bgp-gr2 branch from d962059 to 2165b33 Compare May 28, 2024 21:37
@Pdoijode
Copy link
Contributor Author

ci:rerun

@Pdoijode Pdoijode force-pushed the pdoijode/bgp-gr2 branch 2 times, most recently from a0e03bd to 80d92e8 Compare May 28, 2024 22:44
@ton31337 ton31337 self-requested a review May 29, 2024 10:19
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.

Need a documentation update and topotests yet.

bgpd/bgp_fsm.c Outdated Show resolved Hide resolved
bgpd/bgp_open.c Outdated Show resolved Hide resolved
bgpd/bgp_open.c Outdated Show resolved Hide resolved
bgpd/bgp_route.c Outdated Show resolved Hide resolved
bgpd/bgp_route.c Outdated Show resolved Hide resolved
bgpd/bgp_vty.c Show resolved Hide resolved
bgpd/bgp_vty.c Outdated Show resolved Hide resolved
bgpd/bgp_vty.c Outdated Show resolved Hide resolved
bgpd/bgp_vty.c Show resolved Hide resolved
bgpd/bgp_fsm.c Outdated Show resolved Hide resolved
@Pdoijode Pdoijode force-pushed the pdoijode/bgp-gr2 branch 2 times, most recently from 5825807 to dde73af Compare May 29, 2024 21:30
@Pdoijode
Copy link
Contributor Author

Working on updating documentation and topotest.

@Pdoijode Pdoijode force-pushed the pdoijode/bgp-gr2 branch from cb6b85a to 4456f95 Compare June 3, 2024 18:36
@Pdoijode
Copy link
Contributor Author

Pdoijode commented Jun 3, 2024

ci:rerun

@riw777 riw777 self-requested a review June 4, 2024 15:29
@Pdoijode Pdoijode force-pushed the pdoijode/bgp-gr2 branch 2 times, most recently from 0d7c60b to 4bbb1eb Compare June 5, 2024 18:14
@ton31337
Copy link
Member

This will be reviewed/merged after the stabilization branch is pulled (stable/10.1).

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.

one small question, but code looks good

bgpd/bgp_fsm.c Outdated Show resolved Hide resolved
@frrbot frrbot bot added the tests Topotests, make check, etc label Jun 24, 2024
@ton31337
Copy link
Member

Could you also rebase? It's quite too much behind the base.

@Pdoijode Pdoijode force-pushed the pdoijode/bgp-gr2 branch 2 times, most recently from b6615f6 to e962370 Compare June 25, 2024 19:20
@Pdoijode
Copy link
Contributor Author

Added new commit: *: Add and use option for graceful (re)start

@Pdoijode Pdoijode force-pushed the pdoijode/bgp-gr2 branch 4 times, most recently from a93fed3 to 233a6eb Compare June 27, 2024 01:28
@riw777 riw777 requested a review from ton31337 June 27, 2024 15:35
@Pdoijode
Copy link
Contributor Author

ci:rerun

@Pdoijode
Copy link
Contributor Author

@riw777, I have taken care of changes requested by @ton31337.

vivek-cumulus and others added 3 commits June 27, 2024 11:40
Add support for a BGP-wide setting for graceful restart modes and
parameters. This setting will apply to all BGP peers across all BGP
instances, but per-neighbor configuration can override it.
Per-instance configuration is disallowed if the BGP-wide setting
is in effect.

Signed-off-by: Vivek Venkatraman <[email protected]>
Signed-off-by: Pooja Jagadeesh Doijode <[email protected]>
Streamline the BGP graceful-restart configuration at the global and
peer level some more. Similar to many other neighbor capability
parameters like MP and ENHE, reset the session immediately upon a
change to the configuration. This will be more aligned with the
transactional UI model also and will not require a separate 'clear'
command to be executed.

Note: Peer-group graceful-restart configuration is not yet supported.

Signed-off-by: Vivek Venkatraman <[email protected]>
@Pdoijode
Copy link
Contributor Author

Rebased again with master

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.

I don't see --graceful_restart documented, or I'm blind?

@Pdoijode
Copy link
Contributor Author

@ton31337, I have added documentation for newly introduced global commands here: ed1e90a. Other than these, no new commands were added and GR functionality hasn't changed. Let me know if I'm missing something.

@ton31337
Copy link
Member

@ton31337, I have added documentation for newly introduced global commands here: ed1e90a. Other than these, no new commands were added and GR functionality hasn't changed. Let me know if I'm missing something.

I meant -K isn't documented, is it?

@Pdoijode
Copy link
Contributor Author

Wasn't aware that daemon options were also documented in .rst. Thanks for pointing it out. Updated documentation in 6f342c1

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.

Please drop this commit and do the actual changes instead of back and forth. I'm about 555d8fc and d8b898f.

@Pdoijode Pdoijode force-pushed the pdoijode/bgp-gr2 branch 2 times, most recently from 87a1bc6 to f7a73fa Compare July 1, 2024 19:39
vivek-cumulus and others added 6 commits July 1, 2024 13:02
Introduce BGP-wide flags to denote if BGP has started gracefully
and GR is in progress or not. Use this for setting of the R-bit in
the GR capability, and not a timer which is set for any new
instance creation. Mark graceful restart is complete when the
deferred path selection has been done and route sync with zebra as
well as deferred EOR advertisement has been initiated.

Introduce a function to check on F-bit setting rather than just
base it on configuration.

Subsequent commits will extend these functionalities.

Signed-off-by: Vivek Venkatraman <[email protected]>
This also fixes Rx F-bit log which was incorrect.

Signed-off-by: Vivek Venkatraman <[email protected]>
Signed-off-by: Vivek Venkatraman <[email protected]>
Added topotest and documentation for BGP wide GR configurations

Signed-off-by: Pooja Jagadeesh Doijode <[email protected]>
Add a new start option "-K" to libfrr to denote a graceful start,
and use it in zebra and bgpd.

zebra will use this option to denote a planned FRR graceful restart
(supporting only bgpd currently) to wait for a route sync completion
from bgpd before cleaning up old stale routes from the FIB. An optional
timer provides an upper-bounds for this cleanup.

bgpd will use this option to denote either a planned FRR graceful
restart or a bgpd-only graceful restart, and this will drive the BGP
GR restarting router procedures.

Signed-off-by: Vivek Venkatraman <[email protected]>
@Pdoijode Pdoijode force-pushed the pdoijode/bgp-gr2 branch from f7a73fa to b5682ff Compare July 1, 2024 20:04
@Pdoijode
Copy link
Contributor Author

Pdoijode commented Jul 2, 2024

ci:rerun

@Pdoijode
Copy link
Contributor Author

Pdoijode commented Jul 2, 2024

Dropped 555d8fc and made the changes in d8b898f

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.

LGTM

@ton31337 ton31337 merged commit 9ab4186 into FRRouting:master Jul 2, 2024
11 checks passed
Jafaral added a commit that referenced this pull request Nov 12, 2024
New Features Highlight:

- PIM candidate BSR/RP [#16438]
- Static IGMP join without an IGMP report [1#6450]
- PIM AutoRP discovery/announcements [#16634]
- IGMP proxy [#16861]
- SRv6 SID Manager [#15604]
- Add `bgp ipv6-auto-ra` command [#16354]
- Implement `neighbor x remote-as auto` for BGP [#16345]
- Implement `bgp dual-as` for BGP [#16816]
- Implement BGP-wide configuration for graceful restart [#16099]
- Handle kernel routes appropriately (should fix recent NOPREFIXROUTE issue) [#16300]
- Add `cisco-authentication` password support for NHRP [#16172]

Signed-off-by: Jafar Al-Gharaibeh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bgp master rebase PR needs rebase size/XXL tests Topotests, make check, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants