Early Review of draft-ietf-teas-yang-te-21
review-ietf-teas-yang-te-21-yangdoctors-early-krejci-2019-06-13-00

Request Review of draft-ietf-teas-yang-te-21
Requested rev. 21 (document currently at 21)
Type Early Review
Team YANG Doctors (yangdoctors)
Deadline 2019-06-24
Requested 2019-05-31
Requested by Lou Berger
Draft last updated 2019-06-13
Completed reviews Yangdoctors Early review of -21 by Radek Krejčí
Comments
Early review in preparation for LC - note this is a very large module, so may require more time to review.
Assignment Reviewer Radek Krejčí
State Completed
Review review-ietf-teas-yang-te-21-yangdoctors-early-krejci-2019-06-13
Posted at https://mailarchive.ietf.org/arch/msg/yang-doctors/sOWRqaYYxxFYyhGX9pPJfQMVUJo
Reviewed rev. 21
Review result On the Right Track
Review completed: 2019-06-13

Review
review-ietf-teas-yang-te-21-yangdoctors-early-krejci-2019-06-13

This is my YANG doctor review of draft-ietf-teas-yang-te-21 containing 2 YANG modules:
- ietf-te@2019-04-09.yang
- ietf-te-device@2019-04-09.yang
The draft and both modules follows NMDA. 

Since I'm not expert in the area, the review focuses on YANG-specifics.


Draft text
----------
1. The second paragraph in 3.2  - items of the list are not properly formatted.


Both YANG modules
-----------------

2. Some of the multiline strings (descriptions) are not correctly formatted, there is:

     "some
     text";

   instead of

     "some
      text";

   Also better consistency in writing description as a sentence (with capital first letter and full stop at the end) or not would be nice.

3. Remove WG Chairs from contact information (see https://tools.ietf.org/html/rfc8407#appendix-B)


ietf-te@2019-04-09.yang
-----------------------

4. grouping path-properties: path-properties/path-route-objects/path-computed-route-objects
   - The list is defined as ordered-by user, but as a config false item (inherited from path-route-objects), the ordered-by statement is ignored (RFC 7950: 7.7.7.).

5. grouping p2p-path-properties-common :path-computation-server
   grouping p2p-path-properties-common :use-path-computation
   - Use derived-from-or-self() in when-stmt expression instead of comparing string values (RFC 8407: 4.6.2.).

6. There is a number of single-instantiated groupings:
   - p2p-reverse-path-properties
   - p2p-primary-reverse-path-properties
   - p2p-secondary-reverse-path-properties
   - p2p-primary-path-properties
   - p2p-secondary-path-properties
   - hierarchical-link-properties
   - protection-restoration-properties-state
   - ... (mybe the most of all that groupings) 
   While it is a good mechanism to allow repeated instantiation of a group of statements, it can produce kind of "spaghetti code" - with such a number of groupings, it is quite challenging for human readers to follow the schema. And one of the YANG advantages is its readability. Please consider, if there is some real possibility to reuse groupings somewhere outside the schema itself. If yes, then ok - readability is the price for reuseability, but otherwise you actually don't get anything for poor readability.

7. grouping p2p-reverse-path-properties: named-path-constraint
   grouping p2p-path-properties: named-path-constraint
   - Use absolute leafref path, in this case you probably don't want to refer to something relative to the grouping (or something in the grouping), but specifically to /te/globals/... using relative path in this case really complicates ability to reuse the grouping somewhere else. Maybe in some cases this can be also an indicator of a grouping which is actually not supposed to be a separate grouping.
   - Similarly in the following nodes:
   grouping p2p-dependency-tunnels-properties: dependency-tunnels/dependency-tunnel/name
   grouping p2p-path-candidate-secondary-path-config: secondary-path
   ...

8. grouping protection-restoration-properties-state: lockout-of-normal
   grouping protection-restoration-properties-state: freeze
   - Weird formatting of the descriptions with the first and last empty lines.

9. grouping protection-restoration-properties: protection/hold-off-time
   grouping protection-restoration-properties: restoration/hold-of-time
   - Is there any compatibility reason to have 'milli-seconds' units? If not, change it to milliseconds.

10. grouping tunnel-p2p-associations-properties: association-objects(-extended)
    - As mentioned, I'm not an expert in the area, but what is the global-source (in contrast to source) and why is it a key of the lists? I didn't find it mentioned in RFC4872, so the reference is wrong.
    - Use lower case for the node identifiers (ID, extended-ID - other *-id identifiers in the document are lowercase)

11. RPCs and Notifications
    - From the descriptions, it is unclear what exactly the RPCs are supposed to do and about what the Notifications are supposed to notify. The description "TE tunnels RPC nodes" is completely useless.


ietf-te-device@2019-04-09.yang
------------------------------

12. grouping tunnel-device-config
    - It is not very clear why this grouping is actually defined, why other modules should import and instantiate this single-leaf grouping? At least the description must be improved.
    - Similarly, what are the use cases for interfaces-rpc and (empty) interfaces-notif (as 11)?
  
13. grouping te-admin-groups-config: admin-group-type/named-admin-groups/named-admin-groups/named-admin-group
    grouping te-srlgs-config: srlg-type/named-srlgs/named-srlgs/named-srlg
    - Same as 7.
  
14. grouping te-srlgs-config: srlg-type/value-srlgs/values/value
    - The range actually covers whole range of the type itself.