Add BFD support #967

fedepaol · 2021-09-28T17:16:52Z

This PR adds BFD support on top of BGP.

It will be possible to enable BFD for a given BGP peer, following the configuration described in the design proposal: https://github.com/metallb/metallb/blob/main/design/bgp-bfd.md

liornoy · 2021-11-06T13:29:52Z

internal/bgp/frr/config.go

+    echo-mode
+    {{end -}}
+    {{ if .EchoInterval -}}
+    echo-interval {{.EchoInterval}}


I don't think the echo interval of the BFD in frr is configured this way.
In their docs it is splits to:
echo receive-interval <disabled|(10-60000)>
echo transmit-interval (10-60000)

That part changed between 7.5 (which we are pinning now) and latest.

maybe I am missing something in the config there should be a matching peer section in the bfd config ? the above configure only the profile ? https://docs.frrouting.org/en/latest/bfd.html?highlight=bfd#configuration

You can associate a neighbor to a bfd profile, which is what happens in https://github.com/metallb/metallb/pull/967/files#diff-1d9a33c302812585b1b2d1897f84126492129d1f9415786a73dcfa26603686c9R51

Documented here https://docs.frrouting.org/en/latest/bfd.html#bgp-bfd-configuration

do u know how this will work with view bgp do u have one bfd process for every bgp instance or its shared and in that case u pick peers from both ?

this isn't neighbour I was asking about what I am asking about is the following

bfd peer 192.168.0.1 label home-peer no shutdown ! !

bgp config will have what u showed

router bgp 65530 neighbor 192.168.0.1 remote-as 65531 neighbor 192.168.0.1 bfd profile blah !

We are not configuring bfd peers explicitly because we are leveraging the bgp's bfd option described here https://docs.frrouting.org/en/latest/bfd.html#bgp-bfd-configuration

If you have a bfd profile defined, it's totally fine to add the option to the neighbour and that neighbor will be backed up by a bfd session. So the configuration is more compact without the need of adding the peer explicitly.
This is what we do here 694ed34#diff-1d9a33c302812585b1b2d1897f84126492129d1f9415786a73dcfa26603686c9R51

do u know how this will work with view bgp do u have one bfd process for every bgp instance or its shared and in that case u pick peers from both ?

I don't know, I guess we'll figure it out if / when we re-enable views.

oribon · 2021-11-10T08:40:53Z

internal/bgp/frr/config.go

+type BFDProfile struct {
+	Name             string
+	ReceiveInterval  *uint32
+	TransmitInterval *uint32
+	DetectMultiplier *uint32
+	EchoInterval     *uint32
+	EchoMode         bool
+	PassiveMode      bool
+	MinimumTTL       *uint32
+}


why is this declared both here and in internal/config (I assume it's intentional because of configBFDProfileToFRR)?

To enforce domain and module separation.
One is the data structure we read from the configuration, the other one is the one we pass to the template. They serve different purpouses and I did not want the config data structures to leak into the frr package.

oribon · 2021-11-10T09:14:53Z

speaker/bgp_controller.go

+	if c.bgpType != bgpFrr && len(profiles) > 0 {
+		return errors.New("bfd Profiles not supported in native mode")
+	}


why is this necessary? the native mode will return that error anyways?

Good catch, it's a leftover of the previous check. Will remove it.

oribon · 2021-11-10T09:41:23Z

internal/config/config_test.go

+`,
+		},
+		{
+			desc: "Peer with duplicated BFD Profile",


This should be duplicate BFD Profile name?

oribon · 2021-11-10T09:46:40Z

internal/config/config_test.go

+		},
+		{
+			desc: "BFD Profile with too high range receive interval",
+			raw: `


this is the same as the one above? + probably you should add a test for the other parameters that are validated.

Yes, need to change. I was torn about testing every single parameter, thought it was significant enough (and less boring) to validate the range function against only one of them, but I can add more tests if you think it's necessary.

oribon · 2021-11-10T10:00:09Z

e2etest/bgp_test.go

 			var peers []peer
 			var peerAddrs []string
-			for _, c := range frrContainers {
+			for i, c := range frrContainers {


General comment about the commit: Filling ... in the external containers but using them as configuration of speakers is misleading. - that's misleading because it wasn't used here correctly 😅
From the containerConfig's (which represents the external FRR container) perspective, configuration for the speaker side should be under NeighborConfig (and not RouterConfig). That's because RouterConfig is how it should configure itself and NeighborConfig is the way the speaker should present itself to the container.

I think we should preserve that and not declare any parameters somewhere else.

I think if we call the structure FRR container, a reader expects to find parameters affecting the container only.
We are currently leveraging some of those to set up the metallb config because they are symmetric (i.e. the password, the ASNs). Some other belong to metallb only and it does not make sense to have them here unless we think about a wider wrapper.

oribon · 2021-11-10T10:12:27Z

e2etest/bgp_test.go

+					container.NeighborConfig.BFDEnabled = true
+				})
+			}
+


