implement SBFD #17336

forrestchu · 2024-11-04T08:51:02Z

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:

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
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

looks good ... I think we need a topo test for this, though

riw777

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

donaldsharp · 2024-11-12T16:53:48Z

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 · 2024-11-26T14:30:20Z

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

riw777 · 2024-12-03T13:44:35Z

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

forrestchu · 2024-12-11T23:56:47Z

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.

forrestchu · 2025-01-06T13:04:50Z

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

SBFD with SRv6 encapsulation
echo SBFD with SRv6 encapsulation
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.

Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

pguibert6WIND · 2025-01-20T09:18:17Z

configure.ac

+dnl ----------------------
+dnl checking for IPV6_HDRINCL
+dnl ----------------------
+AC_MSG_CHECKING([for IPV6_HDRINCL])


is this block really needed ?
I don't see HAVE_IPV6_HDRINCL in the code, so I think is not needed

Yes Philippe, I saw build errors on a lower kernel version linux distribution. Added this to solve the build issue.

frrouting does not test older distros.
if you don't use that flag, will the CI pass?

whatever, I suspect the feature will not work on this older distro.
so why maintaining something that will be broken?

frrouting does not test older distros. if you don't use that flag, will the CI pass?

whatever, I suspect the feature will not work on this older distro. so why maintaining something that will be broken?

CI will fail at building on RedHat 8. So I add this to work around the CI.

ok, can you confirm that if you run in a test the code that uses IPV6_HDRINCL, then the SBFD service will work?
I mean, if the problem is only API related, and the kernel implements it, this is fine.
otherwise, you bring up an inconsistency..

ok, can you confirm that if you run in a test the code that uses IPV6_HDRINCL, then the SBFD service will work? I mean, if the problem is only API related, and the kernel implements it, this is fine. otherwise, you bring up an inconsistency..

IPV6_HDRINCL is a mandatory socket option for SBFD in our implementation. Because we construct the whole IPv6+SRH+IP/IPv6+UDP+BFD data on our own and instruct kernel to route the packet at the same time.
Here is a tcpdump packet with multiple sids encapped:
aa:a6:32:d5:75:64 > 5e:63:7d:1d:be:97, ethertype IPv6 (0x86dd), length 150: 2001::20 > 100::B RT6 (len=2, type=4, segleft=1, last-entry=0, tag=0, [0]100::D) 2001::20.3784 > 2001::10.7784: UDP, length 24

It means that there is a limitation on SBFD that it may not work on lower version kernel since IPV6_HDRINCL not supported.

so if I understand correctly, this setsockopt operation will fail:

if (setsockopt(sd, IPPROTO_IPV6, IPV6_HDRINCL, &on, sizeof(on)))

and sbfd test will fail.
I don't know how the tests are distributed on the various linux distros. Assuming this is random, sooner or later, the sbfd test over redhat8 will fail.

if it is true, you should think of a protection:
what about this code in the place you are using it:

#ifdef IPV6_HDRINCL if (setsockopt(sd, IPPROTO_IPV6, IPV6_HDRINCL, &on, sizeof(on))) { #else if (1) { /* kernel does not support IPV6_HDRINCL */ #endif zlog_err("setsockopt IPV6_HDRINCL error: %s", strerror(errno)); close(sd); return -1; }

and in this way, you remove the configure.ac change.

second change , in the topotest: use the API required_linux_kernel_version to avoid running BFD on a too old release.

so if I understand correctly, this setsockopt operation will fail:

if (setsockopt(sd, IPPROTO_IPV6, IPV6_HDRINCL, &on, sizeof(on)))

and sbfd test will fail. I don't know how the tests are distributed on the various linux distros. Assuming this is random, sooner or later, the sbfd test over redhat8 will fail.

if it is true, you should think of a protection: what about this code in the place you are using it:

#ifdef IPV6_HDRINCL if (setsockopt(sd, IPPROTO_IPV6, IPV6_HDRINCL, &on, sizeof(on))) { #else if (1) { /* kernel does not support IPV6_HDRINCL */ #endif zlog_err("setsockopt IPV6_HDRINCL error: %s", strerror(errno)); close(sd); return -1; }

and in this way, you remove the configure.ac change.

second change , in the topotest: use the API required_linux_kernel_version to avoid running BFD on a too old release.

seems a perfect solution. I will have a try :)

pguibert6WIND · 2025-01-20T09:35:03Z

