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

From: tom petch <[email-masked]>
Date: Wed, 5 Jan 2022 12:09:44 +0000

From: Dhruv Dhody <[email-masked]>
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 <[email-masked]<mailto:[email-masked]>> wrote:
From: Pce <[email-masked]<mailto:[email-masked]>> on behalf of [email-masked]<mailto:[email-masked]> <[email-masked]<mailto:[email-masked]>>
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
[email-masked]<mailto:[email-masked]>
https://www.ietf.org/mailman/listinfo/pce

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

---

From: tom petch <[email-masked]>
Date: Thu, 6 Jan 2022 11:40:26 +0000

Top posting a separate issue

The PCC sends an Open message with a proposed value for Keepalive (can be zero)

If receiver is ok with the proposed value, Open continues

If receiver is not ok, then it sends a PCErr message Type 1 Value 4 with a suggested value

If sender is willing to negotiate (negotiation enabled, min and max values configured for Keepalive), and the proposed value falls within range of min and max, then Open continues using the proposed value for Keepalive.  If not, open fails.

This implies that the sender is configured with boolean 'negotiation enabled', min and max acceptable values for Keepalive (values can be equal).

To me, this means:
- if negotiation is disabled, then min and max MUST NOT be configured.
- if negotiation is enabled, min and max MUST be configured with min GTE max,

Have I understood?

This is not what I see in the YANG module so if I am right, then I will propose some changes such as making 'enabled' a presence container with min and max in it and with a constraint max GTE min..  I have yet to look at the other negotiated values but expect that something similar will apply.

If not ....?

Tom Petch
________________________________________
From: Pce <[email-masked]> on behalf of tom petch <[email-masked]>
Sent: 05 January 2022 12:09
Subject: Re: [Pce] I-D Action: draft-ietf-pce-pcep-yang-16.txt

Hi Tom,

<tp>
Some more thoughts

5.4.  The Session Lists

s.5.5
PCEP MIB

8.1

9.1
' https://tools'
datatracker I think

TLP out of date

