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.

Sign up for GitHub

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

Jump to bottom

Multipath link bandwidth enabled #1028

Merged
merged 13 commits into from
Jan 30, 2024
Merged

Multipath link bandwidth enabled #1028

merged 13 commits into from
Jan 30, 2024

Conversation

rszarecki
Copy link
Contributor

@rszarecki rszarecki commented Dec 28, 2023 •
edited
Loading

Change Scope

This PR provides configuration hirarchy that allows to enable/disable honoring link-bandwidth extended community when BGP multipath is forming RIB/FIB entries.

It is done by by link-bandwidth-ext-community dedicated container, so more attributes can be added in future, without breaking model. (e.g. precission/scalaling/reference factors. Or zero value handling).

  +--rw network-instances
     +--rw network-instance* [name]
        +--rw protocols
           +--rw protocol* [identifier name]
              +--rw bgp
                 +--rw global
                    +--rw afi-safis
                       +--rw afi-safi* [afi-safi-name]
                          +--rw use-multiple-paths
                             +--rw config
                             |  +--rw enabled?   boolean
                             +--ro state
                             |  +--ro enabled?   boolean
                             +--rw ebgp
                             |  +--rw link-bandwidth-ext-community         <<<<<<<<
                             |  |  +--rw config
                             |  |  |  +--rw enabled?   boolean
                             |  |  +--ro state
                             |  |     +--ro enabled?   boolean
                             ...
                             +--rw ibgp
                                +--rw link-bandwidth-ext-community         <<<<<<<<
                                |  +--rw config
                                |  |  +--rw enabled?   boolean
                                |  +--ro state
                                |     +--ro enabled?   boolean

same structure repeats for all levels:

/network-instances/network-instance/protocols/protocol/bgp/global/use-multiple-paths/ebgp/link-bandwidth-ext-community/config/enabled
/network-instances/network-instance/protocols/protocol/bgp/global/use-multiple-paths/ebgp/link-bandwidth-ext-community/state/enabled
/network-instances/network-instance/protocols/protocol/bgp/global/use-multiple-paths/ibgp/link-bandwidth-ext-community/config/enabled
/network-instances/network-instance/protocols/protocol/bgp/global/use-multiple-paths/ibgp/link-bandwidth-ext-community/state/enabled
/network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/afi-safis/afi-safi/use-multiple-paths/ebgp/link-bandwidth-ext-community/config/enabled
/network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/afi-safis/afi-safi/use-multiple-paths/ebgp/link-bandwidth-ext-community/state/enabled
/network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/afi-safis/afi-safi/use-multiple-paths/ibgp/link-bandwidth-ext-community/config/enabled
/network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/afi-safis/afi-safi/use-multiple-paths/ibgp/link-bandwidth-ext-community/state/enabled
/network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/use-multiple-paths/ebgp/link-bandwidth-ext-community/config/enabled
/network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/use-multiple-paths/ebgp/link-bandwidth-ext-community/state/enabled
/network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/use-multiple-paths/ibgp/link-bandwidth-ext-community/config/enabled
/network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/use-multiple-paths/ibgp/link-bandwidth-ext-community/state/enabled

This leaf has no effect if BGP multi-path is disabled or if maximum-path attribute of BGP multi-path value is set to 1.

Implementation that do support only one state (e.g. implementation do not support BGP wECMP at all) should still support this leaf and accept configuration if value is set to FALSE, while reject configuration when value is set to TRUE.

Platform Implementations

@OpenConfigBot
Copy link

OpenConfigBot commented Dec 28, 2023 •
edited
Loading

No major YANG version changes in commit e0a2d5d

@rszarecki rszarecki marked this pull request as ready for review December 28, 2023 19:48
@rszarecki rszarecki requested a review from a team as a code owner December 28, 2023 19:48
@dplore dplore self-assigned this Dec 28, 2023
@dplore
Copy link
Member

dplore commented Dec 28, 2023

Should we not specify a default? Since Arista EOS is disabled by default and the other vendors are enabled by default we could leave out a default value, which means the default, if any, is up to the device. We can further give guidance in the description for this leaf to always set the desired value to ensure the desired behavior occurs.

@nandanarista
Copy link

Should we not specify a default? Since Arista EOS is disabled by default and the other vendors are enabled by default we could leave out a default value, which means the default, if any, is up to the device. We can further give guidance in the description for this leaf to always set the desired value to ensure the desired behavior occurs.

+1 to not having a default.
Depending on how a h/w platform implements wECMP, enabling wECMP can lead to more resource utilization, so seems like a somewhat unsafe default=True in that regard.

@rszarecki
Copy link
Contributor Author

rszarecki commented Jan 4, 2024 via email

dplore
dplore reviewed Jan 9, 2024
View reviewed changes
Copy link
Member

@dplore dplore left a comment

Choose a reason for hiding this comment

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

looks good except the default should be removed as discussed. You may update the description to indicate the expected behavior when setting this leaf.

nandanarista
nandanarista reviewed Jan 9, 2024
View reviewed changes
Copy link

@nandanarista nandanarista left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together, Rafal!
one nit and one question

@dplore
Copy link
Member

dplore commented Jan 11, 2024

This was reviewed at the Jan 9th OC Operators meeting without objection. Setting last call to Jan 16, 2024

@dplore dplore changed the title Multipath link bandwdth Multipath link bandwidth enabled Jan 12, 2024
@dplore
Copy link
Member

dplore commented Jan 26, 2024

@rszarecki this is ready except for the version conflicts. Can you fix for us?

@rszarecki
Copy link
Contributor Author

@dplore
version conflicts fixed.

dplore
dplore approved these changes Jan 30, 2024
View reviewed changes
Copy link
Member

@dplore dplore left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @rszarecki !

@dplore dplore merged commit f46ca2c into openconfig:master Jan 30, 2024
14 checks passed
romeyod pushed a commit to romeyod/aftNextHop that referenced this pull request Sep 19, 2024
@rszarecki @romeyod
Multipath link bandwidth enabled (openconfig#1028)
617fa6e 
* BGP link bandwidth multipath enable at global level
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nandanarista nandanarista nandanarista left review comments

@dplore dplore dplore approved these changes

Assignees

@dplore dplore

Labels
non-breaking
Projects
OC Operator Review
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rszarecki @OpenConfigBot @dplore @nandanarista