one more useless code:

 /* BFD session flags */
 enum bfd_session_flags {
        BFD_SESS_FLAG_NONE = 0,
-       BFD_SESS_FLAG_ECHO = 1 << 0,    /* BFD Echo functionality */
-       BFD_SESS_FLAG_ECHO_ACTIVE = 1 << 1, /* BFD Echo Packets are being sent
+       BFD_SESS_FLAG_ECHO = 1 << 0,            /* BFD Echo functionality */
+       BFD_SESS_FLAG_ECHO_ACTIVE = 1 << 1,     /* BFD Echo Packets are being sent
                                             * actively
                                             */
-       BFD_SESS_FLAG_MH = 1 << 2,        /* BFD Multi-hop session */
-       BFD_SESS_FLAG_IPV6 = 1 << 4,    /* BFD IPv6 session */
+       BFD_SESS_FLAG_MH = 1 << 2,              /* BFD Multi-hop session */
+       BFD_SESS_FLAG_IPV6 = 1 << 4,            /* BFD IPv6 session */
        BFD_SESS_FLAG_SEND_EVT_ACTIVE = 1 << 5, /* send event timer active */
        BFD_SESS_FLAG_SEND_EVT_IGNORE = 1 << 6, /* ignore send event when timer
                                                 * expires
                                                 */
        BFD_SESS_FLAG_SHUTDOWN = 1 << 7,        /* disable BGP peer function */
-       BFD_SESS_FLAG_CONFIG = 1 << 8, /* Session configured with bfd NB API */
-       BFD_SESS_FLAG_CBIT = 1 << 9,   /* CBIT is set */
-       BFD_SESS_FLAG_PASSIVE = 1 << 10, /* Passive mode */
-       BFD_SESS_FLAG_MAC_SET = 1 << 11, /* MAC of peer known */
+       BFD_SESS_FLAG_CONFIG = 1 << 8,          /* Session configured with bfd NB API */
+       BFD_SESS_FLAG_CBIT = 1 << 9,            /* CBIT is set */
+       BFD_SESS_FLAG_PASSIVE = 1 << 10,        /* Passive mode */
+       BFD_SESS_FLAG_MAC_SET = 1 << 11         /* MAC of peer known */
+};

pguibert6WIND · 2025-01-20T09:38:34Z

where are the missing things list in doc/developer/sbfd.rst please ?
The below list is what I think I understood, which is missing.
Please correct me if I am wrong, and add it to the developer doc, pls.

admin of admin down at init side and route reflector side. (bit is not set in packet).
handle of ipv4 sbfd packets over SRv6
handle of ipv4 sbfd packets without srv6 seg list
sbfd over mpls

forrestchu · 2025-01-20T09:40:21Z

one more useless code:

 /* BFD session flags */
 enum bfd_session_flags {
        BFD_SESS_FLAG_NONE = 0,
-       BFD_SESS_FLAG_ECHO = 1 << 0,    /* BFD Echo functionality */
-       BFD_SESS_FLAG_ECHO_ACTIVE = 1 << 1, /* BFD Echo Packets are being sent
+       BFD_SESS_FLAG_ECHO = 1 << 0,            /* BFD Echo functionality */
+       BFD_SESS_FLAG_ECHO_ACTIVE = 1 << 1,     /* BFD Echo Packets are being sent
                                             * actively
                                             */
-       BFD_SESS_FLAG_MH = 1 << 2,        /* BFD Multi-hop session */
-       BFD_SESS_FLAG_IPV6 = 1 << 4,    /* BFD IPv6 session */
+       BFD_SESS_FLAG_MH = 1 << 2,              /* BFD Multi-hop session */
+       BFD_SESS_FLAG_IPV6 = 1 << 4,            /* BFD IPv6 session */
        BFD_SESS_FLAG_SEND_EVT_ACTIVE = 1 << 5, /* send event timer active */
        BFD_SESS_FLAG_SEND_EVT_IGNORE = 1 << 6, /* ignore send event when timer
                                                 * expires
                                                 */
        BFD_SESS_FLAG_SHUTDOWN = 1 << 7,        /* disable BGP peer function */
-       BFD_SESS_FLAG_CONFIG = 1 << 8, /* Session configured with bfd NB API */
-       BFD_SESS_FLAG_CBIT = 1 << 9,   /* CBIT is set */
-       BFD_SESS_FLAG_PASSIVE = 1 << 10, /* Passive mode */
-       BFD_SESS_FLAG_MAC_SET = 1 << 11, /* MAC of peer known */
+       BFD_SESS_FLAG_CONFIG = 1 << 8,          /* Session configured with bfd NB API */
+       BFD_SESS_FLAG_CBIT = 1 << 9,            /* CBIT is set */
+       BFD_SESS_FLAG_PASSIVE = 1 << 10,        /* Passive mode */
+       BFD_SESS_FLAG_MAC_SET = 1 << 11         /* MAC of peer known */
+};

seems an indentation issue.

pguibert6WIND · 2025-01-20T09:44:16Z

replace MAX_SEG_NUM with SRV6_MAX_SEGS which is already defined in lib/srv6.h

