Early Review of draft-ietf-teas-yang-l3-te-topo-04
review-ietf-teas-yang-l3-te-topo-04-yangdoctors-early-jethanandani-2019-06-18-00

Request Review of draft-ietf-teas-yang-l3-te-topo-04
Requested rev. 04 (document currently at 05)
Type Early Review
Team YANG Doctors (yangdoctors)
Deadline 2019-06-24
Requested 2019-05-31
Requested by Lou Berger
Authors Xufeng Liu, Igor Bryskin, Vishnu Beeram, Tarek Saad, Himanshu Shah, Oscar de Dios
Draft last updated 2019-06-18
Completed reviews Yangdoctors Early review of -04 by Mahesh Jethanandani (diff)
Comments
Early review in preparation for WG LC
Assignment Reviewer Mahesh Jethanandani
State Completed
Review review-ietf-teas-yang-l3-te-topo-04-yangdoctors-early-jethanandani-2019-06-18
Posted at https://mailarchive.ietf.org/arch/msg/yang-doctors/8objExbhLzhYw5Y4q5bnUkRQtg8
Reviewed rev. 04 (document currently at 05)
Review result On the Right Track
Review completed: 2019-06-18

Review
review-ietf-teas-yang-l3-te-topo-04-yangdoctors-early-jethanandani-2019-06-18

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 (--).