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 <[email protected]>

sbfd will use bfdname for key hash Signed-off-by: wumu.zsl <[email protected]>

Signed-off-by: wumu.zsl <[email protected]>

bfd_session_create support both BFD and SBFD, add some validation check for existing bfd Xpath callbacks Signed-off-by: wumu.zsl <[email protected]>

Signed-off-by: wumu.zsl <[email protected]>

Two types of sbfd packets are supported: initiator packet and echo packet Signed-off-by: wumu.zsl <[email protected]>

Signed-off-by: wumu.zsl <[email protected]>

create related sockets and start timers to send sbfd packets Signed-off-by: wumu.zsl <[email protected]>

pguibert6WIND · 2025-01-09T16:01:59Z

bfdd/bfdd_cli.c

+
+#define IPV4_ADDRESS 4
+#define IPV6_ADDRESS 6
+#define INVALID_IP   0 



lib/prefix.h

will use enum ipaddr_type_t instead of these macros

pguibert6WIND · 2025-01-09T16:03:27Z

bfdd/bfdd_cli.c

 static bool
 bfd_cli_is_profile(struct vty *vty)
 {
 	return strstr(VTY_CURR_XPATH, "/bfd/profile") != NULL;
 }

+static int determine_ip_version(const char *ip) 
+{
+    struct in_addr inaddr4; 


better to use str2ipaddr() instead of determine_ip_version()

pguibert6WIND · 2025-01-09T16:12:56Z

bfdd/bfdd_cli.c

+	"no peer <A.B.C.D|X:X::X:X> bfd-mode sbfd-init bfd-name BFDNAME$bfdname \
+	local-address <A.B.C.D|X:X::X:X> \
+	remote-discr (0-4294967295)$discr [{encap-type ENCAP_TYPE$encap_type encap-data X:X::X:X source-ipv6 X:X::X:X|vrf NAME}]",
+	NO_STR


is it possible to avoid mismatch with encap_type and defining an enum with one word: srv6 ?
later will be possible to add mpls..

pguibert6WIND · 2025-01-09T16:30:29Z

bfdd/bfdd_cli.c

+	{
+        if (end_discr <= start_discr)
+		{
+			vty_out(vty, "input refector discriminator is illegal.\n");


typo: reflector

pguibert6WIND · 2025-01-09T16:30:38Z

bfdd/bfdd_cli.c

+		sr = sbfd_discr_lookup(start_discr);
+		if (!sr)
+		{
+			vty_out(vty, "input refector discriminator does not exist.\n");


typo: reflector

pguibert6WIND · 2025-01-09T16:41:37Z

bfdd/bfdd_vty.c

+		vty_out(vty, "\t\t\tReceive interval: -\n");
+		vty_out(vty, "\t\t\tTransmission interval: -\n");
+		vty_out(vty, "\t\t\tEcho receive interval: -\n");
+	}


If it is filled in json format, it is best to display in non json format too.
for consistency.

pguibert6WIND · 2025-01-09T16:44:13Z

bfdd/bfdd_vty.c

@@ -397,6 +562,7 @@ static struct json_object *__display_peer_counters_json(struct bfd_session *bs)
 	json_object_int_add(jo, "session-up", bs->stats.session_up);
 	json_object_int_add(jo, "session-down", bs->stats.session_down);
 	json_object_int_add(jo, "zebra-notifications", bs->stats.znotification);
+	json_object_int_add(jo, "tx-fail-packet", bs->stats.tx_fail_pkt);



if stats are only for SBFD, display the json or the value only if it is a SBFD session.

pguibert6WIND · 2025-01-09T16:46:48Z

bfdd/bfdd_vty.c

@@ -631,6 +927,50 @@ void _display_rtt(uint32_t *min, uint32_t *avg, uint32_t *max,
 /*
 * Show commands.
 */
+DEFPY(bfd_show_by_bfdname, bfd_show_by_bfdname_cmd,
+      "show bfd [vrf NAME$vrf_name] bfd-name BFDNAME$bfdname [json]",
+      SHOW_STR


as bfdname is only for BFD, what about redefining the command to : show sbfd ?
there is already the 'show sbfd reflector' command.

pguibert6WIND · 2025-01-09T16:47:07Z

bfdd/bfdd_vty.c

+      JSON_STR)
+{
+	_display_bfd_counters_by_bfdname(vty, vrf_name, bfdname, use_json(argc, argv));
+


ditto : show sbfd counters?

pguibert6WIND · 2025-01-09T16:47:19Z

bfdd/bfdd_vty.c

+{
+    _clear_bfd_counters_by_bfdname(vrfname, bfdname);
+
+	return CMD_SUCCESS;


ditto: show sbfd counters?

pguibert6WIND · 2025-01-09T16:57:16Z

bfdd/bfd_packet.c

+{
+    srv6h->nexthdr = IPPROTO_IPV6;
+    srv6h->hdrlen = GET_RTH_HDR_LEN(RTH_BASE_HEADER_LEN + sizeof(struct in6_addr)*seg_num);
+    srv6h->type = 4; // IPV6_SRCRT_TYPE_4


Can you try to include "linux/ipv6.h" ? and try using IPV6_SRCRT_TYPE_4 ?

The best would be to add ipv6.h in the include/linux headers of frr repository.

pguibert6WIND · 2025-01-09T17:00:36Z

bfdd/bfd_packet.c

+	//caller should make sure: seg_num > 1
+    srv6h->nexthdr = IPPROTO_IPV6;
+    srv6h->hdrlen = GET_RTH_HDR_LEN(RTH_BASE_HEADER_LEN + sizeof(struct in6_addr)*(seg_num - 1));
+    srv6h->type = 4; // IPV6_SRCRT_TYPE_4


use IPV6_SRCRT_TYPE_4.

pguibert6WIND · 2025-01-09T17:10:56Z

yang/frr-bfdd.yang

+          }
+
+          leaf segment-list {
+            type inet:ip-address;


this will not be possible if we have a multiple segment list.
is it possible to define a leaf-list with a size of 1 for now ?
this will avoid breaking the yang model when moving to multiple sids.

pguibert6WIND · 2025-01-09T17:16:23Z

bfdd/bfd_packet.c

+	int i;
+	for(i = 0;i < seg_num;i++)
+	{
+		memcpy(&srv6h->segments[i], &segment_list[seg_num-1-i], sizeof(struct in6_addr));


I am not at ease with the prototype definition of bp_sbfd_encap_srh_rth.
segment_list should be a list of addresses, and here, there is only one pointer to an address.

pguibert6WIND · 2025-01-10T09:15:29Z

bfdd/bfd.h

@@ -212,6 +221,8 @@ enum bfd_diagnosticis {
 	BD_ADMIN_DOWN = 7,
 	/* Reverse Concatenated Path Down. */
 	BD_REVCONCATPATH_DOWN = 8,
+	/* Sbfd Detect Function Failed. */
+	BD_SBFD_DETECT_FAILED = 9,
 	/* 9..31: reserved. */


add a comment : RFC6428 to inform the user that this value is defined by this RFC.

pguibert6WIND · 2025-01-10T09:15:58Z

bfdd/bfdd.c

@@ -150,6 +150,7 @@ const struct bfd_diag_str_list diag_list[] = {
 	{.str = "concatenated-path-down", .type = BD_CONCATPATH_DOWN},
 	{.str = "administratively-down", .type = BD_ADMIN_DOWN},
 	{.str = "reverse-concat-path-down", .type = BD_REVCONCATPATH_DOWN},
+	{.str = "sbfd-detect-failed", .type = BD_SBFD_DETECT_FAILED},
 	{.str = NULL},


ditto: rfc6428

pguibert6WIND · 2025-01-10T09:19:06Z

bfdd/bfd.h

@@ -27,9 +28,11 @@
 #endif

 #ifndef MAXNAMELEN
-#define MAXNAMELEN 32
+#define MAXNAMELEN 128
 #endif


can't we keep 32 ? what is the limit on the configuration perspective?

pguibert6WIND · 2025-01-10T09:26:13Z

bfdd/bfd.h

@@ -233,6 +244,7 @@ enum bfd_session_flags {
 	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_REM_ADMIN_DOWN = 1 << 13, /* remote notify admindown */


nowhere in the code this value is set. is it really used?

pguibert6WIND · 2025-01-10T09:27:49Z

bfdd/bfd.h


+#define BFD_SBFD_INITIATOR_DEMAND 1
+#define BFD_IPV6_UDP_DISABLE_CHECKSUM 1
+#define UDP_NO_CHECK6_RX 102


unused define. please remove.

pguibert6WIND · 2025-01-10T09:28:16Z

bfdd/bfd.h


+#define BFD_SBFD_INITIATOR_DEMAND 1
+#define BFD_IPV6_UDP_DISABLE_CHECKSUM 1


unused define. please remove.

pguibert6WIND · 2025-01-10T09:43:53Z

bfdd/bfd.h

@@ -154,6 +154,7 @@ struct bfd_echo_pkt {
 	uint64_t time_sent_usec;
 };

+#define BFD_XMTDEL_DELAY_TIMER               5


useless define. remove it.

pguibert6WIND · 2025-01-10T09:51:53Z

lib/bfd.c

+
+	if (bfd_name[0])
+	{
+		hook_call(sbfd_state_change_hook, bfd_name, state);


I found out some code https://github.com/eddieruan-alibaba/sonic-frr/blob/project-phoenixwing/pathd/path_cli.c#L852, that implements BFD in pathd. is this piece of code part of an other pull request? Is there a HLD available somewhere?

pguibert6WIND · 2025-01-10T09:53:32Z

lib/bfd.h


 #define BFD_PROFILE_NAME_LEN 64

+#define BFD_NAME_SIZE 255


is there a relationship with MAXNAMELEN ?
should not it be set to the same value?

pguibert6WIND · 2025-01-10T10:01:26Z

bfdd/bfd_packet.c

@@ -1876,6 +1874,7 @@ int _ptm_sbfd_init_send(struct bfd_session *bfd, const void *data, size_t datale
 	local = bfd->key.local;
 	peer  = bfd->key.peer;

+    /*SBFD Control pkt dst port should be 7784, src port can be any but NOT 7784 according to RFC7781 */


use te correct RFC: 7881

pguibert6WIND · 2025-01-10T10:01:42Z

bfdd/bfd_packet.c

@@ -1921,6 +1920,7 @@ int _ptm_sbfd_echo_send(struct bfd_session *bfd, const void *data, size_t datale

    local = bfd->key.local;
 	peer  = bfd->key.peer;
+	/*SBFD echo pkt dst port should use BFD Echo port 3785, src port can be any according to RFC7781*/


ditto: correct RFC 7881

pguibert6WIND · 2025-01-10T10:09:49Z

some doc warnings to be fixed too.

/root/frr/doc/user/sbfd.rst:204: WARNING: duplicate clicmd description of sbfd reflector source-address 200::D discriminator 456, other instance in sbfd
/root/frr/doc/user/sbfd.rst:239: ERROR: Unexpected indentation.
/root/frr/doc/user/sbfd.rst:244: WARNING: Block quote ends without a blank line; unexpected unindent.
/root/frr/doc/user/sbfd.rst:293: ERROR: Unexpected indentation.
/root/frr/doc/user/sbfd.rst:298: WARNING: Block quote ends without a blank line; unexpected unindent.
/root/frr/doc/user/sbfd.rst: WARNING: document isn't included in any toctree

pguibert6WIND · 2025-01-10T10:10:54Z

tests/topotests/sbfd_topo1/test_sbfd_topo1.py

+# Part of NetDEF Topology Tests
+#
+# Copyright (c) 2017 by
+# Network Device Education Foundation, Inc. ("NetDEF")


you can also leave the alibaba copyright.

pguibert6WIND · 2025-01-10T10:23:00Z

bfdd/bfdd_nb_config.c

@@ -1237,7 +1238,7 @@ int bfdd_bfd_sessions_srte_sbfd_source_ipv6_destroy(
 */
 int bfdd_bfd_sessions_srte_sbfd_init_create(struct nb_cb_create_args *args)
 {
-	return bfd_session_create(args, false, BFD_MODE_TYPE_SBFD_INIT);
+	return bfd_session_create(args, true, BFD_MODE_TYPE_SBFD_INIT);


It is not clear if for interop we should leave single hop or multihop .
On cisco, I read that single hop sbfd was used. so I am a bit puzzled!

I would not like that later, we realise that we need to add both single-hop and multi-hop options.
do you have any inputs ?

pguibert6WIND · 2025-01-10T10:27:42Z

tests/topotests/sbfd_topo1/test_sbfd_topo1.py

+    # up interface
+    r2.cmd("vtysh -c 'config t' -c 'interface r2-eth0' -c 'no shutdown'")
+    logger.info('waiting 5 sec ... for sbfd up after no shutdown')
+    time.sleep(5)


test sleep must be replaced with finer test functions, something like below.

test_func = functools.partial( check_sbfd_up, r1, 'up', type='sbfd initiator' ) success, _ = topotest.run_and_expect(test_func, None, count=5, wait=1)

pguibert6WIND · 2025-01-10T12:49:46Z

doc/user/sbfd.rst

+
+implementation
+===============
+


impleme part should be moved to developer folder

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

forrestchu added 12 commits January 7, 2025 03:31

bfdd: add enum bfd_mode_type for different bfd types

32705fe

Signed-off-by: wumu.zsl <[email protected]>

bfdd: add bfdname in bfd_key

6648243

sbfd will use bfdname for key hash Signed-off-by: wumu.zsl <[email protected]>

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

3796741

Signed-off-by: wumu.zsl <[email protected]>

bfdd: add vtysh cmd for sbfd

b4d2550

Signed-off-by: wumu.zsl <[email protected]>

bfdd: implement sbfd Xpath callbacks

14cebb4

Signed-off-by: wumu.zsl <[email protected]>

bfdd: refactor bfd_session_create

7d06317

bfd_session_create support both BFD and SBFD, add some validation check for existing bfd Xpath callbacks Signed-off-by: wumu.zsl <[email protected]>

bfdd: add sbfd cmds to bfdd cli

f7e761c

Signed-off-by: wumu.zsl <[email protected]>

bfdd: add show sbfd commands

074710b

Signed-off-by: wumu.zsl <[email protected]>

bfdd: implement functions for sending sbfd pkts with SRv6 header

2bda257

Two types of sbfd packets are supported: initiator packet and echo packet Signed-off-by: wumu.zsl <[email protected]>

bfdd: add sbfd state machine functions

1c46d9b

Signed-off-by: wumu.zsl <[email protected]>

bfdd: create a hash table for local sbfd reflector info

f055db1

Signed-off-by: wumu.zsl <[email protected]>

bfdd: enable sbfd session

23a05ed

create related sockets and start timers to send sbfd packets Signed-off-by: wumu.zsl <[email protected]>

pguibert6WIND reviewed Jan 9, 2025

View reviewed changes

bfdd/bfdd_cli.c

{

if (end_discr <= start_discr)

{

vty_out(vty, "input refector discriminator is illegal.\n");

Copy link

Member

pguibert6WIND Jan 9, 2025

Choose a reason for hiding this comment

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

typo: reflector

pguibert6WIND reviewed Jan 9, 2025

View reviewed changes

bfdd/bfdd_vty.c

{

_clear_bfd_counters_by_bfdname(vrfname, bfdname);

return CMD_SUCCESS;

Copy link

Member

pguibert6WIND Jan 9, 2025

Choose a reason for hiding this comment

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

ditto: show sbfd counters?

pguibert6WIND reviewed Jan 9, 2025

View reviewed changes

pguibert6WIND reviewed Jan 10, 2025

View reviewed changes

bfdd/bfd.h

#define BFD_SBFD_INITIATOR_DEMAND 1

#define BFD_IPV6_UDP_DISABLE_CHECKSUM 1

Copy link

Member

pguibert6WIND Jan 10, 2025

Choose a reason for hiding this comment

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

unused define. please remove.

pguibert6WIND reviewed Jan 10, 2025

View reviewed changes

doc/user/sbfd.rst

implementation

===============

Copy link

Member

pguibert6WIND Jan 10, 2025

Choose a reason for hiding this comment

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

impleme part should be moved to developer folder


		#define BFD_SBFD_INITIATOR_DEMAND 1
		#define BFD_IPV6_UDP_DISABLE_CHECKSUM 1

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

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

Choose a reason for hiding this comment

pguibert6WIND Jan 9, 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

pguibert6WIND Jan 9, 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

pguibert6WIND Jan 10, 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

Choose a reason for hiding this comment

pguibert6WIND Jan 10, 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

pguibert6WIND commented Jan 10, 2025

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 9, 2025 •

edited

Loading

pguibert6WIND Jan 9, 2025 •

edited

Loading

pguibert6WIND Jan 10, 2025 •

edited

Loading

pguibert6WIND Jan 10, 2025 •

edited

Loading