Re: [yang-doctors] Yangdoctors early review of draft-ietf-i2rs-rib-data-model-09

[Ebben Aries <[e-mail]>]

On Feb 23 19:31 PM, Ebben Aries wrote:
> Amit,
> 
> I took your latest pre -11 .xml revision that you attached so below is a
> re-review against -10 and pre (-11).  Comments below
> 
> 
> Compilation error that was introduced between -10 and -11
> - [e-mail]:1224: error: unexpected keyword "range"
> 
>     leaf hop-limit {
>       type uint8;
>             range "1..255";
>       description
>         "The path hop limit header.";
>     }
> - Also, by introducing a range on a uint8, are you saying that a hop-limit=0
>   would never be in use for any use-case here?
> 
> There are formatting and alignment issues all throughout the document that
> need fixing (Mahesh pointed this out as well)
> 
> 'ietf-interfaces' import carries the wrong RFC reference (RFC 7227)
> - This should rather be RFC 7277
> 

I meant RFC 7223 rather

  import ietf-interfaces {
      prefix if;
      reference "RFC 7223: A YANG Data Model for Interface Management";
  }

> There are still ongoing discussions among yang-doctors on how references in
> imports should be treated.  At a minimum for now, lets carry the expanded
> syntax w/ the title of the RFC as such: (As Mahesh previous mentioned, this is
> subject to change pending further discussion)
> - reference "RFC 6991: Common YANG Data Types";
> - reference "RFC 7277: A YANG Data Model for IP Management";
> - reference "RFC 6991: Common YANG Data Types";
> 
> Module description still does not carry full copyright/distribution section as
> specified in: https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D15-23appendix-2DC&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=oqu2ooyPcpQ1BsLe7O0eVojE-sP-YgMoHTpQoRTH954&s=nZJnXRNh9vlo9VZexb3zXgyY6mCzlN1ztsrmPjezh50&e=
> 
> There still appears to be one remaining 'Rib' reference
> - s/Routing Instance and Rib/Routing Instance and RIB/
> 
> Section 1.2 - Tree Diagrams
> - The text here is an instruction template rather than what the actual text
>   should be.  It should rather read:
> 
>    A simplified graphical representation of the data model is used in
>    this document.  The meaning of the symbols in these diagrams is
>    defined in [I-D.ietf-netmod-yang-tree-diagrams].
> 
> You did not address my previous comment/question re: NMDA (Tom mentioned this
> as well)
> 
> You did not address my previous comment/question re: Section 2.5 (route-add
> RPC)
> 
> I still have concern to modules introducing large amounts of
> features/if-features as previously noted but will defer to other's comments on
> this as well
> 
> Formatting nits:
> - L3605: s/route:A/route: A/
> - L3610: s/from\/to RIB/from\/to a RIB/
> - L3610: s/lead to suboptimal/lead to a suboptimal/
> - L3611: s/possibly/possible/
> - L3626: s/his/their/
> 
> Tom's previous comment re: ENABLE_IP_RPF_CHECK has not been addressed and
> exists in both -10 and (pre) -11 versions
> 
> Thx
> 
> /ebben
> 
> On Feb 20 15:24 PM, Amit Dass wrote:
> > Hi Ebben,
> > 
> > I have updated the draft based on your comments. Could you please have a look at the same and provide your feedback?
> > 
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__datatracker.ietf.org_doc_draft-2Dietf-2Di2rs-2Drib-2Ddata-2Dmodel_&d=DwIGaQ&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=Bd9RZ2mCWMYNPyhFBtSbqwApeVo8_SOmuA3FNG3trxA&s=h0scwYvh7UsfradRz7u1meraj47zZhr6vH3qiHJXiZ4&e=
> > 
> > Best regards,
> > Amit
> > 
> > -----Original Message-----
> > From: Ebben Aries [mailto:[e-mail]] 
> > Sent: Thursday, January 18, 2018 9:33 AM
> > To: [e-mail]
> > Cc: [e-mail]; [e-mail]; [e-mail]
> > Subject: Yangdoctors early review of draft-ietf-i2rs-rib-data-model-09
> > 
> > Reviewer: Ebben Aries
> > Review result: On the Right Track
> > 
> > 1 module in this draft:
> > - [e-mail]
> > 
> > No YANG validation errors or warnings (from pyang 1.7.3 and yanglint 0.14.59)
> > 
> > 0 examples are provided in this draft (section 3.12 of
> > draft-ietf-netmod-rfc6087bis-15)
> > 
> > Module [e-mail]:
> > - yang-version statement missing - should be 1.1
> > - prefix 'iir' is recommended for this module, would 'rib' suffice better?
> > - import "ietf-inet-types" should reference RFC 6991 per (not as a comment)
> >   https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D15-23section-2D4.7&d=DwIGaQ&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=Bd9RZ2mCWMYNPyhFBtSbqwApeVo8_SOmuA3FNG3trxA&s=dYhxw9bO0Uk78WJ5uTBmc5ZcElOT446ETFp7DXU546s&e=
> > - import "ietf-interfaces" should reference RFC 7223 per
> >   https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D15-23section-2D4.7&d=DwIGaQ&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=Bd9RZ2mCWMYNPyhFBtSbqwApeVo8_SOmuA3FNG3trxA&s=dYhxw9bO0Uk78WJ5uTBmc5ZcElOT446ETFp7DXU546s&e=
> > - import "ietf-yang-types" should reference RFC 6991 per
> >   https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D15-23section-2D4.7&d=DwIGaQ&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=Bd9RZ2mCWMYNPyhFBtSbqwApeVo8_SOmuA3FNG3trxA&s=dYhxw9bO0Uk78WJ5uTBmc5ZcElOT446ETFp7DXU546s&e=
> > - Since this module imports "ietf-interfaces", a normative references must be
> >   added per
> >   https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D15-23section-2D3.9&d=DwIGaQ&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=Bd9RZ2mCWMYNPyhFBtSbqwApeVo8_SOmuA3FNG3trxA&s=IaSTmgPV-jaH8vLp0nasBDWLSQ9-dDRqYwAkIvsNfkY&e=
> > - prefix "if" in the import "ietf-interfaces" can remove quotes to remain
> >   consistent with other imports
> > - Remove WG Chairs from contact information per
> >   https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D15-23appendix-2DC&d=DwIGaQ&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=Bd9RZ2mCWMYNPyhFBtSbqwApeVo8_SOmuA3FNG3trxA&s=1U0JCnUyX_pqexjCB9KgzvhWsdLDQMN_iOgkNv29dEs&e=
> > - Module description must contain most recent copyright notice per
> >   https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D15-23appendix-2DC&d=DwIGaQ&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=Bd9RZ2mCWMYNPyhFBtSbqwApeVo8_SOmuA3FNG3trxA&s=1U0JCnUyX_pqexjCB9KgzvhWsdLDQMN_iOgkNv29dEs&e=
> > - Module description should contain note to RFC Ed. and placeholder reference
> >   to RFC when assigned
> >   https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D15-23appendix-2DC&d=DwIGaQ&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=Bd9RZ2mCWMYNPyhFBtSbqwApeVo8_SOmuA3FNG3trxA&s=1U0JCnUyX_pqexjCB9KgzvhWsdLDQMN_iOgkNv29dEs&e=
> > - Add placeholder reference and note to RFC Ed. for RFC when assigned
> >   https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D15-23appendix-2DC&d=DwIGaQ&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=Bd9RZ2mCWMYNPyhFBtSbqwApeVo8_SOmuA3FNG3trxA&s=1U0JCnUyX_pqexjCB9KgzvhWsdLDQMN_iOgkNv29dEs&e=
> > - Security Considerations should be updated to reflect new template at
> >   https://urldefense.proofpoint.com/v2/url?u=https-3A__trac.ietf.org_trac_ops_wiki_yang-2Dsecurity-2Dguidelines&d=DwIGaQ&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=Bd9RZ2mCWMYNPyhFBtSbqwApeVo8_SOmuA3FNG3trxA&s=tfCcnSFoI3jjNNzCj6pgkp94JZ-quOQEqMlv9hITBog&e=
> > - Section 1.2 should be replaced with reference to
> >   draft-ietf-netmod-yang-tree-diagrams-02 rather (as-is in other i2rs YANG
> >   drafts in progress) per
> >   https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D15-23section-2D2.5.1&d=DwIGaQ&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=Bd9RZ2mCWMYNPyhFBtSbqwApeVo8_SOmuA3FNG3trxA&s=DjKFw7t6_CU_2_03rwIHxgvz5_oC5mjX_rMUC5UuV6M&e=
> > - This module contains '12' features.  While it is understood the purpose of
> >   these features in the module, take precaution as to complexity for clients
> >   if they need to understand >= quantity of features per module in use on a
> >   network-element.
> > - A few comments exist that are either unecessary or redundant.  Encode the
> >   comment intent rather in description fields if need be.
> > - Per NMDA, which datastores are targeted for the module?  Will all RPC
> >   operations be acting upon the dynamic/ephemeral datastore?  It is not clear
> >   to me if the intention is to be persistent or ephemeral
> > 
> > General comments/Nits:
> > - references to 'def' could be expanded out to 'definition'
> > - references to 'decap' could be expanded out to 'decapsulation' for
> >   readability (across definitions and descriptions)
> > - Follow consistent capitalization of 'RIB' throughout document text.  Mixed
> >   use of 'Rib' and 'rib' exists (Outside of YANG node lowercase definitions).
> > - Is it necessary to prefix all nodes under the nexthop container with
> >   "nexthop-"?
> > - Section 2.5 - route-add RPC - text mentions it is required that the nh-add
> >   RPC be called as a pre-requisite however if the nh already exists and the
> >   nexthop-id is known, this should not be necessary.  In addition, the text
> >   reads 'or return' which should rather be a result of querying the
> >   appropriate node in the data tree.
> > - In 'IANA Considerations' - s/This document requests to register/This
> >   document registers/
> > 
> 
> _______________________________________________
> yang-doctors mailing list
> [e-mail]
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.ietf.org_mailman_listinfo_yang-2Ddoctors&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=oqu2ooyPcpQ1BsLe7O0eVojE-sP-YgMoHTpQoRTH954&s=GTV9rhtmIT5nIY6w8DKgR9QiRak314uAIzfjhtj_z-k&e=