This looks good to me and also removes the other leaves that aren't really valid for the aggregated interface. Thanks!

/gcbrun

Major YANG version changes in commit 78af366:
openconfig-if-ethernet.yang: 2.13.0 -> 3.0.0

Adding a backwards compatibility note. As currently proposed, a client / management system must know the type of interface in order to access the speed of the interface. (ie: physical vs. logical).

Would it be better to have a new "speed" leaf which is uint32 in Mbps that is generic across physical and logical interfaces? This could be introduced as a non-breaking change and would be easier to consume.

How important is it to use an identity for the speed of a physical port?

Adding a backwards compatibility note. As currently proposed, a client / management system must know the type of interface in order to access the speed of the interface. (ie: physical vs. logical).

Would it be better to have a new "speed" leaf which is uint32 in Mbps that is generic across physical and logical interfaces? This could be introduced as a non-breaking change and would be easier to consume.

How important is it to use an identity for the speed of a physical port?

The identity (or enum) approach is helpful for a finite set of values especially when configuring between an choice. Having this as a unbound uint32 will require implementations to dictate the format/matrix of what is acceptable vs. not and have more checks in place as to what to accept.

But it would represent a single leaf at a common location no matter the type however some interface types may not have a notion of "speed" still

For reference from ietf-interfaces for a r/o state node only:

      leaf speed {
        type yang:gauge64;
        units "bits/second";
        config false;
        description
            "An estimate of the interface's current bandwidth in bits
             per second.  For interfaces that do not vary in
             bandwidth or for those where no accurate estimation can
             be made, this node should contain the nominal bandwidth.
             For interfaces that have no concept of bandwidth, this
             node is not present.";
        reference
          "RFC 2863: The Interfaces Group MIB -
                     ifSpeed, ifHighSpeed";
      }

This patch set also differentiates more than just "speed" and more about physical vs. logical

The lag-speed under /interfaces/interface/aggregation/state
Reports effective speed of the aggregate interface, based on speed of active member interfaces
so do we need another speed leaf for it?

Would it be better to have a new "speed" leaf which is uint32 in Mbps that is generic across physical and logical interfaces? This could be introduced as a non-breaking change and would be easier to consume.

I would say no, it will not be better.

First, the current ethernet interface speed is RW (can be configured). In some cases the speed is auto-derived, but that's not necessarily true. And the config have to be restricted to the IEEE defined ethernet speeds (one way or another).

Secondly, is there a real use case for the consumption of interface speeds by a controller that is not aware of what type of interface it uses? And to add to that, that controller cannot subscribe to two sets of leaves and has to consume it from the single one? That feels like a bit of a stretch.

Last but not least, I would disagree with the notion of introduced as a non-breaking change.
The way "non-breaking" is defined in OpenConfig community is very limited and doesn't take into the account the real world usage of the models.

The current leaves are well defined and been present in the models for almost 10 years, and there are a significant number of implementations. Introducing a new leaf that is going to replace the two old ones is actually will be non-backward compatible for all these implementations (client software will have to be updated first and support the consumption of two sets of leaves, until all NOS systems are fully migrated to the new model; and make a decision which one to consume based on a specific NOS version/capabilities advertisement). Are we making it easier to consume? In my opinion, the answer is no (and we are not solving a real issue anyway)

Reviewed in Sep 24, 2024 OC Operators meeting:

It seems the model already has the leaves we need

• port-speed for singleton interfaces (non-lag / non-aggregate ports)
• aggregation/state/lag-speed for aggregate / lag interfaces

This PR introduces a change to limit port-speed to only be present when interface type is iana-if-type:ethernetCsmacd

lag-speed is already conditional on ianaift:ieee8023adLag

So in concept this PR looks good.

The CI check with pyang detected some issue, probably related to naming changes in the yang groupings. Can you fix @earies ?

Apologies for any confusion on my earlier naive comment about adding a leaf as we already have the leaf I was thinking about (lag-speed).

The CI check with pyang detected some issue, probably related to naming changes in the yang groupings. Can you fix @earies ?

This check actually appears to open a can of worms on the design and integration of wifi related modules (something I have not really paid attention to the progressions over the years)

The wifi modules make heavy reuse of other published models - for this case openconfig-if-ethernet - however redefine trees up to a certain anchor point and use groupings from other modules thus absorbing wifi related namespaces (and thus creating issues for prefix checks in when/must clauses) - short excerpt of where the offending clause is triggered below (note the prefixes in use):

module: openconfig-access-points
  +--rw access-points
     +--rw access-point* [hostname]
        +--rw oc-ap-if:interfaces
           +--rw oc-ap-if:interface* [name]
              +--rw oc-ap-if:name             -> ../config/name
              +--rw oc-ap-if:config
              |  +--rw oc-ap-if:name?            string
              |  +--rw oc-ap-if:type             identityref
              |  +--rw oc-ap-if:mtu?             uint16
              |  +--rw oc-ap-if:loopback-mode?   oc-opt-types:loopback-mode-type
              |  +--rw oc-ap-if:description?     string
              |  +--rw oc-ap-if:enabled?         boolean

This then means that the when restriction introduced here cannot be qualified for the wifi modules because they carry a different prefix in the relative path (oc-ap-if:) from an entirely different module. This new when statement also cannot accommodate including the reuse of groupings in wifi as that would create a circular dependency between the modules (needing to import the wifi modules into the core interface modules).

The wifi modules appear to implement a controller/service viewpoint vs. a network-element viewpoint exacerbating the issue (as intents then differ) with grouping reuse and imposing restrictions into used model-sets.

Due to the design and colocation of the wifi modules with what are mostly router/switch feature set device modules, it appears we have a design consideration we need to discuss further as I cannot see any elegant way out of this atm.

cc: @mike-albano

For this particular change, perhaps a simple description update would be sufficient?

For this particular change, perhaps a simple description update would be sufficient?

Sadly that is the likely fastest/easiest option here but this represents a bigger issue. Raised #1193 for further discussion

/gcbrun

@cfyo @dplore - please re-review - the PR has been updated to remove any when statements and describe behavior within description statements. This should be looked at as a temporary solution until we address the bigger problem statement on model reuse/inheritance.

/gcbrun

Setting last call for comments on Oct 22, 2024