Re: [netmod] WG Last Call: draft-ietf-netmod-sub-intf-vlan-model-05
Hi, Here is my (late) review of draft-ietf-netmod-sub-intf-vlan-model-05. o 1 The YANG module names are not correct; they are listed as: if-l3-vlan.yang - Defines the model for basic classification of VLAN tagged traffic to L3 transport services flexible-encapsulation.yang - Defines the model for flexible classification of Ethernet/VLAN traffic to L2 transport services Should be "ietf-if-l3-vlan" and "ietf-flexible-encapsulation". Or "ietf-if-l3-vlan" and "ietf-if-flexible-encapsulation". But I also wonder if these names should somehow be changed. What is a "l3-vlan"? And "flexible-encapsulation" sound a bit too generic. o 1.1 The text says: Sub-interface: A sub-interface is a small augmentation of a regular interface in the standard YANG module for Interface Management that represents a subset of the traffic handled by its parent interface. I think the augmentation is the YANG-realization of a sub-interface, but it is not what a sub-interface is. Also, this definition is mis-leading; it doesn't mention that a sub-interface has its own interface type and is represented as one separate entry in the interface list. I think it is better to import this term from draft-ietf-netmod-intf-ext-yang (section 3.6) o 3 The text says: The L3 Interface VLAN model provides appropriate leaves for termination of an 802.1Q VLAN tagged segment to a sub-interface based L3 service. There is a comment in the YANG model that says the same thing. But the YANG model itself augments not only to sub-interface-based interface, but also to ethernet-like interfaces. o YANG modules Both modules lack the IETF Trust Copyright statement. We don't list WG Chairs anymore. The revision statements should be on the form: "RFC : " Many descriptions are full sentences w/o the ending ".". The modules should be indented properly; a starting point can be pyang -f yang --yang-line-length 69 o ietf-if-l3-vlan There is a comment: /* * Matches a single VLAN Id, or a pair of VLAN Ids to classify * traffic into an L3 service. */ This should be moved into a description clause. o ietf-if-l3-vlan / container dot1q-vlan The must statement has: count(../../if-cmn:forwarding-mode) = 0 This can be changed to not(../../if-cmn:forwarding-mode) which is more direct imo. The must statement's description statement seems to be a copy-and-paste error. o ietf-if-l3-vlan / container dot1q-vlan The descriptions talk about "matching frames" and "classifying traffic", but it is not described anywhere how the matching and classifying is used. (also applies to ietf-flexible-encapsulation) o ietf-if-l3-vlan / outer-tag / second-tag These names are a bit inconsistent. The description describes them as "outermost tag" and "second outermost tag". Perhaps use these names instead? (same names are found in ietf-flexible-encapsulation) o ietf-flexible-encapsulation / all features The features are described as: "This feature indicates whether the network element supports specifying flexible rewrite operations"; Should this be s/whether/that ? o ietf-flexible-encapsulation There is some descriptive text in comments that should be moved to description statements. o ietf-flexible-encapsulation The descriptions for pop/push are a bit terse. It seems to assume that readers already know what this (from somewhere) is so it doesn't need to be described. If this is intended, perhaps add a reference to where this is described. /martin ___ netmod mailing list netmod@ietf.org https://www.ietf.org/mailman/listinfo/netmod
Re: [netmod] WG Last Call: draft-ietf-netmod-intf-ext-yang-07
Hi Martin, Please see comments inline ... > -Original Message- > From: Martin Bjorklund > Sent: 22 August 2019 12:35 > To: Rob Wilton (rwilton) > Cc: vladi...@transpacket.com; netmod@ietf.org > Subject: Re: [netmod] WG Last Call: draft-ietf-netmod-intf-ext-yang-07 > > Hi, > > Some comments inline. > > > "Rob Wilton (rwilton)" wrote: > > Hi Vladimir, > > > > Thanks for your detailed review. Sorry for the slow reply, I've been > > away. I'm also about to be away again for a couple of days. > > > > Please see my comments inline ... > > > > I'll also track these issues to closure on > > https://github.com/netmod-wg/interface-extensions-yang/issues > > > > > -Original Message- > > > From: netmod On Behalf Of Vladimir > > > Vassilev > > > Sent: 13 August 2019 17:05 > > > To: Kent Watsen ; netmod@ietf.org > > > Subject: Re: [netmod] WG Last Call: > > > draft-ietf-netmod-intf-ext-yang-07 > > > > > > I have reviewed the draft. I have the following (19) IMO useful > > > proposals: > > > > > > 1. Dedicated module (ietf-if-oper-status-debounce.yang) for the > > > oper- status debouncing/dampening functionality currently in > > > ietf-interfaces- > > > common.yang. > > > > I don't think that we want a proliferation of too many separate YANG > > modules for small features. Each of the areas of different > > functionality within this module are already conditional on > > if-feature, so I don't think that there is a strong justification to > > separating this out as a separate module. > > I agree. > > > > 4. Dedicated module (ietf-if-loopback.yang) for the loopback > > > functionality currently in ietf-interfaces-common.yang. > > > > Same answer as for 1. I don't think that we should have too many > > really small modules. > > I agree. > > > > > 10. Introducing config true "forwarding-mode" leaf breaks clients > > > that support e.g. rfc8344 ietf-ip (which has its dedicated > > > forwarding leafs e.g. > > > /ietf-interfaces:interfaces/interface/ietf-ip:ipv4/forwarding ) by > > > introducing this new module with a new leaf they know nothing about. > > > I support this leaf as config false. If NETCONF was not > > > transactional a global leaf enabling the forwarding configuration > would be a feature. > > > But NETCONF is transactional. > > > > I don't get the relevance of transactions, but it isn't intended to > > break existing clients/YANG modules. > > > > The idea of this leaf is that if it is configured then the system can > > use it to check other constraints. E.g. to validate that an L2 QoS > > policy isn’t being configured on an L3 interface. If the leaf isn't > > configured then those constraints are not checked. > > Hmm. Are you saying that this leaf doesn't have any direct effect in the > server? It depends on the device. Some devices require a leaf like this (or similar) to correctly provision the services. Other devices don't need it. > > > > 12. I do not agree we need this text. Normally NETCONF devices > > > should accept transactions to any valid configuration: > > > > > > OLD: > > > ... > > > Normally devices will not allow the parent-interface leaf to be > > > changed after the interfce has been created. If an > > > implementation > > > did allow the parent-interface leaf to be changed then it could > > > cause > > > all traffic on the affected interface to be dropped. The > > > affected > > > leaf is: > > > > > > o /if:interfaces/if:interface/parent-interface > > > ... > > > > > > NEW: > > > ... > > > Changing the parent-interface leaf could cause > > > all traffic on the affected interface to be dropped. > > > The affected leaf is: > > > > > > o /if:interfaces/if:interface/parent-interface > > > ... > > > > This isn't about transactions so much as valid configuration. > > > > Normally, the name of the sub-interface is tightly bound to the parent > > interface. E.g. if the parent in "Ethernet0/1" then the sub-interface > > would be "Ethernet0/1.1". If you tried to change the parent-interface > > leaf of "Ethernet0/1.1" to "Ethernet2/2" then I would expect the > > system to reject that change (because the configuration is invalid not > > because of transactions). > > Well, this is already described in section 3.6. The quoted text is from > Security Considerations. I agree with Vladimir; I think his suggested > text is better. Ah, OK. If it is in the security section, then I agree that it isn't required. > > > > 14. I propose the in-pkts and out-pkts counters standardized too. > > > https://github.com/YangModels/yang/blob/master/vendor/cisco/xe/1641/ > > > ietf- > > > interfaces-ext.yang. > > > And yes someone forgot to update the boilerplate text. > > > > This one I think that we need to get further input on. > > > > https://github.com/YangModels/yang/blob/master/standard/ieee/published > > /802.3/ieee802-ethernet-interface.yang > > > > defines in-frames and out-frames, but these are only for
Re: [netmod] Barry Leiba's No Objection on draft-ietf-netmod-artwork-folding-08: (with COMMENT)
> These look like good comments to me, and I think we should fix them all. Thanks, Adrian! b ___ netmod mailing list netmod@ietf.org https://www.ietf.org/mailman/listinfo/netmod
Re: [netmod] WG Last Call: draft-ietf-netmod-intf-ext-yang-07
Martin Bjorklund wrote: > Hi, > > Some comments inline. > > > "Rob Wilton (rwilton)" wrote: > > Hi Vladimir, > > > > Thanks for your detailed review. Sorry for the slow reply, I've been > > away. I'm also about to be away again for a couple of days. > > > > Please see my comments inline ... > > > > I'll also track these issues to closure on > > https://github.com/netmod-wg/interface-extensions-yang/issues > > > > > -Original Message- > > > From: netmod On Behalf Of Vladimir Vassilev [...] > > > 19. ietf-if-common.yang and ietf-if-ethernet-like.yang instead of > > > ietf- > > > interfaces-common.yang and ietf-interfaces-ethernet-like.yang. > > > Setting a shorter naming precedent for future modules augmenting ietf- > > > interfaces. > > > > I'm not opposed to shorter names, but would be interested in the views > > of others in the WG. > > I had a similar concern for the modules in the sub-intf-vlan draft (I > will post my review of that doc later). > > Currently we have: > > ietf-interfaces-common > ietf-interfaces-ethernet-like > ietf-if-l3-vlan > ietf-flexible-encapsulation > > I think we should have consistency, either: > > ietf-interfaces-common > ietf-interfaces-ethernet-like > ietf-interfaces-l3-vlan > ietf-interfaces-flexible-encapsulation > > or > > ietf-if-common > ietf-if-ethernet-like > ietf-if-l3-vlan > ietf-if-flexible-encapsulation One comment re naming here. The name "ietf-interfaces-common" seems a bit odd; isn't "ietf-interfaces" for "common" definitions? I was going to suggest "ietf-interfaces-extensions", but then I re-read the description in the module: This module contains common definitions for extending the IETF interface YANG model (RFC 8343) with common configurable layer 2 properties. So perhaps "ietf-interfaces-l2-extensions" would be better? but then "forwarding-mode" isn't a l2 property. /martin ___ netmod mailing list netmod@ietf.org https://www.ietf.org/mailman/listinfo/netmod
Re: [netmod] WG Last Call: draft-ietf-netmod-intf-ext-yang-07
Hi, Some comments inline. "Rob Wilton (rwilton)" wrote: > Hi Vladimir, > > Thanks for your detailed review. Sorry for the slow reply, I've been > away. I'm also about to be away again for a couple of days. > > Please see my comments inline ... > > I'll also track these issues to closure on > https://github.com/netmod-wg/interface-extensions-yang/issues > > > -Original Message- > > From: netmod On Behalf Of Vladimir Vassilev > > Sent: 13 August 2019 17:05 > > To: Kent Watsen ; netmod@ietf.org > > Subject: Re: [netmod] WG Last Call: draft-ietf-netmod-intf-ext-yang-07 > > > > I have reviewed the draft. I have the following (19) IMO useful > > proposals: > > > > 1. Dedicated module (ietf-if-oper-status-debounce.yang) for the oper- > > status debouncing/dampening functionality currently in > > ietf-interfaces- > > common.yang. > > I don't think that we want a proliferation of too many separate YANG > modules for small features. Each of the areas of different > functionality within this module are already conditional on > if-feature, so I don't think that there is a strong justification to > separating this out as a separate module. I agree. > > 4. Dedicated module (ietf-if-loopback.yang) for the loopback > > functionality > > currently in ietf-interfaces-common.yang. > > Same answer as for 1. I don't think that we should have too many > really small modules. I agree. > > 10. Introducing config true "forwarding-mode" leaf breaks clients that > > support e.g. rfc8344 ietf-ip (which has its dedicated forwarding leafs > > e.g. /ietf-interfaces:interfaces/interface/ietf-ip:ipv4/forwarding ) > > by > > introducing this new module with a new leaf they know nothing about. I > > support this leaf as config false. If NETCONF was not transactional a > > global leaf enabling the forwarding configuration would be a feature. > > But NETCONF is transactional. > > I don't get the relevance of transactions, but it isn't intended to > break existing clients/YANG modules. > > The idea of this leaf is that if it is configured then the system can > use it to check other constraints. E.g. to validate that an L2 QoS > policy isn’t being configured on an L3 interface. If the leaf isn't > configured then those constraints are not checked. Hmm. Are you saying that this leaf doesn't have any direct effect in the server? > > 12. I do not agree we need this text. Normally NETCONF devices should > > accept transactions to any valid configuration: > > > > OLD: > > ... > > Normally devices will not allow the parent-interface leaf to be > > changed after the interfce has been created. If an implementation > > did allow the parent-interface leaf to be changed then it could > > cause > > all traffic on the affected interface to be dropped. The affected > > leaf is: > > > > o /if:interfaces/if:interface/parent-interface > > ... > > > > NEW: > > ... > > Changing the parent-interface leaf could cause > > all traffic on the affected interface to be dropped. > > The affected leaf is: > > > > o /if:interfaces/if:interface/parent-interface > > ... > > This isn't about transactions so much as valid configuration. > > Normally, the name of the sub-interface is tightly bound to the parent > interface. E.g. if the parent in "Ethernet0/1" then the sub-interface > would be "Ethernet0/1.1". If you tried to change the parent-interface > leaf of "Ethernet0/1.1" to "Ethernet2/2" then I would expect the > system to reject that change (because the configuration is invalid not > because of transactions). Well, this is already described in section 3.6. The quoted text is from Security Considerations. I agree with Vladimir; I think his suggested text is better. > > 14. I propose the in-pkts and out-pkts counters standardized too. > > https://github.com/YangModels/yang/blob/master/vendor/cisco/xe/1641/ietf- > > interfaces-ext.yang. > > And yes someone forgot to update the boilerplate text. > > This one I think that we need to get further input on. > > https://github.com/YangModels/yang/blob/master/standard/ieee/published/802.3/ieee802-ethernet-interface.yang > > defines in-frames and out-frames, but these are only for Ethernet, but > you are probably looking for a counter across all interface types. in-pkts is presumably in-unicast-pkts + in-broadcast-pkts + in-multicast-pkts. So is this really needed? > > 19. ietf-if-common.yang and ietf-if-ethernet-like.yang instead of > > ietf- > > interfaces-common.yang and ietf-interfaces-ethernet-like.yang. > > Setting a shorter naming precedent for future modules augmenting ietf- > > interfaces. > > I'm not opposed to shorter names, but would be interested in the views > of others in the WG. I had a similar concern for the modules in the sub-intf-vlan draft (I will post my review of that doc later). Currently we have: ietf-interfaces-common ietf-interfaces-ethernet-like ietf-if-l3-vlan ietf-flexible-enc
Re: [netmod] WG Last Call: draft-ietf-netmod-intf-ext-yang-07
Hi Vladimir, Thanks for your detailed review. Sorry for the slow reply, I've been away. I'm also about to be away again for a couple of days. Please see my comments inline ... I'll also track these issues to closure on https://github.com/netmod-wg/interface-extensions-yang/issues > -Original Message- > From: netmod On Behalf Of Vladimir Vassilev > Sent: 13 August 2019 17:05 > To: Kent Watsen ; netmod@ietf.org > Subject: Re: [netmod] WG Last Call: draft-ietf-netmod-intf-ext-yang-07 > > I have reviewed the draft. I have the following (19) IMO useful proposals: > > 1. Dedicated module (ietf-if-oper-status-debounce.yang) for the oper- > status debouncing/dampening functionality currently in ietf-interfaces- > common.yang. I don't think that we want a proliferation of too many separate YANG modules for small features. Each of the areas of different functionality within this module are already conditional on if-feature, so I don't think that there is a strong justification to separating this out as a separate module. > > 2. In sec "3.1 Carrier delay" use of the under-defined "Carrier" > definition can be replaced with direct reference to the oper-status leaf > (which is what is actually targeted by the algorithm) "Operational status > transition debouncing". I think that different vendors have different names for this technology. I've just used the one that our products use. I think that this is just a name, rather than something that has to be defined. I could add a comment that this feature is sometimes called hold time? > > 3. "timer-running" and "suppressed" leafs are both "config false" and have > "default" statements. Although this is valid YANG I do not think the > "default" statements are intended. I think that this is a more general question that needs a bit more discussion. Here, I am using defaults for the config false node to document what the normal value is expected. > > 4. Dedicated module (ietf-if-loopback.yang) for the loopback functionality > currently in ietf-interfaces-common.yang. Same answer as for 1. I don't think that we should have too many really small modules. > > 5. Less verbose loopback identities. With dedicated module the > (loopback-* identities can be shortened skipping the prefix). I think that it is normal to bind the identity names to the common base identity. I don't see that the length of the identities should really be an issue. > > 6. The draft introduces "loopback-internal", "loopback-line" and > "loopback-connector" loopback identities. What is confusing is that > "internal loopback" is historically the opposite of "external loopback" > which is a loopback with a connector. I think terminology already in use > like "near-end" and "far-end" is less confusing. The internal/line loopback configuration has been used in parts of the industry for at least 20 years, so this terminology is already in use. I'm not sure that "near-end" and "far-end" would be less confusing. Assuming that "loopback far-end" was equivalent to "loopback-line" then it would be somewhat of a misnomer since it acts on the near end, not the far end. I.e. both loopback internal, and loopback line act on the local interface, the only difference is in which direction they reflect the signals, i.e. Egress -> Ingress (internal), or Ingress -> Egress (line). Perhaps the description text could be slightly clarified here to help avoid confusion? OLD: The following loopback modes are defined: o Internal loopback - All egress traffic on the interface is internally looped back within the interface to be received on the ingress path. o Line loopback - All ingress traffic received on the interface is internally looped back within the interface to the egress path. o Loopback Connector - The interface has a physical loopback connector attached that loops all egress traffic back into the interface's ingress path, with equivalent semantics to internal loopback. NEW: The following loopback modes are defined: o Internal loopback - All frames that egress out of the interface are looped back internally within the interface hardware to be received on the ingress path. o Line loopback - All ingress frames received on the interface from the line are looped back within the interface hardware and transmitted back out of the interface. o Loopback connector - The interface has a physical loopback connector attached that loops all egress frames back into the interface's ingress path, with equivalent semantics to internal loopback. > > 7. I am not sure standardizing the "loopback-connector" identity is > justified. All usecases of connecting a loopback connector I can think of > require the system to not be aware there is special external loopback > connector on the interface. I think that it will depend on how smart of dumb the external loopback c
Re: [netmod] Barry Leiba's No Objection on draft-ietf-netmod-artwork-folding-08: (with COMMENT)
Thanks for the review, Barry.
These look like good comments to me, and I think we should fix them all.
Best,
Adrian
-Original Message-
From: Barry Leiba via Datatracker
Sent: 22 August 2019 05:41
To: The IESG
Cc: draft-ietf-netmod-artwork-fold...@ietf.org; Lou Berger ;
netmod-cha...@ietf.org; lber...@labn.net; netmod@ietf.org
Subject: Barry Leiba's No Objection on draft-ietf-netmod-artwork-folding-08:
(with COMMENT)
Barry Leiba has entered the following ballot position for
draft-ietf-netmod-artwork-folding-08: No Objection
When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)
Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.
The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-netmod-artwork-folding/
--
COMMENT:
--
— Section 4.1 —
I find the BCP 14 “SHOULD” in this section to be odd, and would lower-case
them.
When needed, this effort again
SHOULD be automated to reduce effort and errors resulting from manual
processing.
This sentence is really awkward: “when needed”, the use of “effort” twice, and
the uncertainty of whether the clause “resulting from manual processing”
applies to both effort and errors, or only to the latter. I would say it this
way:
NEW
This work should also be automated to reduce the effort and to reduce errors
resulting from manual processing. END
— Section 6 —
assumes that the continuation begins at the character that is
not a space character (' ') on the following line.
Should be “at the first character”.
— Section 7.1.1 —
The second line is a blank line.
The code in the appendix generates an *empty* line (no text). Is that what you
mean by “blank line”? Will a line that contains only space characters (*looks*
the same) work also? The code in the appendix appears to discard the second
line without checking its content at all. I think you should be clearer about
what qualifies as a “blank line”. (This also applies to Section 8.1.1.)
— Section 7.2.1 —
If this text content needs to and can be folded, insert the header
described in Section 7.1.1, ensuring that any additional printable
characters surrounding the header does not result in a line exceeding
the desired maximum.
Should be “do not result” (to match the plural “printable characters”).
___
netmod mailing list
netmod@ietf.org
https://www.ietf.org/mailman/listinfo/netmod
