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

From: Dhruv Dhody <[email-masked]>
Date: Sun, 24 Mar 2019 22:07:18 +0100

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-masked]>
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

---

From: tom petch <[email-masked]>
Date: Tue, 26 Mar 2019 12:49:40 +0000

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-masked]>
To: "Mahesh Jethanandani" <[email-masked]>
Cc: <[email-masked]>; <[email-masked]>;
<[email-masked]>
Sent: Sunday, March 24, 2019 9:07 PM
Subject: Re: [Pce] Yangdoctors early review of
draft-ietf-pce-pcep-yang-08

Hi Mahesh,

More details inline...

On Tue, Nov 27, 2018 at 8:28 AM Mahesh Jethanandani
<[email-masked]>

Used 2 spaces now.

Done.

Done.

Done.

Done.

Done.

Done.

Removed.

Removed.

Changed.

References added.

Updated.

Updated.

Corrected.

> 'keep-alive-timer' or 'dead-timer'? Same for 'min-keep-alive-timer'
and
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.

Updated.

Updated.

Updated.

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.

Updated.

> these statistics? In other words, are all these statistics required to
be
We have kept most of these as optional and not mandated it, these are
also
aligned to stats in PCEP MIB RFC.

> 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?

Regards,
Dhruv

------------------------------------------------------------------------
--------

> _______________________________________________
> Pce mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/pce
>

---

From: Martin Bjorklund <[email-masked]>
Date: Tue, 26 Mar 2019 14:16:08 +0100 (CET)

Hi,

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

> yang-doctors mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/yang-doctors

---

From: Dhruv Dhody <[email-masked]>
Date: Tue, 26 Mar 2019 14:30:05 +0100

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
[email-masked] --ietf >ietf-pcep.tree

That made me pick a shorter prefix, but happy to learn if there is a
better way out there!

Thanks!
Dhruv

---

From: Martin Bjorklund <[email-masked]>
Date: Tue, 26 Mar 2019 15:00:53 +0100 (CET)

Hi,

> [email-masked] --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

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

---

From: Dhruv Dhody <[email-masked]>
Date: Tue, 26 Mar 2019 15:38:45 +0100

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

---

From: Dhruv Dhody <[email-masked]>
Date: Tue, 26 Mar 2019 16:49:58 +0100

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

---

From: tom petch <[email-masked]>
Date: Tue, 26 Mar 2019 17:34:14 +0000

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" <[email-masked]>
Sent: Tuesday, March 26, 2019 2:38 PM

Hi Martin,

Thanks!
Dhruv

On Tue, Mar 26, 2019 at 3:00 PM Martin Bjorklund <[email-masked]> wrote:
> > > > > I am not an expert in PCEP. This review is looking at the
draft from a
> > > > > 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
> > > > > - 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
> > > > > 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
> > > > > 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
> > > > > - Please list all RFCs and I-D that are referenced in the
model, so
> > > > there
> > > > > - Please expand the reference to different RFCs to include the
title
> > > > of the
> > > > > I-D.ietf-netconf-tls-client-server, as it is not an RFC as
yet. Also,
> > > > > to replace XXXX with? With the RFC number assigned to this
draft?? I
> > > > think
> > > > > - What is "PCEP common"? That term has not been defined
anywhere in
> > > > > - Why is pcep-admin-status a enum and not a boolean? Since
YANG nodes
> > > > > - Where are the different operational status definitions
defined? Can
> > > > > - 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?
> > > > > 'keep-alive-timer' or 'dead-timer'? Same for
'min-keep-alive-timer'
> > > > > - The grouping pce-scope seems to be defining a header with
each of
> > > > > - 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.
> > > > > - 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
> > > > > - 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

---

From: Dhruv Dhody <[email-masked]>
Date: Tue, 26 Mar 2019 18:35:05 +0100

Done and a new version is posted.

---

From: tom petch <[email-masked]>
Date: Wed, 27 Mar 2019 17:48:21 +0000

----- Original Message -----
From: "Dhruv Dhody" <[email-masked]>
Sent: Tuesday, March 26, 2019 5:35 PM

<tp>

The prefixes look good and the line lengths I think are ok - I make it
81 characters including the left margin spaces -  but ...

in -10 you had
"   import ietf-te-types {   prefix "te-types";  reference
       "I-D.ietf-teas-yang-te-types: Traffic Engineering Common YANG
       Types";  "
which looks ok

in -11 you have
  import ietf-te-types {     prefix te-types;     reference
      "[I-D.ietf-teas-yang-te-types]: Traffic Engineering Common YANG
       Types";
which does not look ok to me

Recall that a YANG module is plain text so there cannot be any XML/HTML
style references/links and that
     [I-D.ietf-teas-yang-te-types]:
looks to me as if it has XML underpinning it which is not ok in a YANG
module

And this has happened in 16 places.

Also, IANA Considerations does not register

   URI:  urn:ietf:params:xml:ns:yang:ietf-pcep-stats

Security Considerations talks of a YANG module when the I-D has two,
giving the flavour, as with the IANA Considerations, that the second
module, pcep-stats, has not really been considered.  Since it is
statistics, I imagine that there are no sensitive objects there, in
which case I would add a sentence to spell that out.

And something new
/Segement Routing /Segment Routing /

Tom Petch

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

> > > > > On the question of prefix, where I an interested in the
opinion
> of a
> > > > > happens in almost every TEAS module so for me that is not a
very
> good
> > > > on these modules, in order to get them formatted consistently
with
> > > > > so the tools cannot pick it up, it must be spotted by eye and
so
> might
> > > > > confusing but I meant to use YYYY ZZZZ, with an RFC Editor
Note
> for
<snip>

---

From: Dhruv Dhody <[email-masked]>
Date: Thu, 28 Mar 2019 11:02:40 +0100

Hi Tom,

Apologies, I misunderstood your comment on references. The [] would be
removed.
Who knew updating and compiling yang module while sitting in a WG session
is not good :)
Will make an update SOON fixing these!

Thanks again for all your effort in keeping yang drafts in good condition!

Regards,
Dhruv