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

Add "bgp bestpath peer-type multipath-relax" to FRR #5629

Merged
merged 5 commits into from
Apr 16, 2021

Conversation

jmmikkel
Copy link
Contributor

This new BGP configuration is akin to "bgp bestpath aspath multipath-relax".
When applied, paths learned from different peer types will be eligible
to be considered for multipath (ECMP). Paths from all of eBGP, iBGP, and
confederation peers may be included in a multipath group if they are
otherwise equal cost.

When such a multipath group is created, it is not desirable for
iBGP nexthops to be discarded from the FIB because they are not directly
connected. So when publishing the nexthop group to zebra, bgpd will allow
recursive resolution, but only when there are iBGP-learned paths in the
group.

Signed-off-by: Joanne Mikkelson jmmikkel@arista.com

- Why I did it

In a VoQ chassis, BGP routes learned on one ASIC Instance are advertised to other ASICs over iBGP. Normally such iBGP-learned routes cannot form an ECMP/multipath group with locally eBGP-learned routes because the BGP bestpath algorithm prefers eBGP paths over iBGP paths. To allow ECMP in this case, we need to introduce a new configuration to bgpd.

The VoQ chassis BGP HLD discusses the expected ECMP behavior for the chassis: sonic-net/SONiC#674

- How I did it

Added a new BGP configuration option to FRR allowing eBGP and iBGP/confed routes to form a multipath group if they are otherwise equal cost. Modified the bestpath code to honor the option. Once a such a group has been created, ensured that all the nexthops in the group are allowed to go through recursive resolution when written from RIB to FIB.

- How to verify it

Configure an FRR bgp instance with "bgp bestpath as-path multipath-relax" enabled. Advertise a route for the same prefix with the same length AS_PATH from both an eBGP and iBGP peer. For good measure, make the iBGP path's nexthop recursively resolve over a static route. Without the new config, check that only the eBGP path is marked best and the iBGP path is not multipath. Add the new "bgp bestpath peer-type multipath-relax" config and check that the iBGP path is now marked multipath (=). Check that no nexthop is marked "inactive" in "show ip route".

An swss virtual_chassis test for this new feature is forthcoming.

- Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006

- Description for the changelog

Add "bgp bestpath peer-type multipath-relax" to FRR

- A picture of a cute animal (not mandatory but encouraged)

This new BGP configuration is akin to "bgp bestpath aspath multipath-relax".
When applied, paths learned from different peer types will be eligible
to be considered for multipath (ECMP). Paths from all of eBGP, iBGP, and
confederation peers may be included in a multipath group if they are
otherwise equal cost.

When such a multipath group is created, it is not desirable for
iBGP nexthops to be discarded from the FIB because they are not directly
connected. So when publishing the nexthop group to zebra, bgpd will allow
recursive resolution, but only when there are iBGP-learned paths in the
group.

Signed-off-by: Joanne Mikkelson <jmmikkel@arista.com>
@pavel-shirshov
Copy link
Contributor

Hi May I suggest to open a PR in https://github.com/FRRouting/frr ?
We're moving to 7.4 So your patch is going to be outdated as soon as we move there.

@eswaranb
Copy link

Hi May I suggest to open a PR in https://github.com/FRRouting/frr ?
We're moving to 7.4 So your patch is going to be outdated as soon as we move there.

Hi Pavel,

Just to confirm - is the plan to use frr 7.4 for the 202012 release ?

We do intend to upstream this patch to frr, but it may be time consuming to patch it there and bring it downstream to sonic. We will rebase and create a patch based off 7.4 and then upstream it to the frr repo soon after.

Do you have any guidance on when the 7.4 change will merge into sonic-buildimage?

Thanks,
Eswaran

@pavel-shirshov
Copy link
Contributor

Hi @eswaranb

Yes. I'll try to merge FRR 7.4 to 202012 release. Currently it breaks one our test, but working on my dut well.
I'm going to work on it in a few days.

Let me know if I can help you to upstream the patch to FRR master.

Thanks

@jmmikkel
Copy link
Contributor Author

Pavel, it's been two months and I don't see 7.4 merged yet? Any news on 7.4, or can we move on with this patch?

Thanks

(renumbers new patch, no other changes)
@pavel-shirshov
Copy link
Contributor

@jmmikkel FRR 7.4 fails graceful restart bgp test for ipv6.
FRR 7.5 fails warm restart test (actually it fails whole sonic).
Working onf FRR 7,5 currently.

@jmmikkel
Copy link
Contributor Author

jmmikkel commented Jan 8, 2021

retest vs please

@arlakshm
Copy link
Contributor

@jmmikkel can you fix the conflicts ?

Add-bgp-bestpath-peer-type-multipath-relax.patch to match the current
version from FRR PR FRRouting/frr#8056
(minus the test).
@arlakshm
Copy link
Contributor

/Azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@arlakshm
Copy link
Contributor

This change is merged in FRR in this PR FRRouting/frr#8056

@arlakshm arlakshm merged commit e1bee85 into sonic-net:master Apr 16, 2021
raphaelt-nvidia pushed a commit to raphaelt-nvidia/sonic-buildimage that referenced this pull request May 23, 2021
* Add "bgp bestpath peer-type multipath-relax" to frr

This new BGP configuration is akin to "bgp bestpath aspath multipath-relax".
When applied, paths learned from different peer types will be eligible
to be considered for multipath (ECMP). Paths from all of eBGP, iBGP, and
confederation peers may be included in a multipath group if they are
otherwise equal cost.

When such a multipath group is created, it is not desirable for
iBGP nexthops to be discarded from the FIB because they are not directly
connected. So when publishing the nexthop group to zebra, bgpd will allow
recursive resolution, but only when there are iBGP-learned paths in the
group.

This change is merged in FRR in this PR FRRouting/frr#8056

Signed-off-by: Joanne Mikkelson <jmmikkel@arista.com>
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
* Add "bgp bestpath peer-type multipath-relax" to frr

This new BGP configuration is akin to "bgp bestpath aspath multipath-relax".
When applied, paths learned from different peer types will be eligible
to be considered for multipath (ECMP). Paths from all of eBGP, iBGP, and
confederation peers may be included in a multipath group if they are
otherwise equal cost.

When such a multipath group is created, it is not desirable for
iBGP nexthops to be discarded from the FIB because they are not directly
connected. So when publishing the nexthop group to zebra, bgpd will allow
recursive resolution, but only when there are iBGP-learned paths in the
group.

This change is merged in FRR in this PR FRRouting/frr#8056

Signed-off-by: Joanne Mikkelson <jmmikkel@arista.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Chassis 🤖 Modular chassis support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants