Re: [Softwires] Yangdoctors early review of draft-ietf-softwire-dslite-yang-02

[<[e-mail]>]

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:[e-mail]]
> Envoyé : samedi 7 octobre 2017 02:17
> À : [e-mail]
> Cc : [e-mail]; [e-mail];
> [e-mail]
> 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 (--).
>