Hi Tom, I just had a chat with RFC Editor and they say that they are happy with the draft names in the references instead of RFC YYYY/ RFC ZZZZ for other documents being referenced.
Thanks again for your reviews, I will fix the others and plan to make an update today. Regards, Dhruv On Tue, Mar 26, 2019 at 1:49 PM tom petch <[email protected]> wrote: > > On the question of prefix, where I an interested in the opinion of a > YANG > Doctor, you use the single letter 'p' and say that a longer prefix gives > you line length problems. YANG does allow statements to span lines, as > happens in almost every TEAS module so for me that is not a very good > reason; I would prefer something of two characters or more. > > I note that IANA Considerations says > Prefix: pcep > which would be my first choice even if I then have to span lines. > > You import the module key-chain but you do not use the prefix that it > defines, namely key-chain; not forbidden but not recommended practice > > Likewise tls-client should be tlsc and tls-server tlss. > > Security and IANA Considerations deal with > Name: ietf-pcep > What about > module ietf-pcep-stats { > which I think needs separate coverage, a separate section, in Security > and must be covered in IANA Considerations. > > The problem with > "I-D.ietf-pce-association-group: PCEP Extensions for ... > as a reference is that when it appears in the text of the I-D, then it > is as > [I-D.ietf-pce-association-group] > i.e. a XML/HTML type anchor which is picked up by tools so the RFC > Editor cannot miss it. > > When it appears in the YANG module, it must be plain text as in > "I-D.ietf-pce-association-group: PCEP Extensions for .... > so the tools cannot pick it up, it must be spotted by eye and so might > be missed. Hence I suggest using > > "RFC YYYY - PCEP Extensions for > Establishing Relationships Between Sets of LSPs"; > > with a note to the RFC Editor asking them to replace YYYY with the RFC > number assigned to I-D.ietf-pce-association-group > > Likewise RFC ZZZZ for > "I-D.ietf-pce-segment-routing: PCEP Extensions for Segment > and so on for the others (of which there are several) > > The RFC Editor is ok, likes even, all the notes thereon to appear once > at the start of the I-D. > > So my previous comment was that using XXXX for multiple I-Ds was > confusing but I meant to use YYYY ZZZZ, with an RFC Editor Note for > each, and not to use the I-D name. > > HTH > > Tom Petch > > ----- Original Message ----- > From: "Dhruv Dhody" <[email protected]> > To: "Mahesh Jethanandani" <[email protected]> > Cc: <[email protected]>; <[email protected]>; > <[email protected]> > Sent: Sunday, March 24, 2019 9:07 PM > Subject: Re: [Pce] Yangdoctors early review of > draft-ietf-pce-pcep-yang-08 > > > Hi Mahesh, > > Apologies for a late reply to your review. Being stuck in a long flight > finally gave me enough time to fix up the indentation in the model :) > > An update (-10) has been posted. > > More details inline... > > On Tue, Nov 27, 2018 at 8:28 AM Mahesh Jethanandani > <[email protected]> > wrote: > > > 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. > > > > Used 2 spaces now. > > > > > - 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. > > > > > All groupings that were used only once are now removed. > > > > > 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 > > > > Done. > > > > > > > Introduction: > > > > Please update reference of YANG to RFC7950. These are YANG 1.1 modules > > after > > all. > > > > Done. > > > > > > > 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. > > > > Done. > > > > > > > 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. > > > > Done. > > > > > > > - Please expand the reference to different RFCs to include the title > of the > > RFC, and not just the number. > > > > Done. > > > > > > > - 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. > > > > Done. > > > > > > > - What is "PCEP common"? That term has not been defined anywhere in > the > > document, but is used in the YANG model. > > > > > Removed. > > > > > - What is the 'identify pcep' for? > > > > Removed. > > > > > > > - 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). > > > > Changed. > > > > > > > - Where are the different operational status definitions defined? Can > that > > RFC > > be referenced? Same for Session state, Association Type, Objective > > Function. > > > > > References added. > > > > > - 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. > > > > > Updated. > > > > > - Can the document use more descriptive names for features such as > 'gco'. > > > > Updated. > > > > > > > - If the range of the timer is 1..65535, why does it need to be a > uint32? > > Same > > for the range of 0..255. > > > > Corrected. > > > > > > > - 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'. > > > > > You are right that these are not explicitly stated in 5440, but are > needed > to set what is the acceptable range of these values as received in the > open > message from a peer. These are different from the max value as part of > the > range allowed by the protocol. You would also find these in our PCEP MIB > RFCs. > > > > > - What is the default value for 'admin-status'? > > > > set now to enabled (true). > > > > > > > - 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' > > > > > Updated. > But the priority fields are kept outside of bits/bit. > Also in case of capability, fields that are not part of RFC5088/RFC5089 > capability bit fields are kept outside. > > > > > - 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'. > > > > > Updated. > > > > > - Could description "Valid at PCC" be improved upon? > > > > > Updated. > > > > > - 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? > > > > > Updated according to the Key chain RFC that allows both ASCII and Hex > (instead of binary), i think this is better aligned to other related > work. > > > > - 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'. > > > > > The examples you cited are already 'ro'. I did a check throughout the > document as well. > > > > > - Can this error-message and description be reconciled? > > > > error-message > > "The Path-key should be retreived"; > > description > > "When Path-Key has been retreived"; > > > > > Updated. > > > > > - 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? > > > > > We have kept most of these as optional and not mandated it, these are > also > aligned to stats in PCEP MIB RFC. > > > > - 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. > > > > > It is the latter case, where some statistics are written based on the > role. > Do you think this usage of 'when' is incorrect and needs changing? > > Thanks again for your detailed review. > > Regards, > Dhruv > > > > ------------------------------------------------------------------------ > -------- > > > > _______________________________________________ > > Pce mailing list > > [email protected] > > https://www.ietf.org/mailman/listinfo/pce > > > _______________________________________________ Pce mailing list [email protected] https://www.ietf.org/mailman/listinfo/pce
