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.