Re: [Pce] I-D Action: draft-ietf-pce-pcep-yang-16.txt

[Dhruv Dhody <[e-mail]>]

Hi Tom,

Apologies for the delay. I have made an update to the I-D.

https://www.ietf.org/rfcdiff?url2=draft-ietf-pce-pcep-yang-17

Further details inline...

On Thu, Mar 11, 2021 at 6:26 PM tom petch <[e-mail]> wrote:

> From: Pce <[e-mail]> on behalf of [e-mail] <
> [e-mail]>
> Sent: 22 February 2021 14:27
>
> <tp>
> I notice a number of glitches in this I-D, several about terminology,
> others about arithmetic.  I post the ones I have seen so far because my
> cache is full, but I expect that there will be more!
>
> s.3 lacks
> **PLSPID
> ** Association
> ** OF  RFC5441
> **SRP RFC8231
> all of which I think are needed along with the relevant RFC as references
>
>
[Dhruv]: Done!



> RFC5540 is consistent in its terminology as it should be, but this I-D
> does not follow that terminology and is not consistent.  I refer to such as
> Keepalive
> OpenWait
> KeepWait
> DeadTimer
> SyncTimer
> back-off
> Where these appear in text in the I-D then I think that they should appear
> exactly as in the RFC.  YANG leaf names is trickier.  YANG does not do
> camelcase so I think that hyphenated is best e,g,
> open-wait, keep-wait; keep-alive I am unsure about and note that the I-D
> has both with and without a hyphen which needs fixing
>
>
[Dhruv]: Done!



