Hi Tom, Thanks for your continued review.
On Wed, Jan 5, 2022 at 5:39 PM tom petch <[email protected]> wrote: > From: Dhruv Dhody <[email protected]> > 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 > > Update in 3 places. > 'Further incase when the entity is PCE ' > perhaps just > 'When the entity is PCE ' > > Ack. > 'Each peer in the list is identified by its IP address' > (addr-type, addr). > I do not see addr-type in the YANG > > Removed. > 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, ' > > Ack > s.5.5 > PCEP MIB > could do with a reference > > Ack > 7.1. Stateful PCE's LSP-DB > ' In the operational state ' > perhaps 'operational datastore' > > Updated to - In the operational datastore of stateful PCE... > 8.1 > 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. > 9.1 > ' https://tools' > datatracker I think > > 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. > 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. > > I will remove the default statement. > /retreived/retrieved/ > in several places > > I found only 1, which I updated. > case auth-tls > 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).* > RPC > 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. > 9.2 > /tools/datatracker/ > > TLP is out of date > > Updated. Thanks! Dhruv https://www.ietf.org/rfcdiff?url2=draft-ietf-pce-pcep-yang-18 > > > 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 protected]<mailto: > [email protected]>> wrote: > From: Pce <[email protected]<mailto:[email protected]>> on behalf > of [email protected]<mailto:[email protected]> < > [email protected]<mailto:[email protected]>> > 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 protected]<mailto:[email protected]> > https://www.ietf.org/mailman/listinfo/pce > > _______________________________________________ > Pce mailing list > [email protected]<mailto:[email protected]> > https://www.ietf.org/mailman/listinfo/pce >
_______________________________________________ Pce mailing list [email protected] https://www.ietf.org/mailman/listinfo/pce