' leaf capability'

         leaf open-wait-timer {
           type uint16;
           units "seconds";
           default "60";
           config false;

/retreived/retrieved/
in several places

case auth-tls

RPC

9.2
/tools/datatracker/

TLP is out of date

Tom Petch

<tp>

s.3 lacks
**PLSPID
** Association
** OF  RFC5441
**SRP RFC8231

[Dhruv]: Done!

[Dhruv]: Done!

leaf intra-area-pref {

[Dhruv]: Added

[Dhruv]: Updated

container capability

[Dhruv]: Added

[Dhruv]: Ack

container neigh

[Dhruv]: Updated

[Dhruv]: OK

admin-status oper-status

          leaf enabled {
            type boolean;
            description
              "Enabled or Disabled";

[Dhruv]: Updated.

      leaf open-wait-timer {

      leaf dead-timer {

      leaf allow-negotiation {
default?

[Dhruv]: Added

            Zero means that the PCEP

            Zero means that

      leaf min-dead-timer {

      leaf max-unknown-reqs {

[Dhruv]: OK

            leaf srp-id {

[Dhruv]: Ack for these!

              case auth-tls {
                if-feature "tls";
                choice role {
                  description

[Dhruv]: Ack

              leaf dead-timer {

[Dhruv]: See above!

              leaf overload-time {

[Dhruv]: Added

                      "The PST authorized";

[Dhruv]: Updated

[Dhruv]: Added

       leaf avg-rsp-time {
I would find
       leaf rsp-time-avg

[Dhruv]: Updated

counters

5.2

[Dhruv]: Updated

Thanks for your review!
Dhruv

Tom Petch

Abstract:

[email-masked]

---

From: Dhruv Dhody <[email-masked]>
Date: Tue, 25 Jan 2022 15:47:54 +0530

Hi Tom,

Thanks for your continued review.

> /refrenc/referenc/ in lots of places
Update in 3 places.

> 'When the entity is PCE  '
Ack.

> I do not see addr-type in the YANG
Removed.

> 'When one of these sessions reaches the active state, '
Ack

> could do with a reference
Ack

> perhaps 'operational datastore'
Updated to - In the operational datastore of stateful PCE...

> What is the procedure when a PCE is both client and server?
A PCEP speaker could act as both a client (PCC) and a server (PCE) at the
same time. Within a PCEP session, its role is determined by the peer it is
interacting with. And the same holds good for TLS as well. I have added
text.

> 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
>
>
RFC8253 says "Support for TLS v1.2 [RFC5246
<https://datatracker.ietf.org/doc/html/rfc5246>] or later". Thus I updated
it accordingly.

> TLP out of date
Updated.

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

> I echo the comment about the addresses being 'no-zone'
Updated.

> 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
>
>
While admin-state is simple a boolean as set via config, the oper-status
has a lot more status information and is purely read-only. It makes sense
to not put this info in a single leaf.

> likewise what does a default do for 'config false'?  The RFC says that the
I will remove the default statement.

> in several places
I found only 1, which I updated.

> what if the role is both client and server?
While the entity could be both pcc-and-pce, a peer role should either be as
a client or server and not both. I have updated the description of the role
to say that.

*   Note that, a PCEP speaker could act as both a client (PCC) and a*

*   server (PCE).  The role within the context of a PCEP session is
 determined by the relationship it has with its peer (the same holds*
*   good for TLS as well).*

> might benefit from a NACM default deny all
Resynchronization is a way to sync DB without bringing down the session. I
don't think we need a deny-all for this RPC.

> TLP is out of date
Updated.

Thanks!
Dhruv
https://www.ietf.org/rfcdiff?url2=draft-ietf-pce-pcep-yang-18

> From: Pce <[email-masked]<mailto:[email-masked]>> on behalf
> of [email-masked]<mailto:[email-masked]> <
> [email-masked]<mailto:[email-masked]>>
> 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
> 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!
>
>
> 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
> [Dhruv]: Hmm, the current way is clearer with admin-status is boolean (and
> configurable) whereas oper-status has many more states (and config-false).
>
>
> 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
>
>
> 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
>
>
> 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
> 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
> [Dhruv]:  PCEP session will not be established with session parameters are
> unacceptable during negotiation.  Also, this aligns with RFC 7420.
>
>
>       leaf max-unknown-reqs {
> 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.
>
>
> 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";
> 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
>
>
>

---

From: Dhruv Dhody <[email-masked]>
Date: Tue, 25 Jan 2022 15:49:14 +0530

Hi Tom,

On Thu, Jan 6, 2022 at 5:10 PM tom petch <[email-masked]> wrote:

> If sender is willing to negotiate (negotiation enabled, min and max values
> configured for Keepalive), and the proposed value falls within range of min
> and max, then Open continues using the proposed value for Keepalive.  If
> not, open fails.
>
> This implies that the sender is configured with boolean 'negotiation
> enabled', min and max acceptable values for Keepalive (values can be equal).
>
>
This seems correct.

> This is not what I see in the YANG module so if I am right, then I will
> propose some changes such as making 'enabled' a presence container with min
> and max in it and with a constraint max GTE min..  I have yet to look at
> the other negotiated values but expect that something similar will apply.
>
> If not ....?
>
>
What we currently have is  -

           +--rw allow-negotiation?            boolean
           +--rw max-keepalive-timer?          uint8
           +--rw max-dead-timer?               uint8
           +--rw min-keepalive-timer?          uint8
           +--rw min-dead-timer?               uint8

Note that there is a difference between "unacceptable" and
"non-negotiable", the max value is used to also check if the received
parameter is acceptable, thus the max values are needed even when it is
non-negotiable.

I don't see a need to make max values mandatory to configure -- most
implementation would have some logic to initialize these values when they
bring the system up, as well as using the locally configured value as a
guide to generate min/max acceptable from the peer. Since this is local
behavior and not standardized, perhaps no need to update here. Thoughts?

BTW this is the same as our MIB - RFC7420.

Thanks!
Dhruv

Tom Petch