[Teas] Yangdoctors last call review of draft-ietf-teas-yang-te-topo-08

[Mahesh Jethanandani <[e-mail]>]

Reviewer: Mahesh Jethanandani
Review result: Ready with Issues

Document reviewed: draft-ietf-teas-yang-te-topo-08

Status: Ready with Issues

I am not an expert in Traffic Engineering. This review is looking at
the draft from a YANG perspective. With that said, I have marked it as
“Ready with Issues” because of some of the points discussed below.

Summary:

This document defines a YANG data model for representing, retrieving
and manipulating TE Topologies. The model serves as a base model that
other technology specific TE Topology models can augment.

Comments:

Almost all the containers in the model are presence containers. Is
there a reason why they have to be presence containers? Note, that
presence containers are containers whose existence itself represents
configuration data. What particular configuration data is each
container representing in itself?

It is difficult to co-relate the diagram with the model itself because
of different terms being used to define different parts of the model.
There is “TE Topology Model” and then there is “Generic TE Topology
Model”. Are these one and the same models? If so, a common term for
both of them would be helpful.

There is extensive use of groupings in the document. However, not all
instances of groupings are used multiple number of times. Where they
are not being repeated, it would be better to move the grouping
directly where the uses statement resides. Case in point the grouping
connectivity-label-restriction-list.

The split between config and state containers does not seem to follow
any particular pattern. It is neither a top level split, as is the
case with existing IETF models, nor do they follow the OpenConfig
style of splitting config and state under each relevant leaf, nor do
they follow the new guidelines as suggested by the new datastore model
draft
(https://tools.ietf.org/html/draft-dsdt-nmda-guidelines-01.html).
Since there recommendation for all new models is to follow the new
datastore guidelines, would suggest that the state containers be
removed and any non-duplicate leafs within state be moved under a
single container. I am told that the change in ietf-te-topology from
the current draft to NMDA style is about 175 lines and for te-types it
is 24 line diff (thanks Robert). As such, the change is not major and
can be done easily. See the diffs at the end of this review.

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, but they are parts of the diagram, which confuses idnits.

te-types.diff
—————
507,509c507,509
<   grouping explicit-route-hop_config {
<     description
<       "The explicit route subobject grouping";
---
>   grouping explicit-route-hop {
>     description "Explicit route hop grouping";
> 
595,609d594
<     }
<   }
< 
<   grouping explicit-route-hop {
<     description "Explicit route hop grouping";
<     container config {
<       description
<         "Configuration parameters for the explicit route hop";
<       uses explicit-route-hop_config;
<     }
<     container state {
<       config false;
<       description
<         "State parameters for the explicit route hop";
<       uses explicit-route-hop_config;

te-topology.diff
——————-
264a265
>       config false;
323a325
>       config false;
327a330
>       config false;
357a361
>       config false;
361a366
>       config false;
733,744c738,739
<       container config {
<         description
<           "Configuration data.";
<         uses te-link-config;
<       } // config
<       container state {
<         config false;
<         description
<           "Operational state data.";
<         uses te-link-config;
<         uses te-link-state-derived;
<       } // state
---
>       uses te-link-config;
>       uses te-link-state-derived;
780,781c775,776
<                 path "../../../../../../nw:node[nw:node-id = "
<                   + "current()/../../../../../nt:source/"
---
>                 path "../../../../../nw:node[nw:node-id = "
>                   + "current()/../../../../nt:source/"
792,793c787,788
<                 path "../../../../../../nw:node[nw:node-id = "
<                   + "current()/../../../../../nt:destination/"
---
>                 path "../../../../../nw:node[nw:node-id = "
>                   + "current()/../../../../nt:destination/"
841c836
<         path "../../../../../te/templates/link-template/name";
---
>         path "../../../../te/templates/link-template/name";
1087a1083
>       config false;
1092a1089
>       config false;
1106a1104
>       config false;
1113a1112
>       config false;
1128a1128
>       config false;
1267,1279c1267,1268
<       container config {
<         description
<           "Configuration data.";
<         uses te-node-config;
<       } // config
<       container state {
<         config false;
<         description
<           "Operational state data.";
< 
<         uses te-node-config;
<         uses te-node-state-derived;
<       } // state
---
>       uses te-node-config;
>       uses te-node-state-derived;
1297,1309c1286,1287
<         container config {
<           description
<             "Configuration data.";
<           uses te-node-tunnel-termination-attributes;
<         }
<         container state {
<           config false;
<           description
<             "Operational state data.";
< 
<           uses te-node-tunnel-termination-attributes;
<           uses geolocation-container;
<         } // state
---
>         uses te-node-tunnel-termination-attributes;
>         uses geolocation-container;
1406c1384
<         path "../../../../../te/templates/node-template/name";
---
>         path "../../../../te/templates/node-template/name";
1473,1474c1451
<               path "../../../../../../../nt:termination-point/"+
<                 "nt:tp-id";
---
>               path
"../../../../../../nt:termination-point/nt:tp-id";
1485,1486c1462
<               path "../../../../../../../nt:termination-point/"+
<                 "nt:tp-id";
---
>               path
"../../../../../../nt:termination-point/nt:tp-id";
1577a1554
>       config false;
1583a1561
>       config false;
1595a1574
>       config false;
1738c1717
<             path "../../../../../../nt:termination-point/nt:tp-id";
---
>             path "../../../../../nt:termination-point/nt:tp-id";
1753c1732
<     uses te-types:explicit-route-hop_config;
---
>     uses te-types:explicit-route-hop;
1773,1779c1752,1754
<       container config {
<         description
<           "Configuration data.";
<         uses te-termination-point-config;
<       } // config
<       container state {
<         config false;
---
>       uses interface-switching-capability-list;
>       leaf inter-layer-lock-id {
>         type uint32;
1781,1784c1756,1765
<           "Operational state data.";
<         uses te-termination-point-config;
<         uses geolocation-container;
<       } // state
---
>           "Inter layer lock ID, used for path computation in a TE
>            topology covering multiple layers or multiple regions.";
>         reference
>           "RFC5212: Requirements for GMPLS-Based Multi-Region and
>            Multi-Layer Networks (MRN/MLN).
>            RFC6001: Generalized MPLS (GMPLS) Protocol Extensions
for
>            Multi-Layer and Multi-Region Networks (MLN/MRN).";
>       }
> 
>       uses geolocation-container;
1787,1802d1767
<   grouping te-termination-point-config {
<     description
<       "TE termination point configuration grouping.";
<     uses interface-switching-capability-list;
<     leaf inter-layer-lock-id {
<       type uint32;
<       description
<         "Inter layer lock ID, used for path computation in a TE
<          topology covering multiple layers or multiple regions.";
<       reference
<         "RFC5212: Requirements for GMPLS-Based Multi-Region and
<          Multi-Layer Networks (MRN/MLN).
<          RFC6001: Generalized MPLS (GMPLS) Protocol Extensions
<          for Multi-Layer and Multi-Region Networks (MLN/MRN).";
<     }
<   } // te-termination-point-config
1879,1890c1844,1845
<       container config {
<         description
<           "Configuration data.";
<         uses te-topology-config;
<       } // config
<       container state {
<         config false;
<         description
<           "Operational state data.";
<         uses te-topology-config;
<         uses geolocation-container;
<       } // state
---
>       uses te-topology-config;
>       uses geolocation-container;