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. > 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). > 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
_______________________________________________ Pce mailing list -- pce@ietf.org To unsubscribe send an email to pce-le...@ietf.org