forrestchu · 2025-01-20T09:44:52Z

where are the missing things list in doc/developer/sbfd.rst please ? The below list is what I think I understood, which is missing. Please correct me if I am wrong, and add it to the developer doc, pls.

admin of admin down at init side and route reflector side. (bit is not set in packet).

handle of ipv4 sbfd packets over SRv6

handle of ipv4 sbfd packets without srv6 seg list

sbfd over mpls

*handle of ipv4 sbfd packets over SRv6 -> this is already supported.

*sbfd over mpls -> this is the missing one, will update it to doc :)

forrestchu · 2025-01-20T11:34:33Z

If I reset all the commits for this branch, I can see that zebra_ptm.h file is modified, whereas it should not:

root@ubuntu2204hwe:~/frr# git reset HEAD~24
--- a/zebra/zebra_ptm.h
+++ b/zebra/zebra_ptm.h
@@ -48,13 +48,13 @@ struct zebra_ptm_cb {
 #define ZEBRA_IF_PTM_ENABLE_ON     1
 #define ZEBRA_IF_PTM_ENABLE_UNSPEC 2
 
-#define IS_BFD_ENABLED_PROTOCOL(protocol)                                      \
-       ((protocol) == ZEBRA_ROUTE_BGP || (protocol) == ZEBRA_ROUTE_OSPF ||    \
-        (protocol) == ZEBRA_ROUTE_OSPF6 || (protocol) == ZEBRA_ROUTE_ISIS ||  \
-        (protocol) == ZEBRA_ROUTE_PIM ||                                      \
-        (protocol) == ZEBRA_ROUTE_OPENFABRIC ||                               \
+#define IS_BFD_ENABLED_PROTOCOL(protocol)                                                          \
+       ((protocol) == ZEBRA_ROUTE_BGP || (protocol) == ZEBRA_ROUTE_OSPF ||                        \
+        (protocol) == ZEBRA_ROUTE_OSPF6 || (protocol) == ZEBRA_ROUTE_ISIS ||                      \
+        (protocol) == ZEBRA_ROUTE_PIM || (protocol) == ZEBRA_ROUTE_OPENFABRIC ||                  \
         (protocol) == ZEBRA_ROUTE_STATIC || (protocol) == ZEBRA_ROUTE_RIP)
 
+

One of the last changes you should do is rework all the changes you made and split in consistent commits.

Thanks Philippe, I will rework all the changes after CI pass.

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 <wumu.zsl@alibaba-inc.com>

Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

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 <wumu.zsl@alibaba-inc.com>

refactor bfd_session_create and bfd_session_destroy to support SBFD Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

Two types of sbfd packets are supported: initiator packet and echo packet Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

1) create socket to send sbfd packets 2) integrate sbfd logic with existing BFD Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

pguibert6WIND · 2025-01-21T12:28:04Z

lib/bfd.c

@@ -742,6 +752,7 @@ void bfd_sess_install(struct bfd_session_params *bsp)
 	event_add_event(bsglobal.tm, _bfd_sess_send, bsp, 0, &bsp->installev);
 }

+


useless space

pguibert6WIND · 2025-01-21T12:28:17Z

lib/bfd.c

@@ -365,6 +374,7 @@ int zclient_bfd_command(struct zclient *zc, struct bfd_session_arg *args)
 	stream_putc(s, args->profilelen);
 	if (args->profilelen)
 		stream_put(s, args->profile, args->profilelen);
+


useless space

pguibert6WIND · 2025-01-21T16:12:57Z

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

Jafaral · 2025-01-21T17:05:02Z

Topotest need to be updated to use unified configs

github-actions bot added master size/XXL labels Nov 4, 2024

riw777 self-requested a review November 5, 2024 15:22

riw777 reviewed Nov 12, 2024

View reviewed changes

riw777 requested changes Nov 12, 2024

View reviewed changes

github-actions bot added the conflicts label Nov 26, 2024

forrestchu force-pushed the sbfd branch from d96c7fd to f133265 Compare November 27, 2024 03:47

github-actions bot removed the conflicts label Nov 27, 2024

forrestchu force-pushed the sbfd branch from f133265 to beb6c8c Compare November 29, 2024 03:32

forrestchu force-pushed the sbfd branch from beb6c8c to 8412137 Compare January 6, 2025 11:29

github-actions bot added the rebase PR needs rebase label Jan 6, 2025

forrestchu force-pushed the sbfd branch from 8412137 to 6851971 Compare January 6, 2025 12:14

forrestchu force-pushed the sbfd branch from 6851971 to 40e8079 Compare January 7, 2025 03:22

bfdd: add enum bfd_mode_type for different bfd types

32705fe

Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

forrestchu force-pushed the sbfd branch from 40e8079 to 185b7a6 Compare January 7, 2025 03:40

