Re: [Pce] Yangdoctors early review of draft-ietf-pce-pcep-yang-08

[Dhruv Dhody <[e-mail]>]

Thanks Mahesh for your review. 

The authors would be back with an update and reply SOON. 

> -----Original Message-----
> From: Pce [mailto:[e-mail]] On Behalf Of Mahesh Jethanandani
> Sent: 27 November 2018 08:29
> To: [e-mail]
> Cc: [e-mail]; [e-mail]; [e-mail]
> Subject: [Pce] Yangdoctors early review of draft-ietf-pce-pcep-yang-08
> 
> Reviewer: Mahesh Jethanandani
> Review result: On the Right Track
> 
> Document reviewed: draft-ietf-pce-pcep-yang-08
> 
> I am not an expert in PCEP. 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 the management of Path
> Computation Element communications Protocol (PCEP) for communications
> between a Path Computation Client (PCC) and a Path Computation Element
> (PCE), or between two PCEs.  The data model includes configuration data
> and state data (status information and counters for the collection of
> statistics).
> 
> Comments:
> 
> General
> 
> - The module uses indentation that varies all over the module, from 2
> spaces to 5. Please fix the module to have consistent indentation.
> 
> - The module makes heavy use of groupings. They are great if they are
> being used in multiple places. But I seem to see single usage of
> groupings, which makes the model hard to read. Please collapse all
> groupings that are used only once into the module.
> 
> Abstract:
> 
> It is best not to try to redefine terms, specially if they have already
> been defined already in another RFC. Case in point, "state data". This
> term has been defined in RFC6241, and it would be best to list it in the
> Terminology and Notation section, as has been done with other definitions.
> 
> The following terms are defined in [RFC6241]:
> 
>    o  configuration data
> 
>    o  state data
> 
> Introduction:
> 
> Please update reference of YANG to RFC7950. These are YANG 1.1 modules
> after all.
> 
> Section 5. The Design of the PCEP Data Model.
> 
> Thank you for first of all for creating a abridged version of the tree
> diagram.
> What would really help to understand the design of the model would be to
> place the full tree diagram at the end of the section, and move sections
> 5.3 to 5.7.
> directly under 5.1. Scrolling through pages of the full diagram to get to
> the design sections is painful to read.
> 
> Section 10. PCEP YANG Modules
> 
> - Please list all RFCs and I-D that are referenced in the model, so there
> is a normative reference to them in the draft.
> 
> - Please expand the reference to different RFCs to include the title of
> the RFC, and not just the number.
> 
> - The reference to tls-server and tls-client should be to I-D.ietf-
> netconf-tls-client-server, as it is not an RFC as yet. Also, the document
> refers to all other RFCs as RFC XXXX. What is the RFC editor supposed to
> replace XXXX with? With the RFC number assigned to this draft?? I think
> you want to refer to I-D that contain those modules.
> 
> - What is "PCEP common"? That term has not been defined anywhere in the
> document, but is used in the YANG model.
> 
> - What is the 'identify pcep' for?
> 
> - Why is pcep-admin-status a enum and not a boolean? Since YANG nodes are
> hierarchical, there should be no reason to repeat prefixes like 'admin-
> status'
> in node names such as 'admin-status-up', both where it is defined and
> where it is used (under admin-status).
> 
> - Where are the different operational status definitions defined? Can that
> RFC be referenced? Same for Session state, Association Type, Objective
> Function.
> 
> - Could the YANG module use existing definitions? For example could the
> module use ospf-area as defined in I-D.ietf-ospf-yang or use isis-area
> defined in the ISIS YANG Module.
> 
> - Can the document use more descriptive names for features such as 'gco'.
> 
> - If the range of the timer is 1..65535, why does it need to be a uint32?
> Same for the range of 0..255.
> 
> - RFC 5440 makes no reference to 'max-keep-alive-timer' or 'max-dead-
> timer'. If they are max value, can they not be expressed as part of the
> range for 'keep-alive-timer' or 'dead-timer'? Same for 'min-keep-alive-
> timer' and 'min-dead-timer'.
> 
> - What is the default value for 'admin-status'?
> 
> - The grouping pce-scope seems to be defining a header with each of the
> leafs as bits in the header. In that case, it would be better if this was
> defined as a bits/bit field, rather than leafs that are of type uint8 and
> boolean. Same for the grouping called 'capability'
> 
> - The description "LSP is PCE-initiated or not" is hardly a description
> for the leaf 'enabled'. It might be more a description of the feature
> 'pce-initiated'.
> 
> - Could description "Valid at PCC" be improved upon?
> 
> - Most keys are defined as 'type binary'. Why is key-string defined as
> 'type string' or 'type hex-string', and not 'type binary'? Is it possible
> to reuse definitions from draft-ietf-netconf-crypto-types?
> 
> - I am not an expert in this protocol, but a lot of the nodes defined are
> generated by the system. Yet, they are defined as rw. For example, the
> list 'path-keys' carries a description "The list of path-keys generated by
> the PCE".
> If so, should this not be marked 'config false'. I would suggest authors
> take a more concerted look and see what nodes are indeed rw and which ones
> are ro.
> Other examples include 'req-id' and 'retrieved'.
> 
> - Can this error-message and description be reconciled?
> 
>                     error-message
>                         "The Path-key should be retreived";
>                     description
>                         "When Path-Key has been retreived";
> 
> - It is great to see that extensive amount of statistics are required to
> be implemented by the model. How many implementations actually support all
> these statistics? What would happen if implementations support a small
> number of these statistics? In other words, are all these statistics
> required to be maintained/implemented?
> 
> - In addition, a lot of the statistics have when statements. Since these
> are statistics maintained by the system, why the when statement? Does it
> mean that even if the statistics are written by the system, they are not
> valid (for
> reading) under certain scenarios. Or is it more likely that they are only
> written when the role is ether of a 'pce' or 'pcc-and-pce', in which case
> reading for other roles would return 0 values.
> 
> _______________________________________________
> Pce mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/pce