Hi Dhruv, Hi Dhruv,
Responding to the second comment/question. Hi Dhruv,
Thanks for addressing most of my comments. But ...
On Mon, Dec 16, 2024 at 4:01 PM Mahesh Jethanandani via Datatracker < [email protected]> 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.
I do not have the full context for factory-reset but if it is what the name suggests, it does not need a pointer to a container. There is probably a global routine that needs to be called. That is why it can get away by not having an 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!
I understand what your preference is. However, your model does not support that. Walk me through the scenario where you register a call point for your rpc, and you get called. What do you do? How do you get to all the containers that contain the stats.
Besides, if by clearing all stats you mean both peers and sessions, then we are talking about two different sets of containers, which requires even more context to be provided to clear all the stats.
Cheers
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.
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.
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?
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"
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.
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.
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.
|