No need to call validateFRRPeeredWithNodes for BFD?

I think I forgot about it

oribon · 2021-11-10T10:15:25Z

e2etest/bgp_test.go

+			}, 4*time.Minute, 1*time.Second).Should(BeNil())
+
+		},
+			table.FEntry("default",


oribon · 2021-11-14T10:51:55Z

lgtm

cgoncalves · 2021-11-15T12:39:57Z

e2etest/config/frr/daemons

 sharpd=no
 pbrd=no
-bfdd=no
+bfdd=yes


Would it make sense to also enable the BFD daemon in dev-env/bgp/frr/daemons? Having it disabled means one would need to manually toggle this setting, restart the external FRR container and resume with the desired BFD configuration. This is only for testing deployments (dev-env) so impact is low to none.

It won't hurt I guess.

Here we enable an optional BFD profile that can be associated with a BGP session. Using BFD in conjunction with BGP allows faster failover detection. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Filling routerid / holdtime in the external containers but using them as configuration of speakers is misleading. Here we decouple it. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

msherif1234 · 2021-11-10T14:55:36Z

internal/bgp/frr/config.go

+	EchoMode         bool
+	PassiveMode      bool
+	MinimumTTL       *uint32
+}


shutdown option ?

Not sure what the question is, if you are asking why I did not add the shutdown option which says:
Enables or disables the peer. When the peer is disabled an ‘administrative down’ message is sent to the remote peer.

I think that goes against the metallb api. If a user puts a peer in the config, it's because he wants to enable it. If he wants to disable, he'll just remove the peer entry.

this is shuting down the bfd peer I am assume its used for diagnosticsnothing against metallb api

I am not sure I understood the question then. Why would we need to add Shutdown here?

its one of the BFD config option in the FRR

But does it matter if we don't use it?

functionality wise probably not but was just curious why u drop it to begin with

I did not add it because we don't need it, as there's no need to add stuff we won't be using.

msherif1234 · 2021-11-10T14:57:19Z

internal/bgp/frr/config.go

+    echo-mode
+    {{end -}}
+    {{ if .EchoInterval -}}
+    echo-interval {{.EchoInterval}}


do u know how this will work with view bgp do u have one bfd process for every bgp instance or its shared and in that case u pick peers from both ?

msherif1234 · 2021-11-10T22:54:28Z

internal/bgp/frr/config.go

+    echo-mode
+    {{end -}}
+    {{ if .EchoInterval -}}
+    echo-interval {{.EchoInterval}}


this isn't neighbour I was asking about what I am asking about is the following

bfd peer 192.168.0.1 label home-peer no shutdown ! !

bgp config will have what u showed

router bgp 65530 neighbor 192.168.0.1 remote-as 65531 neighbor 192.168.0.1 bfd profile blah !

msherif1234 · 2021-11-12T20:47:57Z

internal/config/config.go

+	res := &BFDProfile{}
+	res.Name = p.Name
+	var err error
+	res.DetectMultiplier, err = bfdIntFromConfig(p.DetectMultiplier, 2, 255)


you need to add 4th arg to bfdIntFromConfig to have the default settings if its not specified, pls refer to https://docs.frrouting.org/en/latest/bfd.html#clicmd-bfd to find the appropriate default value for each config options

Wouldn't frr apply the default settings itself if not specified?
I fix defaults here, metallb will have to work with a fixed frr version which has 1:1 mapping to those parameters. If we keep it like this, we won't add the parameters that are not specified, making the config rendering more compatible.

msherif1234 · 2021-11-15T14:14:03Z

e2etest/pkg/frr/bfd.go

+		return nil, errors.Wrapf(err, "Failed to parse neighbours %s", res)
+	}
+	return peers, nil
+}


so the CI as it stands it make sure BFD sessions are created , how about the failure detection part how does that been verified/tested, do we know how long it takes for BFD enabled session to detect a failure ? any plans to cover this in CI or at least manual testing for now and doc it ?

Failover tests are in the backlog. We'd need to carefully think about them, because we are still subject to the time it takes ot execute the commands. Maybe setting a ludicrous hold time on the bgp side and to check that the failover happens in a lower time would work.
I'd keep this on the functional side though.

msherif1234 · 2021-11-15T17:41:10Z

speaker/main.go

 		return k8s.SyncStateErrorNoRetry
 	}

