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

[tom petch <[e-mail]>]

From: Dhruv Dhody <[e-mail]>
Sent: 23 October 2021 12:37

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

<tp>
Some more thoughts

/refrenc/referenc/ in lots of places

'Further incase when the entity is PCE '
perhaps just
'When the entity is PCE  '

'Each peer in the list is identified by its IP address'
   (addr-type, addr).
I do not see addr-type in the YANG
   
5.4.  The Session Lists
   The session list contains PCEP session 
Perhaps 'PCEP sessions'


'If either of these sessions reaches active state first,'
perhaps
'When one of these sessions reaches the active state, '

s.5.5 
PCEP MIB
could do with a reference

7.1.  Stateful PCE's LSP-DB
'   In the operational state '
perhaps 'operational datastore'

8.1
What is the procedure when a PCE is both client and server?

RFC8253 is TLS1.2 only and TLS1.3 bears little resemblance to anything that has gone before so probably worth mentioning that this is 1.2 and referencing RFC5246

9.1
' https://tools'
datatracker I think

TLP out of date

' leaf capability'
I think that this should reference the IANA registry and not the RFC that created it since several of the bits do not appear in the RFC

I echo the comment about the addresses being 'no-zone'

NMDA I do not understand.  I thought it meant that different states of the same entity appear in different datastores so we did not need
        leaf admin-status {
        leaf oper-status {
but perhaps we do

         leaf open-wait-timer {
           type uint16;
           units "seconds";
           default "60";
           config false;
likewise what does a default do for 'config false'?  The RFC says that the value is fixed.

/retreived/retrieved/
in several places

case auth-tls
what if the role is both client and server?

RPC
might benefit from a NACM default deny all

9.2
/tools/datatracker/

TLP is out of date



The modelling of Keepalive confuses me - I need to check the RFC and the MIB and come back to you

Tom Petch

On Thu, Mar 11, 2021 at 6:26 PM tom petch <[e-mail]<mailto:[e-mail]>> wrote:
From: Pce <[e-mail]<mailto:[e-mail]>> on behalf of [e-mail]<mailto:[e-mail]> <[e-mail]<mailto:[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<http://tools.ietf.org>.

Internet-Drafts are also available by anonymous FTP at:
ftp://ftp.ietf.org/internet-drafts/


_______________________________________________
Pce mailing list
[e-mail]<mailto:[e-mail]>
https://www.ietf.org/mailman/listinfo/pce

_______________________________________________
Pce mailing list
[e-mail]<mailto:[e-mail]>
https://www.ietf.org/mailman/listinfo/pce