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 SBFD #17336

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

implement SBFD #17336

wants to merge 18 commits into from

Conversation

forrestchu
Copy link

implementing the SBFD feature (RFC7880, RFC7881) in FRR.

What is the motivation for this PR?
The PhoenixWing project aims to implement SRv6 features into the SONiC community. In PhoenixWing traffic engineering case, we use SBFD to protect SRv6 TE paths.

How did you do it?
SBFD HLD in SoNiC community: sonic-net/SONiC#1766

use SBFD to protect TE path, two types of configs are supported:

  1. SBFD echo mode, which mainly used in Our SRv6 TE case. Only need to config at local side:
    configure terminal->
    bfd ->
    peer X::X bfd-mode sbfd-echo bfd-name name local-address X::X encap-type SRv6 encap-data X::X source-ipv6 X::X
  2. SBFD initiator and reflector mode, Need to config at local side and remote side:
    2.1) local config:
    configure terminal->
    bfd ->
    peer X::X bfd-mode sbfd-init bfd-name name local-address X::X encap-type SRv6 encap-data X::X source-ipv6 X::X remote-discr 12345
    2.2) remote config:
    configure terminal ->
    bfd ->
    sbfd reflector source-address X::X discriminator 12345

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 ... I think we need a topo test for this, though

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.

Needs to be broken into multiple commits, and this also needs a topo test.

@donaldsharp
Copy link
Member

a) This needs to be broken up into multiple commits. 4k lines to review is impossible. Break it down into small logical bits of work, this will never be reviewed otherwise
b) Actually take the time and write a topotest to show that this works.
c) The commit message is utterly useless and will help no-one in the future understand what is going on. This needs to be addressed
d) There is no documentation. This must be added as well.

Without some major changes this is dead in the water.

Copy link

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

Signed-off-by: wumu.zsl <[email protected]>
…heck for existing bfd Xpath callbacks

Signed-off-by: wumu.zsl <[email protected]>
Signed-off-by: wumu.zsl <[email protected]>
… of sbfd pks are supported: init packet and echo packet

Signed-off-by: wumu.zsl <[email protected]>
… use bfdname as a key to track bfd state

Signed-off-by: wumu.zsl <[email protected]>
@riw777
Copy link
Member

riw777 commented Dec 3, 2024

Looks like the docs are in, but we still need a topo test ...

@forrestchu
Copy link
Author

Looks like the docs are in, but we still need a topo test ...

Yes Russ White, Thanks for reviewing the code. The topo test in ongoing, I will update the PR later.

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

Successfully merging this pull request may close these issues.

3 participants