Early Review of draft-ietf-pce-pcep-yang-18
review-ietf-pce-pcep-yang-18-yangdoctors-early-jethanandani-2022-03-28-00

I am not an expert in PCEP. This review is looking at the draft from a YANG
perspective. With that said, I am continuing to mark 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.

Comments:

General

- Thanks for acknowledging me in the Acknowledgement section, but you have my
last name wrong :-). Dhruv, are you thinking of the famous lawyer by that name??

- The module continues to make 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.

Section 4. Objectives

You state what I would consider obvious objectives of any YANG model. Are they
really necessary?

More curiously, what caught my attention was mapping to the MIB module. Other
than an index value, and some notifications that are supported in the MIB, I
did not see a wholesale effort to try to map to the MIB.

If such an effort is desired, and I believe operators would like it, please
capture that mapping as part of the description statement of the node.

Section 5. The Design of the PCEP Data Model.

Thank you for taking care of the following comment from the -08 review.

-- Begin comment from -08 ----
Thank you for first of all for creating an 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. -- End comment from -08 ---

- Some new observations. Generally, the principal we have tried to follow is
that the container is plural (domains), but the list is singular (domain). Same
for capabilities and capability, neighbor-domains and domains, path-keys and
part-key etc. You get the picture. Please address/fix them.

- Could the section numbers for Section 5 follow the hierarchy of the model
itself. For example, pcep consists of entity (Section 5.2), which consists of
peers (Section 5.2.1), which consists of sessions (Section 5.2.1.1) etc.

Section 5.2 The Entity.

- Does pcep have 'entity' and 'peers', and each entity have their own 'peers'?
I see 'peers' under both.

- You say that

   The PCEP yang module may contain status information for the local
   PCEP entity.

   The entity has an IP address (using ietf-inet-types [RFC6991]) and a
   "role" leaf (the local entity PCEP role) as mandatory.

If this is status information (ro), how is the IP address and role leaf
mandatory type being enforced?

- The following statement seems incomplete:

   The various information related to this entity such as its domain,
   capabilities etc.

This paragraph is difficult to read with too many "and" and "as wells". Can it
rewritten?

   There is a list for static peer configuration and operational state
   of all peers (i.e.static as well as discovered)("/pcep/entity/
   peers").  The list is used to enable remote PCE configuration at PCC
   (or PCE) and has the operational state of these peers as well as the
   remote PCE peer which were discovered and PCC peers that have
   initiated session.

And since I am not a PCEP expert, I am allowed to ask some dumb question. Are
all the peer nodes configurable? Including their IP addresses and their
capabilities?? I would have imagined that the peers are configured on the nodes
where they exist, and show up as operational data on this node.

Section 5.6 RPC

Does the rpc not have any output? Success or failure??

Section 6 The Design of PCEP Statistics Data Model.

- Why are stats a separate module? Can they not be part of the base model? If
the idea is that some implementations may not want to implement statistics,
then the model can carry a feature statement 'statistics-supported'.

- Almost all counters are numbers. Is the num- prefix needed for every counter
name?

Section 10. PCEP YANG Modules

I see that many of the points from the -08 review have been incorporated. Thank
you. I have removed those that I think were resolved. But there are still a
few, and I am adding new comments.

- The model defines a dizzying number of feature statements. Is there any
portion of the model that is truly common?

- The YANG module continues the use of single instances of grouping, e.g.
pce-scope. Please collapse the grouping.

- Some of the typedefs continue to reuse the parent name in the definitions.
E.g. 'pcep-oper-status' has oper-status as a prefix for each definition. They
could easily be shortened to 'up', 'down', 'going-up' etc. The name of the
typedef already indicates that it is pcep-oper-status that is being referenced.

- I had provided the following comment in -08.

   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.

I now notice that they have been redefined in the module as 'domain-ospf-area'
and 'domain-isis-area'. Why? Why cannot the original definition in OSPF and
ISIS module be used?

- Again, I had provided the following comment in -08. Thanks for addressing
them. BTW, you can use 1..max instead of 1..65535. Same for 1..max instead of
1..4294967294.

   If the range of the timer is 1..65535, why does it need to be a uint32? Same
   for the range of 0..255.

- This comment was provided as part of -08. I do not think it has been resolved.

  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'.

A run of idnits reveals the following messages. Ignore them as some of the
errors (w.r.t. to the version of a draft) as those are tool issues.

idnits 2.16.04
   Attempted to download RFC Status...
   The downloaded file seems to be corrupt, proceeding with outdated
   information.

draft-ietf-pce-pcep-yang-18.txt:

  Checking boilerplate required by RFC 5378 and the IETF Trust (see
  https://trustee.ietf.org/license-info):
  ---------------------------------------------------------------------

  ** The document seems to lack a License Notice according IETF Trust
     Provisions of 28 Dec 2009, Section 6.b.i or Provisions of 12 Sep
     2009 Section 6.b -- however, there's a paragraph with a matching
     beginning. Boilerplate error?

  -- It seems you're using the 'non-IETF stream' Licence Notice instead

  Checking nits according to
  https://www.ietf.org/id-info/1id-guidelines.txt:
  ---------------------------------------------------------------------

     No issues found here.

  Checking nits according to https://www.ietf.org/id-info/checklist :
  ---------------------------------------------------------------------

  ** There are 3 instances of too long lines in the document, the
     longest one being 3 characters in excess of 72.

  Miscellaneous warnings:
  ---------------------------------------------------------------------

  == Line 487 has weird spacing: '...in-type    ide...'

  == Line 488 has weird spacing: '...in-info    dom...'

  == Line 510 has weird spacing: '...in-type    ide...'

  == Line 511 has weird spacing: '...in-info    dom...'

  == Line 535 has weird spacing: '...ax-rate    uin...'

  == (31 more instances...)

  -- The document date (January 2022) is 72 days in the past.  Is this
     intentional?

  Checking references for intended status: Proposed Standard
  ---------------------------------------------------------------------

     (See RFCs 3967 and 4897 for information about using normative
     references to lower-maturity documents in RFCs)

  == Outdated reference: A later version (-27) exists of
     draft-ietf-netconf-tls-client-server-26

  == Outdated reference: A later version (-29) exists of
     draft-ietf-teas-yang-te-28

  -- Obsolete informational reference (is this intentional?): RFC 5246
     (Obsoleted by RFC 8446)

     Summary: 2 errors (**), 0 flaws (~~), 8 warnings (==), 3 comments
     (--).

     Run idnits with the --verbose option for more detailed
     information about the items above.