[Teas] Yangdoctors early review of draft-ietf-teas-yang-l3-te-topo-04
Mahesh Jethanandani via Datatracker <email@example.com> Wed, 19 June 2019 02:31 UTC
Received: from ietfa.amsl.com (localhost [IPv6:::1]) by ietfa.amsl.com (Postfix) with ESMTP id E655B1203A0; Tue, 18 Jun 2019 19:31:08 -0700 (PDT)
Content-Type: text/plain; charset="utf-8"
From: Mahesh Jethanandani via Datatracker <firstname.lastname@example.org>
Cc: email@example.com, firstname.lastname@example.org, email@example.com
Reply-To: Mahesh Jethanandani <firstname.lastname@example.org>
Date: Tue, 18 Jun 2019 19:31:08 -0700
Subject: [Teas] Yangdoctors early review of draft-ietf-teas-yang-l3-te-topo-04
List-Id: Traffic Engineering Architecture and Signaling working group discussion list <teas.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/teas>, <mailto:email@example.com?subject=unsubscribe>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/teas>, <mailto:firstname.lastname@example.org?subject=subscribe>
X-List-Received-Date: Wed, 19 Jun 2019 02:31:09 -0000
Reviewer: Mahesh Jethanandani Review result: On the Right Track I am not an expert in Traffic Engineering or in topology model. If my understanding of the network topology models, feel free to educate me and everyone else. 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. Summary: This document defines a YANG data model for layer 3 traffic engineering topologies. Nits This sentence construction does not sound right. Possibly add a 'that' between 'enforce' and 'the'. "YANG must statements are used to enforce the referenced objects are in the topologies of proper type." Some amount of consistency in writing description and presence statement would be nice. For example, the sentence should start with a capital letter. Description statements should start on the same line as the keyword 'description' or they should start on a new line. Pick one and stick with it through out the model. Comments: Having 19 pages of a tree diagram without any explanation for the different sections of the tree diagram is hardly helpful. Suggest that --tree-depth and --tree-path options be used in pyang to reduce the size of the tree and to break it up into smaller pieces that can then be explained. There seems to be two grouping with the same name (l3-te-topology-type), one defined in this module, and other imported. It would be better if they had different names. Same for l3-te-topology-attributes. Also, since the two grouping defined in the module are used exactly once, they should be inlined with where they are being used, instead of using the grouping/uses statement. The justification for using them in notifications as articulated in RFC 8346 does not hold true for this module. But perhaps what is most confusing is how the module has been architected. Per RFC 8345, this module should augment the ietf-network module, which it does. It inserts a new network-type, which is good. But then for any data nodes that are specific to this network-type have to be defined. The only thing this module defines is a network-ref (possibly to the underlay network). No name or id is defined. If one has to reference this particular node, how would one do it? Are you saying that the l3-topology-attributes, e.g. name, flag etc. are the same between ietf-l3-unicast-topology and ietf-l3-te-topology? That somehow referencing the ietf-l3-unicast-topology attributes will give me the ietf-l3-te-topology attributes?? And that the l3-flag-type is the same for both the underlay and the overlay?? The same is true for l3-node-attributes, l3-link-attributes and possibly l3-termination-point-attributes. A pyang compilation of the model with —ietf and —lint option was clean. However, a validation of the example reveals errors: err : Unknown element "provider-id". (/ietf-network:networks/network[network-id='example-topo-te']) A idnits run of the draft reveals a few issues. Please address them. Miscellaneous warnings: --------------------------------------------------------------------------- == The document seems to lack the recommended RFC 2119 boilerplate, even if it appears to use RFC 2119 keywords. (The document does seem to have the reference to RFC 2119 which the ID-Checklist requires). -- The document date (March 11, 2019) is 99 days in the past. Is this intentional? Checking references for intended status: Proposed Standard --------------------------------------------------------------------------- (See RFCs 3967 and 4897 for information about using normative references to lower-maturity documents in RFCs) == Missing Reference: 'RFC3688' is mentioned on line 1799, but not defined == Missing Reference: 'RFC6020' is mentioned on line 1826, but not defined ** Obsolete normative reference: RFC 5246 (Obsoleted by RFC 8446) ** Obsolete normative reference: RFC 6536 (Obsoleted by RFC 8341) ** Obsolete normative reference: RFC 7810 (Obsoleted by RFC 8570) ** Downref: Normative reference to an Informational RFC: RFC 7823 == Outdated reference: A later version (-09) exists of draft-ietf-teas-yang-te-types-06 == Outdated reference: A later version (-21) exists of draft-ietf-teas-yang-te-topo-19 Summary: 4 errors (**), 0 flaws (~~), 5 warnings (==), 1 comment (--).
- [Teas] Yangdoctors early review of draft-ietf-tea… Mahesh Jethanandani via Datatracker
- Re: [Teas] Yangdoctors early review of draft-ietf… Xufeng Liu