> grouping pce-scope lacks the bit name as in the RFC, L, R, S, Rd, etc
>
> leaf intra-area-pref {
> does not say which is the higher pref and again lacks the names in the RFC
>
>
[Dhruv]: Added



> there are several 'domain' which is why the YANG convention is to make a
> list name plural with the leaf therein as singular lest you get a path with
> the same element repeated several times!
>
>
[Dhruv]: Updated



> container capability
> the reference should be to the IANA registry not the RFCs; in two leaf
>
>
[Dhruv]: Added



> NAI needs expanding in the text, SID too
>
>
[Dhruv]: Ack



> container neigh
> neigh is the sound that a horse makes but ....
>
>
[Dhruv]: Updated



> list domain perhaps better as list domains as above
>
>
[Dhruv]: OK



> admin-status oper-status
> did NMDA make this approach redundant?
>
>
[Dhruv]: Hmm, the current way is clearer with admin-status is boolean (and
configurable) whereas oper-status has many more states (and config-false).



> There is an awful lot of 'when pcc or pcc-and-pce.
> It would be lovely if the pcc-and-pce case could be represented with two
> values so that it becomes just when pcc; ditto pce
>
>
[Dhruv]: I am not sure how to do that with enum.



>           leaf enabled {
>             type boolean;
>             description
>               "Enabled or Disabled";
> which is true?  I know, obvious, but I like stating the obvious
>
>
[Dhruv]: Updated.



>       leaf open-wait-timer {
> the RFC says this is fixed not configurable.  If there is a reason to
> ignore the RFC then that needs to be spelt out IMO
>       leaf keep-wait-timer {
> ditto
>
>
[Dhruv]: It is marked config false already. It's kept mainly because RFC
7420 choose to keep it as well.



>       leaf dead-timer {
> the RFC recommends a multiplier  not a value which I think better lest an
> operator increase the wait, forget to increase dead and so kills sessions
>
>
[Dhruv]: Most implementation would set the dead timer value automatically
based on the Keepalive timer. But it is useful for the dead timer value to
be over-written via configuration when needed. You will also find DeadTimer
included in the OPEN message as a value in time and thus consistent with
RFC 5440 and RFC 7420.




>       leaf allow-negotiation {
> default?
>
>
[Dhruv]: Added



>             Zero means that the PCEP
>            entity will allow no Keepalive transmission at
>            all.";
> This seems like a bad idea.  If the peer is not allowed to send Keepalive
> then I would expect them to be greeted with an error message
>
>
[Dhruv]: The zero in max-keepalive-timer is to indicate that during the
negotiation of the keepalive value in the Open message that the only
acceptable value of peer's keepalive-timer is zero. If one receives an
unacceptable value, that does lead to the PCErr message Error-Type=1 and
Error-value=5!



>             Zero means that
>            the PCEP entity will allow not running a Dead
>            timer.";
> Likewise, if the peer wants to run a Deadtimer how can you stop them?
>
>       leaf min-dead-timer {
> and again.  I think that the underlying concepts need more consideration.
> I see nothing like this in the RFC.
>
>
[Dhruv]:  PCEP session will not be established with session parameters are
unacceptable during negotiation.  Also, this aligns with RFC 7420.



>       leaf max-unknown-reqs {
> I think that the RFC fixes this as five.
>
>
[Dhruv]: it is recommended, not fixed.



> /"The PCEP association type";/"The PCEP Association Type";/
> and the reference should be to IANA not RFC
>
>
[Dhruv]: OK



> /"PCEP Association Global Source.";/"PCEP Global Association Source.";/
> as per RFC; this also occurs lower down
>
>             leaf srp-id {
> RFC8231 says 0 and ffffffff reserved, YANG does not.
>
> /"The Path-key should be retreived";/"The Path-key should be retrieved";/
>
>
[Dhruv]: Ack for these!



>               case auth-tls {
>                 if-feature "tls";
>                 choice role {
>                   description
>                     "The role of the local entity";
>                   case server {
>
> what happens when entity is pcc and pce? the PCC client must be the TLS
> client but I do not know how this is handled.
>
>
[Dhruv]: This is configured per peer. So a "pcc-and-pce" entity could act
as a client towards one peer and server towards another. This is consistent
with how RFC 5440 and RFC 8253 describe it.



> .... appear transiently in this yang module. The
> caps for YANG
>
>
[Dhruv]: Ack



>               leaf dead-timer {
> as above I like counts not times
>
>
[Dhruv]: See above!



>               leaf overload-time {
> in several places, does the time duration  have any meaning when there is
> no time of day to go with it to say when it started?
>
>
[Dhruv]: Added



>                       "The PST authorized";
> I am unclear why this is 'authorized; who has said so?
>
>
[Dhruv]: Updated



>   notification pcep-session-local-overload {
> as above does this need a data and time?
>
>
[Dhruv]: Added



> /"Trigger the resyncrinization at the PCE";/"Trigger the resynchronization
> at the PCE";
>
>        leaf avg-rsp-time {
> I would find
>        leaf rsp-time-avg
> clearer for this and the other two times
>
>
[Dhruv]: Updated



> counters
> do they need a discontinuity date and time?
>
>
[Dhruv]: It was in the ietf-pcep, I moved to the ietf-pcep-stats module.



>        leaf num-keepalive-sent {
> elsewhere keep-alive is hyphenated in YANG leaf names
>
> 5.2
> /capcabilities/capabilities
>
>
[Dhruv]: Updated

Thanks for your review!
Dhruv



>
> There are parts of PCE I have not looked at previously and so plan to look
> at the RFC which is likely to generate further comments.
>
> Tom Petch
>
>
>
> A New Internet-Draft is available from the on-line Internet-Drafts
> directories.
> This draft is a work item of the Path Computation Element WG of the IETF.
>
>         Title           : A YANG Data Model for Path Computation Element
> Communications Protocol (PCEP)
>         Authors         : Dhruv Dhody
>                           Jonathan Hardwick
>                           Vishnu Pavan Beeram
>                           Jeff Tantsura
>         Filename        : draft-ietf-pce-pcep-yang-16.txt
>         Pages           : 112
>         Date            : 2021-02-22
>
> Abstract:
>    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 and state data.
>
>
> The IETF datatracker status page for this draft is:
> https://datatracker.ietf.org/doc/draft-ietf-pce-pcep-yang/
>
> There are also htmlized versions available at:
> https://tools.ietf.org/html/draft-ietf-pce-pcep-yang-16
> https://datatracker.ietf.org/doc/html/draft-ietf-pce-pcep-yang-16
>
> A diff from the previous version is available at:
> https://www.ietf.org/rfcdiff?url2=draft-ietf-pce-pcep-yang-16
>
>
> Please note that it may take a couple of minutes from the time of
> submission
> until the htmlized version and diff are available at tools.ietf.org.
>
> Internet-Drafts are also available by anonymous FTP at:
> ftp://ftp.ietf.org/internet-drafts/
>
>
> _______________________________________________
> Pce mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/pce
>
> _______________________________________________
> Pce mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/pce
>