Hi Mahesh, On Tue, Dec 17, 2024 at 11:38 AM Mahesh Jethanandani < mjethanand...@gmail.com> wrote:
> 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> 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/ >> >> 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/ >> >> >> >> ---------------------------------------------------------------------- >> 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. > > Dhruv: I understand but I dont see any harm either. I will wait for the AD direction on this as there is a big churn in the model at the very end stage that I personally would like to avoid. To me it looks more of a matter of style and it is not against any written YANG guidelines. > 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. > Dhruv: I understand and I have made a similar change in the ietf-pcep-stats model. But in RFC 9647 I saw "Implementations that want to support a system-wide reset of Babel statistics need to call this action for every instance of the interface." and I am not sure how the special character of "*" comes in? Can I keep the rpc to reset all? Thanks! Dhruv > > 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 > > > >> 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 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), 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 > Status: https://datatracker.ietf.org/doc/draft-ietf-pce-pcep-yang/ > HTMLized: 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 > > Thanks! > Dhruv > > > > Mahesh Jethanandani > mjethanand...@gmail.com > > > > > > >
_______________________________________________ Pce mailing list -- pce@ietf.org To unsubscribe send an email to pce-le...@ietf.org
