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 <[email protected]> On Behalf Of Martin Bjorklund > Sent: 21 August 2019 15:00 > To: [email protected] > 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 > [email protected] > https://www.ietf.org/mailman/listinfo/netmod _______________________________________________ netmod mailing list [email protected] https://www.ietf.org/mailman/listinfo/netmod
