Dear Mahesh, Thank you very much for the detailed review. Much appreciated.
It seems that you reviewed -02 of the draft, while the latest version is -06 (https://datatracker.ietf.org/doc/draft-ietf-softwire-dslite-yang/). As you can see below, almost all the comments do not apply for -06. Please see inline. Cheers, Med > -----Message d'origine----- > De : Mahesh Jethanandani [mailto:[email protected]] > Envoyé : samedi 7 octobre 2017 02:17 > À : [email protected] > Cc : [email protected]; [email protected]; > [email protected] > Objet : Yangdoctors early review of draft-ietf-softwire-dslite-yang-02 > > Reviewer: Mahesh Jethanandani > Review result: On the Right Track > > Document reviewed: draft-ietf-softwire-dslite-yang-02. Do not know why I > was > assigned -02 version when I now notice that there are several 03 versions > submitted all the way up to -06. > > Status: On the Right Track. > > I am not an expert in DS-Lite. This review is looking at the draft from a > YANG > perspective. With that said, I have marked it as “On the Right Track” > because > of some of the points discussed below. The authors are encouraged to spend > time > looking at other models for examples on how to write the model, read > RFC6087, > Guidelines for YANG documents. > > Summary: > > This document defines a YANG data model for the DS-Lite Address Family > Transition Router (AFTR) and Basic Bridging BroadBand (B4) elements . > > Overall Comments: > > The draft should refer to YANG 1.1 (RFC 7950) instead of RFC6020. [Med] Can fix this one. > > The draft is not NMDA compliant. It carries a separate -state container > that > needs to be collapsed into one single container. For example, it carries > containers such as aftr-current-config which seems to be carrying state > information for the leafs in dslite-config. [Med] There are no such containers in -06. > > Is it given that a server will implement both AFTR and B4? [Med] No. If not, then > please > define feature statements and use if-feature for each part (AFTR and B4), > so > implementors can choose what part of the model is implemented. > > Section 1.1 Terminology > > What terms are defined in RFC 6333 that are being used in this draft. > Please be > explicit. [Med] Actually, all the terms defined in Section 3 of RFC 6333 are used in this document. I added an explicit pointer to Section 3. > > Section 1.2 Tree Diagram > > Please refer to > https://tools.ietf.org/html/draft-ietf-netmod-yang-tree-diagrams-01 > instead of > defining the nomenclature for tree diagram in the document. [Med] Done. Thanks. > > Section 2. > > s/can:/can [Med] There is no such occurrence in -06. > s/provisioned to the AFTR/provisioned on the AFTR/ [Med] There is no such occurrence in -06. > > The document is very light in the architectural description of the YANG > model > itself. Statements like “model supports enabling one or more instances of > the > AFTR function on a device” are obvious looking at the model. A more > reasonable > description would be why multiple instances need to be supported. The same > is > true for “AFTR instance”. The fact that the instance has been defined as a > list > assure that each instance can be provisioned with dedicated configuration > data, > and maintain its own data table. Again why the multiple instances need to > be > maintained, what purpose do they serve, and the fact that they maintain > their > own data table is not clear. > > The document “assumes [RFC4787] [RFC5382][RFC 5508] are enabled”. What > particular aspects of those documents is the draft assuming? Note, RFC4787 > is > NAT Behavioral Requirements for Unicast UDP, and outlines some seven > behaviors, > all the way from NAT and Port Translation, Filtering, Hairpinning to > Fragmentation of Outgoing Packets. Even if all the behaviors are assumed, > it > would be good to know how they impact this particular model. [Med] What it meant is that all the recommendations are supported + all recommended values are the ones defined in those RFCs. > > Section 3 > > General Comments. > > Please follow [RFC6087], Guidelines for YANG documents, on how to write a > YANG > model. For example, the organization in the model should be IETF Softwire > WG, > not just Softwire WG. > > The indentation in the model is off in a few places (reference is a > keyword > that should be at the same indentation as description in all revision > statements). Most models follow an indentation of 2 characters for every > subsequent depth in the model. I see anywhere between 2 to 10 characters > of > indentation. > [Med] Will double check. Thanks. > As far as naming conventions go, there is little value repeating names in > a > given hierarchy. For example, if the grouping is aftr-parameters, then it > is a > given that parameters inside the grouping are aftr parameters. No need to > say > dslite-aftr-ipv6-address, where both dslite and aftr are redundant. > [Med] Please note that this does not apply for -06. > There is little or no use of must statements to qualify the configuration. > I am > not an expert at DS-Lite, so I cannot suggest where a must statement may > be > helpful to have. But with all the parameters that are optional, there must > be > some relationship between the attributes that could be captured with must > statements. For example, for the leaf max-softwire-per-subscriber, the > leaf is > trying to prevent a misbehaving subscriber. Could that behavior be somehow > captured with a must statement for the resources it can/cannot consume. Or > with > port-presevation-enable and port-parity-preservation-enable? Is there a > relationship between the two variables that could be captured in the > model? > > All references to RFC should be moved under a reference section. [Med] Please check -06. All references are under a reference section. For > example: > OLD: > leaf udp-lifetime { > type uint32; > units "seconds"; > default 120; > description > "UDP inactivity timeout [RFC4787]."; > } > NEW: > leaf udp-lifetime { > type uint32; > units "seconds"; > default 120; > description > "UDP inactivity timeout.“; > reference > “[RFC4787].”; > } > > A note should be prepared for the RFC editor to indicate what revision the > model should finally carry, and what the description/reference statement > in the > model should look like when the document is finally published. See > examples in > other drafts such as draft-ietf-netconf-zerotouch Editorial Note on what > to add. > > Specific Comments > > The name of the file in the line with <CODE BEGINS> is missing the .yang > suffix > in the file. When extracted, the extracted file will be missing the > suffix. > Please add it. This leads to the following error from yanglint. [Med] This was fixed in previous version. > > ietf-dslite@2017-01-03" without file extension - unknown format. > > In grouping port-number, could the port-range be something as simple as > > container port-number { > must “starting-port”; > leaf starting-port { > type inet:port-number; > } > leaf ending-port { > type inet:port-number; > } > } > > where if there is no ending-port it is deemed to be a single port, but if > there > is a ending-port is considered to be a range? [Med] This does not apply for -06. > > The list transport-protocol has one leaf which is the transport-protocol- > id. > Why does the single leaf, which is the key, need to be inside a list? [Med] Idem as above. > > Logging-info has a choice statement for protocol. Is it that there can be > only > one choice for the form of logging? Could it be possible that users might > want > to enable both syslog and ipfix? [Med] Idem as above. > > A pyang compilation of the model with —ietf and —lint option was clean. > > A idnits run of the draft reveals some issues with spacing and references. > The > one related to spacing are parts of the diagram, which confuses idnits. [Med] Will double check those. Thanks. > > Checking nits according to https://www.ietf.org/id-info/checklist : > ------------------------------------------------------------------------ > ---- > > ** There are 5 instances of too long lines in the document, the longest > one > being 3 characters in excess of 72. > > == There are 2 instances of lines with non-RFC6890-compliant IPv4 > addresses > in the document. If these are example addresses, they should be > changed. > > Miscellaneous warnings: > ------------------------------------------------------------------------ > ---- > > == Line 162 has weird spacing: '...ocol-id uin...' > > == Line 207 has weird spacing: '...address ine...' > > == Line 215 has weird spacing: '...address ine...' > > == Line 238 has weird spacing: '...rvation boo...' > > == Line 261 has weird spacing: '...ocol-id uin...' > > == (5 more instances...) > > Checking references for intended status: Proposed Standard > ------------------------------------------------------------------------ > ---- > > (See RFCs 3967 and 4897 for information about using normative > references > to lower-maturity documents in RFCs) > > == Unused Reference: 'RFC7753' is defined on line 1988, but no explicit > reference was found in the text > > == Outdated reference: A later version (-04) exists of > draft-boucadair-pcp-yang-03 > > Summary: 1 error (**), 0 flaws (~~), 9 warnings (==), 1 comment (--). > _______________________________________________ Softwires mailing list [email protected] https://www.ietf.org/mailman/listinfo/softwires
