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

[Dhruv Dhody <[e-mail]>]

Hi Mahesh,

Thanks for your 2nd review. Please find the update at -
https://www.ietf.org/rfcdiff?url2=draft-ietf-pce-pcep-yang-19

See inline...

On Tue, Mar 29, 2022 at 4:29 AM Mahesh Jethanandani via Datatracker <
[e-mail]> wrote:

> Reviewer: Mahesh Jethanandani
> Review result: On the Right Track
>
> 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??
>
>
Oops! Corrected!  :)



> - 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.
>
>
I collapsed "grouping pce-scope", everything else is being used more than
once!



> Section 4. Objectives
>
> You state what I would consider obvious objectives of any YANG model. Are
> they
> really necessary?
>
>
Moved this to the appendix



> 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.
>
>
I have added a mapping table in the appendix. It took quite an effort to
dig it all up! Phew!



> 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.
>
>
Updated for domains/domain, neighbor-domains/domain,
capabilities/capability, path-keys/path-key...


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



> Section 5.2 The Entity.
>
> - Does pcep have 'entity' and 'peers', and each entity have their own
> 'peers'?
> I see 'peers' under both.
>
>
We have a single "local" entity that has multiple peers.



> - 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?
>
>
This is (rw) with  "mandatory true".



> - The following statement seems incomplete:
>
>    The various information related to this entity such as its domain,
>    capabilities etc.
>
>
Updated to - "The local PCEP entity contains 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.
>
>
There is support for both static peer configuration as well as peers that
are learned.

I updated the text to -

"This peer list includes peers that are explicitly configured at the local
PCEP entity as well as peers that are learned dynamically. For example, at
a PCC, the remote PCE peer to use could be explicitly configured. A PCC
could also learn PCE in the network via IGP discovery and it will show up
in this list. When a session is initiated at a PCE, the remote PCC peer
information is also added by the system to the peer list."


> Section 5.6 RPC
>
> Does the rpc not have any output? Success or failure??
>
>
No, this is basically to trigger the process of re-synchronization as per
RFC8232 where the idea is for PCE to be able to trigger resynchronization
with a PCC even after the session has been established. The PCE may use
this approach to continuously sanity check its state against the network or
to recover from error conditions without having to tear down sessions.


> 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'.
>
>
It was done to have some sanity in the model as the size of the model was
getting too big with statistics appearing at three different levels. WG
preferred this approach.



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



> 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?
>
>
Everything in RFC 5440 is not controlled by the feature and is common.
Everything else is a new feature with its capability and it made sense to
define it with a feature. It is also the nature of PCEP that has been
extended for various different scenarios with many extensions.



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



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



> - 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?
>
>
Agree, no value in another typedef, removed and using it directly.



> - 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.
>
>
Added this!



>    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 was resolved as far as I can see.


> - 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'.
>
>
I replied last time ->

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.

These values are important for the PCEP session parameter negotiation and
since they have been part of RFC7420, I think we should keep them!
I ran the idnits before posting a new update!

Thanks again for your review!

Regards,
Dhruv



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