+	if c.bgpType == bgpNative {


bgpNative ?!? u meant bgpFrr ?

Nope. The check here is, if native check if something frr only is applied

oribon

Other than these small comments, LGTM

oribon · 2021-11-16T12:28:23Z

e2etest/bgp_test.go

 			for _, item := range speakers.Items {
-				i := item
-				speakerPods = append(speakerPods, &i)
+				speakerPods = append(speakerPods, &item)


accidental change?

yep fixing it

oribon · 2021-11-16T12:31:32Z

e2etest/bgp_test.go

 		}
+
 		for _, c := range frrContainers {
 			pairExternalFRRWithNodes(cs, c)


I think we should have this in the loop above like you did here:

metallb/e2etest/bgp_test.go

Line 452 in d062214

pairExternalFRRWithNodes(cs, c, func(container *frrcontainer.FRR) {

)

Yes did not notice while rebasing

oribon · 2021-11-16T12:33:42Z

e2etest/bgp_test.go

+			configData := configFile{
+				Pools: []addressPool{
+					{
+						Name:     "bgp-test",


would be better to rename this something like bfd-test (maybe with -bfd.Name suffix)?

Extend e2e tests to make sure bfd pairing works with the external containers. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

BFD is FRR only so we won't cover it. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

oribon · 2021-11-16T13:14:14Z

LGTM, the layer2 fail from the ci isn't related

* Enable BGP + BFD support Here we enable an optional BFD profile that can be associated with a BGP session. Using BFD in conjunction with BGP allows faster failover detection. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com> * Decouple the configuration structure of external containers to speakers Filling routerid / holdtime in the external containers but using them as configuration of speakers is misleading. Here we decouple it. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com> * E2ETests: cover BFD Extend e2e tests to make sure bfd pairing works with the external containers. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com> * Skip BFD tests in CI in legacy lanes BFD is FRR only so we won't cover it. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

fedepaol requested review from daxmc99, johananl, rata and russellb as code owners September 28, 2021 17:16

fedepaol force-pushed the bfd branch from b96ed6d to e3cfe88 Compare October 8, 2021 09:23

fedepaol force-pushed the bfd branch 5 times, most recently from be53371 to b69f399 Compare October 20, 2021 09:21

fedepaol force-pushed the bfd branch 9 times, most recently from 74328ef to 7985cda Compare October 29, 2021 09:38

fedepaol force-pushed the bfd branch from 7985cda to b27022e Compare November 5, 2021 17:09

fedepaol changed the title ~~WIP / POC: add BFD support~~ Add BFD support Nov 5, 2021

fedepaol force-pushed the bfd branch from b27022e to 04938d3 Compare November 5, 2021 21:30

liornoy reviewed Nov 6, 2021

View reviewed changes

fedepaol force-pushed the bfd branch 4 times, most recently from 2f43fd7 to 8777ad7 Compare November 9, 2021 13:47

oribon reviewed Nov 10, 2021

View reviewed changes

fedepaol force-pushed the bfd branch 2 times, most recently from c805231 to 66b63f6 Compare November 11, 2021 13:17

fedepaol force-pushed the bfd branch 7 times, most recently from 9de0428 to 374083c Compare November 13, 2021 09:37

fedepaol force-pushed the bfd branch 5 times, most recently from 71233de to ac68518 Compare November 15, 2021 11:34

cgoncalves reviewed Nov 15, 2021

View reviewed changes

fedepaol added 2 commits November 15, 2021 14:09

Enable BGP + BFD support

694ed34

Here we enable an optional BFD profile that can be associated with a BGP session. Using BFD in conjunction with BGP allows faster failover detection. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Decouple the configuration structure of external containers to speakers

8c8196f

Filling routerid / holdtime in the external containers but using them as configuration of speakers is misleading. Here we decouple it. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

fedepaol force-pushed the bfd branch from ac68518 to 6b27f5d Compare November 15, 2021 13:09

msherif1234 reviewed Nov 15, 2021

View reviewed changes

fedepaol force-pushed the bfd branch from 6b27f5d to 22e43eb Compare November 15, 2021 17:19

msherif1234 reviewed Nov 15, 2021

View reviewed changes

fedepaol force-pushed the bfd branch 2 times, most recently from e0a32e6 to d062214 Compare November 16, 2021 10:58

oribon reviewed Nov 16, 2021

View reviewed changes

fedepaol added 2 commits November 16, 2021 13:46

E2ETests: cover BFD

865971c

Extend e2e tests to make sure bfd pairing works with the external containers. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Skip BFD tests in CI in legacy lanes

e139204

BFD is FRR only so we won't cover it. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

fedepaol force-pushed the bfd branch from d062214 to e139204 Compare November 16, 2021 12:46

fedepaol merged commit de8d7b7 into metallb:main Nov 16, 2021

Add BFD support #967

Add BFD support #967

Uh oh!

Conversation

fedepaol commented Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msherif1234 Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oribon commented Nov 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

fedepaol commented Sep 28, 2021 •

edited

Loading

msherif1234 Nov 10, 2021 •

edited

Loading

fedepaol Nov 16, 2021 •

edited

Loading