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

[Dhruv Dhody <[e-mail]>]

Hi Tom,

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

> 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).
>
>
This seems correct.




> 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 ....?
>
>
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
> ________________________________________
> From: Pce <[e-mail]> on behalf of tom petch <
> [e-mail]>
> Sent: 05 January 2022 12:09
> Subject: Re: [Pce] I-D Action: draft-ietf-pce-pcep-yang-16.txt
>
> 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
>
> _______________________________________________
> Pce mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/pce
>