Hi Dhruv, Thanks for addressing most of my comments. But ...
> On Dec 16, 2024, at 6:07 PM, Dhruv Dhody <d...@dhruvdhody.com> wrote: > > Hi Mahesh, > > On Mon, Dec 16, 2024 at 4:01 PM Mahesh Jethanandani via Datatracker > <nore...@ietf.org <mailto:nore...@ietf.org>> wrote: > Mahesh Jethanandani has entered the following ballot position for > draft-ietf-pce-pcep-yang-26: Discuss > > When responding, please keep the subject line intact and reply to all > email addresses included in the To and CC lines. (Feel free to cut this > introductory paragraph, however.) > > > Please refer to > https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/ > <https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/> > > for more information about how to handle DISCUSS and COMMENT positions. > > > The document, along with other ballot positions, can be found here: > https://datatracker.ietf.org/doc/draft-ietf-pce-pcep-yang/ > <https://datatracker.ietf.org/doc/draft-ietf-pce-pcep-yang/> > > > > ---------------------------------------------------------------------- > DISCUSS: > ---------------------------------------------------------------------- > > Section 1, paragraph 3 > > This document contains a specification of the PCEP YANG module, > > "ietf-pcep" which provides the PCEP [RFC5440] data model. Further, > > this document also includes the PCEP statistics YANG module "ietf- > > pcep-stats" which provides statistics, counters and telemetry data. > > I realize that I had provided Early Reveiw from a YANG doctors point of view > for this module. And if the module had some of the same structure that I had > reviewed, I must apologize for not catching this earlier. > > > Dhruv: The structure has not changed since you last reviewed this. > > > It is fairly unusual to split the statistics into a separate module away from > the rest of the module. It is also unusual to have the separate module augment > the main module in the same document. Could this not have been achieved simply > by having a container called 'statistics' under 'peer' and 'session'? What is > achieved by having a complete separate namespace for statistics. > > Dhruv: The extensive number of statistics in PCEP made the module huge and > challenging to parse the tree, as the same statistics were present at > multiple levels. Initially, we had a single model, but since way back in > version -02 of the WG I-D, WGprefered a separate module for stats. Notably, > the IESG has approved the ALTO YANG model, which followed the same approach, > establishing a precedent. I do not think either are good reasons for breaking up the module as it has been done. From a viewing perspective, an abridged tree diagram can be used to illustrate where statistics are defined in the module. You do not need to display the full tree diagram to illustrate the structure of the module. If anything, a full tree diagram is discouraged in the normative part of the draft, and should be only in the appendix. There is overwhelming amount of precedent, of making sure statistics stay with the module where config and other state is defined. And just because a (bad precedent) was established, it does not make it any more correct. > Also, why use > 'rpcs' and not the 'action' statement to clear the statistics? See Section > 7.15.3 of RFC 7950 for an example and why 'action' should be used instead of > 'rpcs'. Hint, rpcs have no node in the datastores tied to them. That context > has to be somehow brought in. Also, how would one clear stats for a particular > peer or session? > > > Dhruv: Okay, I have changed it to an action to reset but I have retained at > rpc to clear all statistics (as I dont have a clear place to add the action > statement). The action statement is applicable to the parent container it is part of, and is also the reason you do not need to provide the context for it. If you define a container for statistics much like what you have, after you remove the augment and a separate module, and by moving 'sess-setup-ok' and 'sess-setup-fail' inside the ’stats’ container (do not know why these are not stats), you will have the following structure. module ietf-pcep { …. container peers { …. list peer { …. container stats { <list of statistics> action reset { input { } output { } } } } } Your action statement is now applicable to each of the peers, i.e., you can selectively reset/clear statistics for a given peer, with the special character of ‘*’ indicating you want to clear stats for all peers. HTH. Cheers. > > > For all these reasons, I feel there must have been a strong motivation for > coming up with a separate module for statistics, and that motivation along > with > the use of rpcs should be clearly described, otherwise it would be better for > the module to be collapsed into a single module, and the rpcs converted to > actions. > > > Dhruv: I hope you are satisfied with the above. > > > Section 4, paragraph 1 > > The PCEP YANG module defined in this document has all the common > > building blocks for the PCEP protocol. > > For the rest of the YANG module, I would like to thank Jan Lindblad for > providing YANG doctor review on the document. But I have not seen any response > to the review, or how it will be addressed. Holding a DISCUSS to make sure > those comments are addressed. > > > Dhruv: I have worked through Jan's comment. See my response there. > > > > ---------------------------------------------------------------------- > COMMENT: > ---------------------------------------------------------------------- > > "Abstract", paragraph 1 > > This document defines a YANG data model for the management of the > > 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. > > Maybe it is just me, but most YANG data models today have both configuration > and state data, even if the state data reflects the state of any of the conifg > leafs. As such, the last sentence seems redundant to me. > > > Dhruv: removed. > > > Section 9, paragraph 1 > > The YANG modules defined in this document are designed to be accessed > > via a network management protocol such as NETCONF [RFC6241] or > > RESTCONF [RFC8040]. The lowest NETCONF layer is the secure transport > > layer and the mandatory-to-implement secure transport is SSH > > [RFC6242]. The lowest RESTCONF layer is HTTPS, and the mandatory-to- > > implement secure transport is TLS [RFC8446] > > The NETCONF access control model [RFC8341] provides the means to > > restrict access for particular NETCONF or RESTCONF users to a pre- > > configured subset of all available NETCONF or RESTCONF protocol > > operations and content. > > Please update this template to match the recent changes in the template as > described in rfc8407bis Section 3.7.1. > > > Dhruv: Updated. > > > The IANA review of this document seems to not have concluded yet. > > > Dhruv: Expert review was done in the past. > > > Check whether the "Implementation Status" section is reasonable. If there is > no > implementation status to report, should this section be removed? > > > Dhruv: This is as per the PCE WG implementation section policy. > https://wiki.ietf.org/group/pce/ImplementationPolicy > <https://wiki.ietf.org/group/pce/ImplementationPolicy> > > DOWNREF [I-D.ietf-teas-yang-te] from this Proposed Standard to > draft-ietf-teas-yang-te of unknown standards level. (For IESG discussion. It > seems this DOWNREF was not mentioned in the Last Call and also seems to not > appear in the DOWNREF registry.) > > > Dhruv: It is an informative reference. It was incorrectly marked! Apologies! > > > Found terminology that should be reviewed for inclusivity; see > https://www.rfc-editor.org/part2/#inclusive_language > <https://www.rfc-editor.org/part2/#inclusive_language> for background and more > guidance: > > * Term "native"; alternatives might be "built-in", "fundamental", > "ingrained", > "intrinsic", "original" > > > Dhruv: Removed > > > Found IP block or address not inside RFC5737/RFC3849 example ranges: > "0.0.0.0". > > > Dhruv: It is a valid usage inside the yang description. > > > ------------------------------------------------------------------------------- > NIT > ------------------------------------------------------------------------------- > > All comments below are about very minor potential issues that you may choose > to > address in some way - or ignore - as you see fit. Some were flagged by > automated tools (via https://github.com/larseggert/ietf-reviewtool > <https://github.com/larseggert/ietf-reviewtool>), so there > will likely be some false positives. There is no need to let me know what you > did with these suggestions. > > Section 1, paragraph 3 > > This document defines a YANG [RFC7950] data model for the management > > of PCEP speakers. It is important to establish a common data model > > for how PCEP speakers are identified, configured, and monitored. The > > data model includes configuration data and state data. > > This is a YANG 1.1 model by virtue of the fact you are referencing RFC 7950. > You might want to just say that it is a YANG 1.1 module. > > > Dhruv: ok > > > Document references draft-ietf-netconf-tls-client-server, but that has been > published as RFC9645. > > Document references draft-ietf-pce-pcep-srv6-yang-05, but -06 is the latest > available revision. > > > Dhruv: updated > > > Reference [RFC5246] to RFC5246, which was obsoleted by RFC8446 (this may be on > purpose). > > > Dhruv: On purpose as we wanted to refer to TLS 1.2 > > "Abstract", paragraph 1 > > the state data reflects the state of any of the conifg leafs. As such, the > > la > > ^^^^^^^^^ > Consider simply using "of" instead. > > > > Dhruv: Not updated. > > URL: https://www.ietf.org/archive/id/draft-ietf-pce-pcep-yang-27.txt > <https://www.ietf.org/archive/id/draft-ietf-pce-pcep-yang-27.txt> > Status: https://datatracker.ietf.org/doc/draft-ietf-pce-pcep-yang/ > <https://datatracker.ietf.org/doc/draft-ietf-pce-pcep-yang/> > HTMLized: https://datatracker.ietf.org/doc/html/draft-ietf-pce-pcep-yang > <https://datatracker.ietf.org/doc/html/draft-ietf-pce-pcep-yang> > Diff: > https://author-tools.ietf.org/iddiff?url2=draft-ietf-pce-pcep-yang-27 > <https://author-tools.ietf.org/iddiff?url2=draft-ietf-pce-pcep-yang-27> > > Thanks! > Dhruv Mahesh Jethanandani mjethanand...@gmail.com
_______________________________________________ Pce mailing list -- pce@ietf.org To unsubscribe send an email to pce-le...@ietf.org
