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

[Dhruv Dhody <[e-mail]>]

Done and a new version is posted.

On Tue, 26 Mar 2019 at 18:34, tom petch <[e-mail]> wrote:

> Dhruv
>
> I commented up thread that the prefixes used in this I-D were not the
> ones that appear in the imported modules and said I thought that that
> was discouraged but ok. Checking RFC8407, it says
>
>    o  The proper module prefix MUST be used for all identifiers imported
>       from other modules.
>
> It is a MUST not a SHOULD so I believe that you must bring those
> prefixes in line for key-chain, tls-client, tls-server.  YANG allows it,
> YANG guidelines does not.
>
> Tom Petch
>
>
> ----- Original Message -----
> From: "Dhruv Dhody" <[e-mail]>
> Sent: Tuesday, March 26, 2019 2:38 PM
>
> Hi Martin,
>
> The newer version of pyang worked! Thanks for your help!
>
> I found the full tree useful when I am searching for a leaf in the
> yang models and understand how it fits in the overall tree. Thus I see
> value in both. We can also consider if we should also update 5.2-5.6
> additionally.
>
> Thanks!
> Dhruv
>
> On Tue, Mar 26, 2019 at 3:00 PM Martin Bjorklund <[e-mail]> wrote:
> >
> > Hi,
> >
> > Dhruv Dhody <[e-mail]> wrote:
> > > Hi Mahesh, Tom,
> > >
> > > Got it, will make the necessary change soon.
> > >
> > > Where I need help is the tree creation, even though I use
> > > '--tree-line-length' I faced the issue with overrunning the 80
> > > characters.
> > >
> > > pyang --ietf -f tree --tree-line-length=68 --tree-depth=10
> > > [e-mail] --ietf >ietf-pcep.tree
> >
> > Have you tried using pyang 1.7.8?  When I run that the tree seems to
> > fit the line lengths.
> >
> > > That made me pick a shorter prefix, but happy to learn if there is a
> > > better way out there!
> >
> > Personally, I'm not too fond of very large tree diagrams.  I prefer to
> > split them into smaller diagrams.  So I like your overview diagram in
> > section 5.1.  I would then probably add a small diagram in each of the
> > section 5.2-5.6, and remove secion 5.7 completely.  But this is just
> > my personal preference!
> >
> > /martin
> >
> > > Thanks!
> > > Dhruv
> > >
> > > On Tue, Mar 26, 2019 at 2:16 PM Martin Bjorklund <[e-mail]>
> wrote:
> > > >
> > > > Hi,
> > > >
> > > > tom petch <[e-mail]> 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.
> > > >
> > > > I strongly agree.  Since the prefix is actually part of the IANA
> > > > registry and needs to be unique, I think you should use a longer
> > > > prefix.  "pcep" seems reasonable.  If you run into line length
> > > > problems, I'll be glad to help you fix them.
> > > >
> > > > Before this document goes to the RFC editor, I suggest you run the
> > > > tool:
> > > >
> > > >    pyang -f yang --keep-comments --yang-line-length 69 <FILE>
> > > >
> > > > on these modules, in order to get them formatted consistently with
> the
> > > > rest of the IETF modules.
> > > >
> > > > > 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" <[e-mail]>
> > > > > To: "Mahesh Jethanandani" <[e-mail]>
> > > > > Cc: <[e-mail]>;
> <[e-mail]>;
> > > > > <[e-mail]>
> > > > > 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
> > > > > <[e-mail]>
> > > > > 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
> > > > > > [e-mail]
> > > > > > https://www.ietf.org/mailman/listinfo/pce
> > > > > >
> > > > >
> > > > > _______________________________________________
> > > > > yang-doctors mailing list
> > > > > [e-mail]
> > > > > https://www.ietf.org/mailman/listinfo/yang-doctors
> > >
>
>