ietf
[Top] [All Lists]

RE: Yangdoctors last call review of draft-ietf-teas-yang-te-topo-08

2017-06-12 16:28:31
Hi Mahesh,

Thank you much for the review. We have submitted an updated draft 
(https://tools.ietf.org/html/draft-ietf-teas-yang-te-topo-09) to address these 
issues. More detailed explanations are put below inline. 

If the responses and updates are satisfactory, we are ready for the last call.

Best regards,
- Xufeng

-----Original Message-----
From: Mahesh Jethanandani [mailto:mjethanandani(_at_)gmail(_dot_)com]
Sent: Wednesday, May 24, 2017 11:44 AM
To: yang-doctors(_at_)ietf(_dot_)org
Cc: ietf(_at_)ietf(_dot_)org; teas(_at_)ietf(_dot_)org; 
draft-ietf-teas-yang-te-topo(_dot_)all(_at_)ietf(_dot_)org
Subject: Yangdoctors last call review of draft-ietf-teas-yang-te-topo-08

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?
[Xufeng] Containers that use “presence” are:
        - Container “underlay”
          o  We have changed 13 occurrences of such containers to be not 
presence container. 
        - Container “te” under augmentation
          o  To indicate that “TE” is enabled (configuration data)
          o  Also used to do augmentation. The “presence” statement can prevent 
the mandatory child from affecting augmented base model.
        - /nw:networks/nw:network/nw:network-types/te-topology!
          o  A mechanism required by I2RS topology model to specify the 
topology type.


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.
[Xufeng] Yes. These two terms are the same. Figure 12, Figure 13, and relevant 
descriptions have been updated to make the document consistent.


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.
[Xufeng] We have removed the following groupings
    te-link-augment
    te-node-augment
    te-termination-point-augment
    te-topologies-augment
    te-topology-augment
    te-link-state-underlay-attributes
    te-node-state-derived-notification
    te-topology-type

The remaining groupings have been kept so that we can:
        - Share the groupings in this model
        - Prepare to be shared by a model augmenting this model
        - Prevent a grouping or configuration section from being too long
        - Improve readability


The split between config and state containers does not seem to follow any
particular pattern. 
[Xufeng] The pattern is clear:
For each manageable entity (object), there is a config container and state 
container. The configurable properties go into the config container and state 
properties go into the state container. Such objects are identified by a list 
item or a presence container so that the “create”, “delete”, and “modify” 
operations can be performed on them. The non-presence containers do not 
represent configuration data so they do not introduce such objects.

It is neither a top level split, as is the case with existing IETF
models, 
[Xufeng] We could not do top level split because the base I2RS network topology 
model (https://tools.ietf.org/html/draft-ietf-i2rs-yang-network-topo-12) that 
we augment does not have the top-level split (for its own reasons).

nor do they follow the OpenConfig style of splitting config and state
under each relevant leaf, 
[Xufeng] The pattern is consistent with this style in principle, with some 
adjustments to fit to our multiple levels of hierarchy.

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.
[Xufeng] The changes are simple, but the implications are not:
        o  Need to re-open the discussions for the I2RS network topology model 
(https://tools.ietf.org/html/draft-ietf-i2rs-yang-network-topo-12), which 
currently does not have the state branch, and is the base model that this model 
augments.
        o  Need to re-discuss the issues of cross referencing between config 
branch and state branch (which was brought up by I2RS model and has no good 
solution currently).
        o  Need to modify existing and on-going implementations with no 
functional benefits.
        o  Will delay publishing both the I2RS base network topology model and 
this document.

This proposed model currently has multiple on-going implementations. 
Implementers wish for advancing this document without unnecessary delays, and 
before the final NETCONF/RESTCON protocol support which is expected to be 
available in a year. 

The authors and contributes have discussed the adoption of the NMDA style, and 
currently prefer the following strategy:
        o  Publish the document with the current model style without structure 
changes, allowing the on-going implementations to continue, supporting 
necessary operational states before the NMDA protocol specification is updated 
and supporting implementations are available. 
        o  Follow this up ASAP with an NMDA-compatible model draft (WG could 
adopt this right away). This was suggested by Kent, and is considered to fit 
our situation the best, allowing the implementations to migrate to the long 
term approach without interruption.


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;