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 config and state for individual Ethernet port priority in a LAG #943

Merged
merged 10 commits into from
Dec 12, 2023

Conversation

marius-ore
Copy link
Contributor

@marius-ore marius-ore commented Aug 17, 2023

Change Scope

  • Add port-priority config and state for Ethernet interface that is part of a logical aggregation / LAG
  • Add /lacp/interfaces/interface/members/member/state/partner-port-priority
  • This change is not backwards compatible due to breaking change of changing type for list.

Platform Implementations

Tree view

module: openconfig-lacp
  +--rw lacp
     +--rw config
     |  +--rw system-priority?   uint16
     +--ro state
     |  +--ro system-priority?   uint16
     +--rw interfaces
        +--rw interface* [name]
           +--rw name       -> ../config/name
           +--rw config
           |  +--rw name?              oc-if:base-interface-ref
           |  +--rw interval?          lacp-period-type
           |  +--rw lacp-mode?         lacp-activity-type
           |  +--rw system-id-mac?     oc-yang:mac-address
           |  +--rw system-priority?   uint16
           +--ro state
           |  +--ro name?              oc-if:base-interface-ref
           |  +--ro interval?          lacp-period-type
           |  +--ro lacp-mode?         lacp-activity-type
           |  +--ro system-id-mac?     oc-yang:mac-address
           |  +--ro system-priority?   uint16
           +--rw members
              +--rw member* [interface]
                 +--rw interface    -> ../config/interface                          <-- changed
                 +--rw config                                                              <-- new
                 |  +--rw interface?       oc-if:base-interface-ref.            <-- new
                 |  +--rw port-priority?   uint16                                <-- new
                 +--ro state
                    +--ro interface?               oc-if:base-interface-ref
                    +--ro port-priority?           uint16
                    +--ro activity?                lacp-activity-type
                    +--ro timeout?                 lacp-timeout-type
                    +--ro synchronization?         lacp-synchronization-type
                    +--ro aggregatable?            boolean
                    +--ro collecting?              boolean
                    +--ro distributing?            boolean
                    +--ro system-id?               oc-yang:mac-address
                    +--ro oper-key?                uint16
                    +--ro partner-id?              oc-yang:mac-address
                    +--ro partner-key?             uint16
                    +--ro port-num?                uint16
                    +--ro partner-port-num?        uint16                                    <-- new
                    +--ro partner-port-priority?   uint16
                    +--ro last-change?             oc-types:timeticks64
                    +--ro counters
                       +--ro lacp-in-pkts?               oc-yang:counter64
                       +--ro lacp-out-pkts?              oc-yang:counter64
                       +--ro lacp-rx-errors?             oc-yang:counter64
                       +--ro lacp-tx-errors?             oc-yang:counter64
                       +--ro lacp-unknown-errors?        oc-yang:counter64
                       +--ro lacp-errors?                oc-yang:counter64
                       +--ro lacp-timeout-transitions?   oc-yang:counter64

@marius-ore marius-ore requested a review from a team as a code owner August 17, 2023 15:54
@google-cla
Copy link

google-cla bot commented Aug 17, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@OpenConfigBot
Copy link

OpenConfigBot commented Aug 17, 2023

Major YANG version changes in commit 42ff19a:
openconfig-lacp.yang: 1.2.0 -> 2.0.0

@OpenConfigBot
Copy link

OpenConfigBot commented Aug 17, 2023

Compatibility Report for commit e86f152:
pyangbind@6b85c2b

Deprecate aggregate-id and move it to dedicated ethernet container for
for interface's link aggregation parameters. Add port-priority to this
container. Add actor and partner port priority to LACP member state.
Fix version numbering, ordering of yang statements to pass
pyang --lint --strict, and remove default value for port priority
release/models/interfaces/openconfig-if-aggregate.yang Outdated Show resolved Hide resolved
release/models/lacp/openconfig-lacp.yang Outdated Show resolved Hide resolved
Add config container for LACP aggregate interface member with port-priority
and interface. Change /lacp/interfaces/interface/members/member/interface to
point to the new leaf /lacp/interfaces/interface/members/member/config/interface.
Change /lacp/interfaces/interface/members to config true.
release/models/lacp/openconfig-lacp.yang Outdated Show resolved Hide resolved
release/models/lacp/openconfig-lacp.yang Show resolved Hide resolved
Replaced repeated definition of leaf port-priority with `uses` statement, as per change request.
@marius-ore
Copy link
Contributor Author

I think I've completed all the changes suggested so far. Any other feedback, or is the code good to merge?

@dplore
Copy link
Member

dplore commented Oct 3, 2023

I added the tree view to the description for the PR.

Summary of what was added:

module: openconfig-lacp
+--rw lacp
+--rw interfaces
+--rw interface* [name]
+--rw members
+--rw member* [interface]
+--rw interface -> ../config/interface
+--rw config
| +--rw port-priority? uint16
+--ro state
+--ro partner-port-priority? uint16

@dplore dplore requested review from robshakir and dplore October 3, 2023 16:55
dplore
dplore previously approved these changes Oct 3, 2023
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, last call for this change

@dplore dplore self-assigned this Oct 3, 2023
@dplore
Copy link
Member

dplore commented Oct 3, 2023

Note, this is considered a breaking change because the list key for members was changed to point at the new config/interface leaf instead of the prior state/interface leaf.

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.

Since this is now a non-breaking change, I've suggested a new, minor version number increment

release/models/lacp/openconfig-lacp.yang Show resolved Hide resolved
release/models/lacp/openconfig-lacp.yang Show resolved Hide resolved
@dplore
Copy link
Member

dplore commented Nov 28, 2023

@robshakir for your review

robshakir
robshakir previously approved these changes Dec 4, 2023
Copy link
Contributor

@robshakir robshakir left a comment

Choose a reason for hiding this comment

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

LGTM, one indentation issue and then I think that this is a backwards incompatible change (as discussed with @dplore).

release/models/lacp/openconfig-lacp.yang Outdated Show resolved Hide resolved
dplore and others added 2 commits December 11, 2023 14:32
Co-authored-by: Rob Shakir <[email protected]>
Due to breaking change of changing type for list
@dplore dplore merged commit 48a1c13 into openconfig:master Dec 12, 2023
14 checks passed
romeyod pushed a commit to romeyod/aftNextHop that referenced this pull request Sep 19, 2024
…penconfig#943)

* add config container to LACP aggregate interface member

Add config container for LACP aggregate interface member with port-priority
and interface. Change /lacp/interfaces/interface/members/member/interface to
point to the new leaf /lacp/interfaces/interface/members/member/config/interface.
Change /lacp/interfaces/interface/members to config true.

This change is not backwards compatible due to breaking change of changing type for list

---------

Co-authored-by: Darren Loher <[email protected]>
Co-authored-by: Rob Shakir <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants