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

Ethernet port speed for aggregated interfaces #1181

Closed
cfyo opened this issue Sep 16, 2024 · 5 comments · Fixed by #1183
Closed

Ethernet port speed for aggregated interfaces #1181

cfyo opened this issue Sep 16, 2024 · 5 comments · Fixed by #1183

Comments

@cfyo
Copy link
Contributor

cfyo commented Sep 16, 2024

We have an aggregated interface that is bundle made up of 5x 100G physical interfaces. The /interfaces/interface/aggregation/state/lag-speed shows up correctly for the aggregated interface but for the path /interfaces/interface/ethernet/state/port-speed & negotiated-port-speed the vendor assigns the closest value which in this case was speed_600GB which is not accurate. What should be the behavior for this use case in OpenConfig?

@dplore
Copy link
Member

dplore commented Sep 16, 2024

Hi, it seems port-speed maybe didn't think through the use case for aggregated ports and only considered speed of physical ports.

Solutions for this could be

  1. add more ETHERNET_SPEED identities. (backwards compatible, but not great scalability as there would need to be very long list of speeds to cover operational use cases)
  2. Create a new leaf specifically for aggregated interface speeds, of type uint64
  3. Other ideas?

@dplore dplore moved this to Todo in OC Operator Review Sep 16, 2024
@cfyo
Copy link
Contributor Author

cfyo commented Sep 16, 2024

If there was a new leaf added called "aggregate-port-speed" what should the behavior be for the other leaves port-speed and negotiated-port-speed in this situation? I would still not want them showing an incorrect value. It seems like we may still need a new ETHERNET_SPEED identity added for this use case even with the addition of a new leaf. And if so, what would you recommend for the name?

@LimeHat
Copy link

LimeHat commented Sep 16, 2024

I would argue that port-speed is simply not applicable to the LAG interfaces.

re.

  1. Create a new leaf specifically for aggregated interface speeds, of type uint64

this already exists as indicated by the OP, /interfaces/interface/aggregation/state/lag-speed

@earies
Copy link
Contributor

earies commented Sep 16, 2024

I don't think (1) is feasible for the reasons already mentioned. The identities are tied to physical ethernet interface speeds and creating multiples to cover all aggregate use-cases is a non-starter imo.

Today, I believe an implementation should omit ethernet/state/port-speed for LAG interfaces for this reason rather than randomly choose the closest identity value, that is just incorrect. This leaf should only be applicable to physical interfaces (and not logical)

aggregation/state/lag-speed is already what should represent this for LAG interfaces and is less restrictive as a uint32 measured in Mbps. There is probably some usefulness to understand how the bundle is configured (e.g. 8x100G thus is desired to be 800Gbps) vs. dynamic adjustments should members fail (e.g. 6x100G == 600Gbps as 2 members went down)

It must be known however there are separate paths in order to access the speed of physical vs. logical but this can be determined by the type of interface

@earies
Copy link
Contributor

earies commented Sep 17, 2024

Submitted #1183 to put restrictions around physical vs. aggregate for ethernet related config/state

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

Successfully merging a pull request may close this issue.

4 participants