Hi Mahesh, On Wed, Dec 18, 2024 at 6:20 PM Mahesh Jethanandani <mjethanand...@gmail.com> wrote:
> Hi Dhruv, > > Responding to the second comment/question. > > On Dec 17, 2024, at 1:02 PM, Dhruv Dhody <d...@dhruvdhody.com> wrote: > > > 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? > > > It is generally true that if you want to clear stats for each instance, > you have to call it in a loop. In some implementations, the ‘*’ character > is used to indicate all/loop. > > I do not have the draft in front of me, but what is the input to the rpc? > If the rpc is supposed to clear all the stats for all the peers, there has > to be one container that contains all the stats for all the peers. Your > current structure is the opposite. You have stats per peer. You do not have > one container/list you can call to clear its contents. Am I making sense? > > Dhruv: The current rpc to clear stats takes no input rpc statistics-reset { description "Reset all the statistics collected."; } This is similar to what I saw in RFC 8808 where rpc factory-reset takes no input. My preference to use rpc to clear all statistics is because I dont have a clear container where I can add the 'action' command, as I have done it for the per-peer stats case! Thanks! Dhruv > Cheers. > > > 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
