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

Implement bgp bandwidth auto for SR OS #1214

Merged
merged 5 commits into from
Jun 9, 2024

Conversation

jbemmel
Copy link
Collaborator

@jbemmel jbemmel commented Jun 7, 2024

  • Enable extended communities for EBGP inside the plugin, for all platforms
  • Also adds MED and localpref

* Enable extended communities for EBGP inside the plugin, for all platforms
@jbemmel jbemmel force-pushed the sros-add-bgp-bandwidth branch from bed0672 to f080c04 Compare June 7, 2024 02:05
@ipspace
Copy link
Owner

ipspace commented Jun 8, 2024

The 'add communities to EBGP' code does nothing as you modify a local variable but not store it. Anyway, we need to enable extended communities for EBGP only if the device is able to set Link Bandwidth in outgoing EBGP updates (for the moment, only EOS and FRR/CL).

Also, it looks like the changes to SR Linux / SR OS templates are not included in this pull request.

jbemmel added 2 commits June 8, 2024 12:46
* Enable extended communities only for 'out'

Note that there are different types of 'in': Accept incoming communities from an EBGP neighbor, or add a community locally
The latter might be called 'local' to distinguish
@jbemmel
Copy link
Collaborator Author

jbemmel commented Jun 8, 2024

The 'add communities to EBGP' code does nothing as you modify a local variable but not store it.

'get' returns a reference, I verified that the topology output contains the 'extended' community for EBGP. You are correct that the code assumes bgp.communities is already defined; if it is not, it would be modifying a local variable

Anyway, we need to enable extended communities for EBGP only if the device is able to set Link Bandwidth in outgoing EBGP updates (for the moment, only EOS and FRR/CL).

Updated, SR OS does support outgoing updates. There is a missing 3rd type: Locally add a community to incoming routes

Also, it looks like the changes to SR Linux / SR OS templates are not included in this pull request.

Added now

@jbemmel jbemmel marked this pull request as draft June 8, 2024 13:13
@jbemmel jbemmel marked this pull request as ready for review June 8, 2024 13:22
@ipspace
Copy link
Owner

ipspace commented Jun 9, 2024

Updated, SR OS does support outgoing updates. There is a missing 3rd type: Locally add a community to incoming routes

That's what bgp.bandwidth.in: auto (or just bgp.bandwidth: auto) is supposed to be doing, or did you have something else in mind?

Added now

Thank you, merging.

@ipspace ipspace merged commit ae80ce3 into ipspace:bgp-bw Jun 9, 2024
5 checks passed
@jbemmel jbemmel deleted the sros-add-bgp-bandwidth branch June 9, 2024 11:43
@jbemmel
Copy link
Collaborator Author

jbemmel commented Jun 9, 2024

Updated, SR OS does support outgoing updates. There is a missing 3rd type: Locally add a community to incoming routes

That's what bgp.bandwidth.in: auto (or just bgp.bandwidth: auto) is supposed to be doing, or did you have something else in mind?

There are 3 knobs:

  1. Add bandwidth extended community to incoming EBGP routes
  2. Accept (trust) extended community on incoming routes
  3. Send extended community on advertised routes

#1 is currently being called "in", and #3 is "out". I didn't add #2 yet, because there is no flag for it

@ipspace
Copy link
Owner

ipspace commented Jun 9, 2024

There are 3 knobs:

On SR OS ;)

  1. Accept (trust) extended community on incoming routes

I could add bgp.bandwidth.in: accept keyword, but we're back to adding nerd knobs for one particular platform. Not sure anyone else has that same nerd knob; if you want to scrub communities, you do it with a route-map.

@jbemmel
Copy link
Collaborator Author

jbemmel commented Jun 10, 2024

There are 3 knobs:

On SR OS ;)

  1. Accept (trust) extended community on incoming routes

I could add bgp.bandwidth.in: accept keyword, but we're back to adding nerd knobs for one particular platform. Not sure anyone else has that same nerd knob; if you want to scrub communities, you do it with a route-map.

I'm perfectly happy with leaving things the way they are for now, merely pointing out that there is some asymmetry in the semantics around "in" versus "out" that could be confusing to some people

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants