Re: [netmod] WG Last Call: draft-ietf-netmod-intf-ext-yang-07

["Rob Wilton (rwilton)" <[e-mail]>]

Hi Martin,

Thanks for the review.  Mostly just updated, a couple of comments/questions inline below.

Latest source XML and txt are at: https://github.com/netmod-wg/interface-extensions-yang/tree/wglc


> -----Original Message-----
> From: netmod <[e-mail]> On Behalf Of Martin Bjorklund
> Sent: 21 August 2019 15:00
> To: [e-mail]
> Subject: Re: [netmod] WG Last Call: draft-ietf-netmod-intf-ext-yang-07
> 
> Hi,
> 
> Here is my (late) review of draft-ietf-netmod-intf-ext-yang-07.  It is a
> well-written document and my comments are mostly minor.
> 
> 
> o  Abstract
> 
>   OLD:
> 
>    The YANG data model in this document conforms to the Network
>    Management Datastore Architecture (NMDA) defined in RFC 8342.
> 
>   NEW:
> 
>    The YANG modules in this document conform to the Network
>    Management Datastore Architecture (NMDA) defined in RFC 8342.
> 

OK.


> 
> o  Section 1
> 
>      One of the aims of this draft is to provide a standard namespace and
>      path for these configuration items regardless of the underlying
>      interface type.  For example a standard namespace and path for
> 
>   "standard namespace and path" sounds a bit clumsy.  In section 2 you
>   use "standard definition", perhaps that can be use here.
> 

OK. 

> 
> o  General
> 
>   s/this internet draft/this document/g
>   s/this draft/this document/g
> 

OK.


> 
> o  Section 2
> 
>   It seems this short section mostly says what is already said in
>   section 1.  Remove?
> 

At this stage yes, I think that this can be removed.

> 
> o  3
> 
>   The text says:
> 
>     o  A parent interface leaf useable for all types of sub-interface
>        that are children of parent interfaces.
> 
>   I suggest you add before that bullet:
> 
>     o  A generic "sub-interface" identity that an interface identity
>        defintion can derive from if it defines a sub-interface.
> 

OK.


> 
> o  3.1
> 
>   The text says:
> 
>     E.g. in the
>     case that the link state transition is suppressed then there is no
>     change of the /if:interfaces-state/if:interface/oper-status or
>     /if:interfaces-state/if:interfaces/last-change leaves for the
>     interface that the feature is operating on.
> 
>   This should be:
> 
>     no change of the /if:interfaces/if:interface/oper-status or
>     /if:interfaces/if:interfaces/last-change leaves for the
>     interface that the feature is operating on.
> 

OK.


> 
> o  3.2
> 
>   It took me some time to understand the dampening algorithm.  Why is
>   it important to talk about nominal values and that a device doesn't
>   have to use 1000 as the penalty, as long as they scale the given
>   values?  Wouldn't it be easier to describe the algorithm w/o any
>   nominal values, and then explain that an implementation is free to
>   implement this algorithm in any way it wants (which of course is
>   true for everything we do...)

That makes sense.  I have tweaked the description.

> 
>   Otherwise, the text currently says:
> 
>    Implementations are not required to use a penalty of 1000 units in
>    their dampening algorithm, but should ensure that the Suppress
>    Threshold and Reuse Threshold values are scaled relative to the
>    nominal 1000 unit penalty to ensure that the same configuration
>    values provide consistent behaviour.
> 
>   Should "should" in this text be "SHOULD"?  Or perhaps "MUST"?
> 

I've just removed this text.  As you say above, this is just a definition of an API, and vendors are free to implement however they wish as long as they are consistent with the API.


> 
> o  3.2.1
> 
>   The text says:
> 
>    When the accumulated penalty reaches the default or
>    configured suppress threshold, the interface is placed in a dampened
>    state.
> 
>   The term "dampended state" occurs twice, in 3.2.1 and 3.2.3.  It is
>   not used in the YANG model.  I suspect the leaf "suppressed"
>   reflects this.  Perhaps align naming.
> 

I've changed this to suppressed.


> 
> o  4
> 
>   It would be useful with a sentence that describes the relationship
>   to /if:interfaces/if:interface/if:phys-address.
> 
>   It seems that the mac-address leaf is useful when the mac address
>   can be configured; otherwise if:phys-address should be sufficient,
>   right?

Yes, if not configured, it returns the same value as if:phys-address (but constrained to being exactly 6 bytes long).

Perhaps a better long term solution here would be to allow if:phys-address to be configurable, and add a hw-phys-address config false leaf to indicate the default hardware value?


  Should the mac-address leaf have a feature, or can we expect
>   all implementations to support configurable mac addresses?

No, I don't think that all Ethernet interfaces will necessarily support configurable MAC addresses, e.g. I suspect that a lightbulb or simple home router probably does not allow it to be configured.  But I would expect Linux and more sophisticated network devices to allow MAC addresses to be configured.  Hence making this conditional on a feature makes sense.

         leaf mac-address {
           type yang:mac-address;
           description
             "The MAC address of the interface.";
         }

In terms of the relationship to phy-address, I would suggest the following update:

      leaf mac-address {
        if-feature "configurable-mac-address";
        type yang:mac-address;
        description
          "The MAC address of the interface.  The operational value
           matches the if:phys-address leaf defined in
           ietf-interface.yang";
      }
 


> 
> 
> o  4
> 
>   You add a container 'statistics' under 'ethernet-like', so we have:
> 
>   +--rw interfaces
>      +--rw interface* [name]
>         ...
>         +--ro statistics
>            ...
>         +--rw ethlike:ethernet-like
>            +--ro ethlike:statistics
>               ...
> 
>   Did you consider augmenting the container if:statistics instead?  I
>   think it can be useful to have all statistics in the same container
>   in this case.

I think that given that we are only adding one counters and they are probably widely useful for Ethernet interfaces then I think that augmenting the existing statistics container makes sense.  I will change this.

If we end up defining Ethernet histogram counters (which we are not doing now, and would be a separate draft), then we might want to think carefully whether or not to put those counters under the main interface statistics container, because I suspect that most of the time clients probably won't be interested in those values.

> 
> 
> o  7.2
> 
>   Perhaps show the (related) 'if:oper-status' leaf as well.
> 

Yes.


> 
> o  7.3
> 
>   Perhaps show the (related) if:phys-address' leaf as well in the
>   first and third examples.

OK.

> 
>   Before the second example, perhaps change:
> 
>    The following example shows an explicit MAC address being configured
>    on interface eth0.
> 
>   to:
> 
>    The following example shows the intended configuration for
>    interface eth0 with an explicit MAC address being configured.
> 

Fixed.

> 
> o  YANG nits
> 
>   Both YANG modules list the WG chairs; we don't do that anymore.

Fixed.

> 
>   Both YANG modules have the IETF Trust Copyright statement, but not
>   exactly as it should be (try: pyang --ietf and/or pyang --ietf-help)
> 
>   Many descriptions are full sentences w/o the ending ".".
> 

Fixed, I think.

>   The reference in the revision statement should be changed to "RFC
>   XXXX: <title>"
> 

Fixed.

Thanks,
Rob


> 
> 
> /martin
> 
> _______________________________________________
> netmod mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/netmod