frrbot bot added the bfd label Jan 7, 2025

forrestchu force-pushed the sbfd branch from 185b7a6 to 8da3e49 Compare January 7, 2025 03:54

frrbot bot added documentation tests Topotests, make check, etc labels Jan 7, 2025

forrestchu force-pushed the sbfd branch 6 times, most recently from 5e0b3d4 to 95d9081 Compare January 7, 2025 09:02

pguibert6WIND reviewed Jan 20, 2025

View reviewed changes

forrestchu force-pushed the sbfd branch from 68a9774 to b61ffb2 Compare January 20, 2025 11:24

forrestchu added 11 commits January 21, 2025 02:43

bfdd: modify frr-bfdd.yang for sbfd echo and sbfd init

71da8cf

Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

bfdd: support sbfd session create and destroy

d41bc33

refactor bfd_session_create and bfd_session_destroy to support SBFD Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

bfdd: implement Xpath callbacks for SBFD

dbbb130

Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

bfdd: add sbfd config commands to bfdd cli

c445db2

Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

bfdd: adjust show commands for SBFD

9506646

Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

bfdd: create a hash table for local sbfd reflector info

edf941d

Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

bfdd: implement functions for sending sbfd pkts with SRv6 header

021a862

Two types of sbfd packets are supported: initiator packet and echo packet Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

bfdd: add sbfd state machine functions

4085d23

Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

bfdd: enable sbfd session

3b705bc

1) create socket to send sbfd packets 2) integrate sbfd logic with existing BFD Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

forrestchu force-pushed the sbfd branch from b61ffb2 to 0ce53bc Compare January 21, 2025 08:03

forrestchu added 3 commits January 21, 2025 08:23

bfdd: add bfdname info when notify bfd status to PTM

27c2d9c

Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

bfdd: add macro HAVE_IPV6_HDRINCL to solve build issue

bd5a63d

Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

tests: add basic topotest cases for sbfd Initiator and Reflector

bd675f4

Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

forrestchu force-pushed the sbfd branch from 0ce53bc to 7b0830c Compare January 21, 2025 08:27

doc: add sbfd doc

7d11353

Signed-off-by: wumu.zsl <wumu.zsl@alibaba-inc.com>

forrestchu force-pushed the sbfd branch from 7b0830c to 7d11353 Compare January 21, 2025 10:57

pguibert6WIND reviewed Jan 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement SBFD #17336

implement SBFD #17336

forrestchu commented Nov 4, 2024

riw777 left a comment

riw777 left a comment

donaldsharp commented Nov 12, 2024

github-actions bot commented Nov 26, 2024

riw777 commented Dec 3, 2024

forrestchu commented Dec 11, 2024

forrestchu commented Jan 6, 2025

pguibert6WIND Jan 20, 2025 •

edited

Loading

forrestchu Jan 20, 2025

pguibert6WIND Jan 20, 2025

forrestchu Jan 20, 2025

pguibert6WIND Jan 21, 2025

forrestchu Jan 21, 2025

pguibert6WIND Jan 21, 2025 •

edited

Loading

forrestchu Jan 21, 2025

pguibert6WIND commented Jan 20, 2025

pguibert6WIND commented Jan 20, 2025

forrestchu commented Jan 20, 2025

pguibert6WIND commented Jan 20, 2025

forrestchu commented Jan 20, 2025

forrestchu commented Jan 20, 2025

pguibert6WIND Jan 21, 2025

pguibert6WIND Jan 21, 2025

pguibert6WIND commented Jan 21, 2025

Jafaral commented Jan 21, 2025

		@@ -742,6 +752,7 @@ void bfd_sess_install(struct bfd_session_params *bsp)
		event_add_event(bsglobal.tm, _bfd_sess_send, bsp, 0, &bsp->installev);
		}

implement SBFD #17336

Are you sure you want to change the base?

implement SBFD #17336

Conversation

forrestchu commented Nov 4, 2024

riw777 left a comment

Choose a reason for hiding this comment

riw777 left a comment

Choose a reason for hiding this comment

donaldsharp commented Nov 12, 2024

github-actions bot commented Nov 26, 2024

riw777 commented Dec 3, 2024

forrestchu commented Dec 11, 2024

forrestchu commented Jan 6, 2025

pguibert6WIND Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pguibert6WIND Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pguibert6WIND commented Jan 20, 2025

pguibert6WIND commented Jan 20, 2025

forrestchu commented Jan 20, 2025

pguibert6WIND commented Jan 20, 2025

forrestchu commented Jan 20, 2025

forrestchu commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pguibert6WIND commented Jan 21, 2025

Jafaral commented Jan 21, 2025

pguibert6WIND Jan 20, 2025 •

edited

Loading

pguibert6WIND Jan 21, 2025 •

edited

Loading