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.

Sign up for GitHub

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

Jump to bottom

implement SBFD #17336

Merged
merged 16 commits into from
Feb 4, 2025
Merged

implement SBFD #17336

merged 16 commits into from
Feb 4, 2025

Conversation

forrestchu
Copy link
Contributor

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

@riw777 riw777 self-requested a review November 5, 2024 15:22
riw777
riw777 reviewed Nov 12, 2024
View reviewed changes
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

riw777
riw777 requested changes Nov 12, 2024
View reviewed changes
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.

@github-actions GitHub Actions
Copy link

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

@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
Contributor 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.

@github-actions github-actions bot added the rebase PR needs rebase label Jan 6, 2025
@forrestchu
Copy link
Contributor Author

Hello, @riw777 @donaldsharp greetings :)
The Docs and topotests are added. For the topotests, there are 3 scenarios to use SBFD according to the document:

  1. SBFD with SRv6 encapsulation
  2. echo SBFD with SRv6 encapsulation
  3. normal SBFD with no SRv6 encapsulation

Currently only scenario-3 is topo-tested. Since for scenario-1 and scenario-2, they depend on the PR(#16894) to implement the SRv6 locator Functions.
I will raise another topotest PR for scenario-1 and scenario-2 once the PR #16894 is merged.

Thanks & Regards.

@frrbot frrbot bot added the bfd label Jan 7, 2025
@frrbot frrbot bot added documentation tests Topotests, make check, etc labels Jan 7, 2025
@forrestchu forrestchu force-pushed the sbfd branch 6 times, most recently from 5e0b3d4 to 95d9081 Compare January 7, 2025 09:02
pguibert6WIND
pguibert6WIND reviewed Jan 21, 2025
View reviewed changes
lib/bfd.c Outdated Show resolved Hide resolved
pguibert6WIND
pguibert6WIND reviewed Jan 21, 2025
View reviewed changes
lib/bfd.c Outdated Show resolved Hide resolved
@pguibert6WIND
Copy link
Member

for frrbot complaints, use the git clang-format HEAD~1 for each commit so that individual commits will all be aligned with frrbot.

@Jafaral
Copy link
Member

Jafaral commented Jan 21, 2025

Topotest need to be updated to use unified configs

riw777
riw777 approved these changes Jan 22, 2025
View reviewed changes
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 ...

@riw777
Copy link
Member

riw777 commented Jan 22, 2025

lint errors need to be fixed ...

@forrestchu
bfdd: add bfdname in bfd_key
c4f9964 
sbfd will use bfdname for key hash, We introduced a bfd-name for every sbfd session, normal BFD sessions can leave it as NULL.
A unique bfd-name can be used to identify a sbfd session quickly. This is quite useful in our Srv6 deployment for path protection case.
For example, if use the sbfd session to protect the SRv6 path A-B-D, we would assign the name 'path-a-b-d' or 'a-b-d' to the session.

Signed-off-by: wumu.zsl <[email protected]>
@forrestchu
bfdd: modify frr-bfdd.yang for sbfd echo and sbfd init
10e8a2e 
Signed-off-by: wumu.zsl <[email protected]>
@forrestchu
bfdd: add vtysh commands for sbfd
2d6a426 
config examples:
    SBFD Initiator: peer 200::D bfd-mode sbfd-init bfd-name a-b-d multihop local-address 200::A remote-discr 456 srv6-source-ipv6 200::A srv6-encap-data 100::B 100::D
    SBFD Reflector: sbfd reflector source-address 200::D discriminator 456
    Echo SBFD: peer 200::A bfd-mode sbfd-echo bfd-name a-b-d local-address 200::A srv6-source-ipv6 200::A srv6-encap-data 100::B 100::D

Signed-off-by: wumu.zsl <[email protected]>
@forrestchu
bfdd: support sbfd session create and destroy
12af5e1 
refactor bfd_session_create and bfd_session_destroy to support SBFD

Signed-off-by: wumu.zsl <[email protected]>
@forrestchu
bfdd: implement Xpath callbacks for SBFD
e58f018 
Signed-off-by: wumu.zsl <[email protected]>
@forrestchu
bfdd: add sbfd config commands to bfdd cli
311f5d4 
Signed-off-by: wumu.zsl <[email protected]>
@forrestchu
bfdd: adjust show commands for SBFD
868c4d0 
Signed-off-by: wumu.zsl <[email protected]>
@forrestchu
bfdd: create a hash table for local sbfd reflector info
5107e27 
Signed-off-by: wumu.zsl <[email protected]>
@forrestchu
bfdd: implement functions for sending sbfd pkts with SRv6 header
e6a531d 
Two types of sbfd packets are supported: initiator packet and echo packet

Signed-off-by: wumu.zsl <[email protected]>
@forrestchu
bfdd: add sbfd state machine functions
c58cbb2 
Signed-off-by: wumu.zsl <[email protected]>
@forrestchu
bfdd: enable sbfd session
3548b30 
1) create socket to send sbfd packets
2) integrate sbfd logic with existing BFD

Signed-off-by: wumu.zsl <[email protected]>
@forrestchu
bfdd: add bfdname info when notify bfd status to PTM
0ef6003 
Signed-off-by: wumu.zsl <[email protected]>
@forrestchu
bfdd: Fix CI build issue on kernel with no IPV6_HDRINCL
e766b68 
Signed-off-by: wumu.zsl <[email protected]>
@forrestchu
tests: add basic topotest cases for sbfd Initiator and Reflector
8653a47 
Signed-off-by: wumu.zsl <[email protected]>
@forrestchu
doc: add sbfd doc
427cf66 
Signed-off-by: wumu.zsl <[email protected]>
@forrestchu
Copy link
Contributor Author

lint errors need to be fixed ...

lint error fixed.

@forrestchu
Copy link
Contributor Author

Topotest need to be updated to use unified configs

Thanks for the suggestion, Topotest updated to use frr.conf

@forrestchu
Copy link
Contributor Author

ci:rerun

1 similar comment
@forrestchu
Copy link
Contributor Author

ci:rerun

pguibert6WIND
pguibert6WIND approved these changes Jan 24, 2025
View reviewed changes
Copy link
Member

@pguibert6WIND pguibert6WIND left a comment

Choose a reason for hiding this comment

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

LGTM

@riw777 riw777 merged commit adeb30d into FRRouting:master Feb 4, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pguibert6WIND pguibert6WIND pguibert6WIND approved these changes

@riw777 riw777 riw777 approved these changes

Assignees
No one assigned
Labels
bfd documentation master rebase PR needs rebase size/XXL tests Topotests, make check, etc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@forrestchu @donaldsharp @riw777 @pguibert6WIND @Jafaral