Early Review of draft-ietf-pce-pcep-yang-08
review-ietf-pce-pcep-yang-08-yangdoctors-early-jethanandani-2018-11-26-00

Request Review of draft-ietf-pce-pcep-yang
Requested rev. no specific revision (document currently at 13)
Type Early Review
Team YANG Doctors (yangdoctors)
Deadline 2018-11-17
Requested 2018-10-16
Requested by Julien Meuric
Authors Dhruv Dhody, Jonathan Hardwick, Vishnu Beeram, Jeff Tantsura
Draft last updated 2018-11-26
Completed reviews Yangdoctors Early review of -08 by Mahesh Jethanandani (diff)
Assignment Reviewer Mahesh Jethanandani
State Completed
Review review-ietf-pce-pcep-yang-08-yangdoctors-early-jethanandani-2018-11-26
Reviewed rev. 08 (document currently at 13)
Review result On the Right Track
Review completed: 2018-11-26

Review
review-ietf-pce-pcep-yang-08-yangdoctors-early-jethanandani-2018-11-26

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.