Hi Adrian, Thanks so much for the comments. Please see my comments starting with AC.
Regards, Aseem Get Outlook for Mac <https://aka.ms/GetOutlookForMac> From: Adrian Farrel via Datatracker <[email protected]> Date: Thursday, October 9, 2025 at 3:48 AM To: [email protected] <[email protected]> Cc: [email protected] <[email protected]>, [email protected] <[email protected]> Subject: [rtgwg] draft-ietf-rtgwg-qos-model-13 early Rtgdir review Document: draft-ietf-rtgwg-qos-model Title: YANG Models for Quality of Service (QoS) in IP networks Reviewer: Adrian Farrel Review result: Not Ready (per my updated review : -11 updated to -13) I began my re-review by looking through my previous review (or -11) and checking the text to see whether my comments had resulted in a change to the text. I see a number of points that are still open (most of the "minor" points and even one of my "major" points. Had you responded to my review by email, this would have been a lot easier to check and resolve. Absent that, I think that the document is still not ready and I see no value in doing a full review of the document. Sorry to be pissy (well, sorry to need to be pissy), but I don't feel that my earlier review has been valued or treated with respect. AC: Sorry for making you feel like that. === In your new text in 1.3 you have: * Per-Hop-Behavior (PHB): A description of the externally observable forwarding treatment applied at a differentiated services- compliant node to a behavior aggregate [RFC2474]. The description of a PHB SHOULD be sufficiently detailed to allow the construction of predictable services, as documented in [RFC2475]. You use of "SHOULD be" implies that it is OK (under some unspecified circumstances) to have a PHB that with a description that is no sufficiently detailed to allow predictable services. Why? AC: The definition has reference to RFC2474 which uses similar text. --- I assume that the continued absence of an Implementation Status section means that there are currently no full or partial implementations and no known plans for implementation. In this case, why is this document being pushed for publication on the Standards Track. Ultimately, this is a quesiton for the chairs, the responsible AD, and the IESG, but it seems to me that something as complex and detailed as this YANG model deserves some code to determine whether it is sound. AC: This document aligns with vendors config model. — Section 1 You didn't answer my comment: > Traffic > Policy module defines the basic building blocks to define a > classifier, policy and target. > > Is the reader meant to assume a normative definition of these three > things? Maybe add "as defined in Section 3" except that only "classifier" is defined there, so you'd have to add definitions for policy and target. AC: Do you mean section 1.3? I see the point. I will add this. --- Section 1 You didn't answer or otherwise address my comment: > Traffic policy is augmented to include packet > match and action parameters to define the Differentiated Services > (DiffServ) policy, Queues policy and Scheduler policy. > > This use of the passive voice leaves the reader unclear about who takes > what actions, when, and where. AC: I will add some more description here. I will update in next version. --- Section 2 You didn't answer or otherwise address my comment: > I think you should introduce and expand on the meaning of "Target" in > this section. You only have: > The target is assumed to be interface > but the groupings can be used for any other target type where QoS > policy is applied. > Right at the end of a section. Indeed, what is a "target type”? AC: I agree. I will add the text. --- 3. para 1 You didn't answer or otherwise address my comment: > and is used to select a Per Hop Behavior (PHB) > > It may be helpful to identfy where the PHB is selected. Indeed, a single > DSCP (for a BA) may be mapped to a different PHB at each node in the > network (and, of course, multiple DSCPs may map to the same PHB at > multiple nodes). AC: sure, I will add these details in next version. --- 4.1 You didn't answer or otherwise address my comment: > Each classifier MAY contain a list of filters. > > This is fine, but usually we also give some advice on when or why a > "MAY" might be chosen. AC: this is difficult to describe customer use-cases here in a very generic way. --- 4.1 You didn't answer or otherwise address my comment: > I think some text should be added to describe Figure 2. > > Ditto Figure 4 in Section 4.2, Figure 6 in 4.3, Figure 8 in 4.4, > Figure 10 in 4.5, and Figure 12 in 4.6. AC: I will take care in next version. --- Notwithstanding the rename of iana-qos-types to ietf-qos-types, you didn't answer or otherwise address my comment: > I found it slightly odd that iana-qos-types is not defined until 4.7 > even though it is heavily imported by the other modules. I suppose this > isn't important, but forward pointers would have helped. AC: I will add the reference earlier. --- You didn't answer or otherwise address my comment: > ietf-traffic-policy has > > list qos-target-policy { > key "direction type"; > description > "Policy target for inbound or outbound direction."; > : > : > leaf name { > type string; > mandatory true; > description > "A unique name for the Policy."; > } > } > > While I think this a fine objective, I am not clear that this MUST be > unique. It's not a key, so it could be non-unique if a user wants to > confuse themselves? > > So perhaps change to "A name for the Policy." > > Same comment for a number of names in all of the modules (but noting > that many of them *are* keys so uniqueness is required). AC: I agree. This will be fixed in next version. --- 4.2 You didn't answer or otherwise address my comment: > grouping priority { > container priority { > leaf level { > type uint8; > description > "Priority level."; > } > description > "Container for priority."; > } > description > "Grouping for priority."; > } > > Might it be nice to give a clue on whether 0 or 255 is the high priority? AC: Agree. This will be updated in next version. --- 4.2 You didn't answer or otherwise address my comment: > grouping count { > container count { > if-feature qos-types:count; > > leaf count-action { > type empty; > description > "Take an action if this is defined."; > } > description > "Container for whether to take an action for count."; > } > description > "The count action grouping."; > } > > I think that it would be better to name 'count' here and in qos-types > to something less specific because it looks too close to something that > might be a generic YANG type (like counter32, etc.). Maybe ‘qoscount’ AC: I agree. This will be updated in next version. --- 4.4 You didn't answer or otherwise address my comment: > QoS Operational module contains all operational statistics. It > contains statistics related to classifier, metering, queueing, and > named. > > Is this right? "...and named" seems very odd. AC: I agree on the comment. I can elaborate on named counters. --- 4.7 I apologise for not entering a full comment here: > identity count { > if-feature count; > base action-type; > description > "Action type defined as count."; > } > > Is this name a bit too close to the This is just plain confusing! Is it a count or an action? AC: This is count as an action to enable counting for a policy. This is to address low-end devices which doesn’t have resources to maintain counters. --- You didn't answer or otherwise address my comment: > I think Appendix B might benefit from some explanation of what values it > adds to the document. As it stands, it is left as an exercise for the > reader. AC: Appendix B provides typical companies implementation details and is provided as reference. I think it is better to keep it. I think some more text can be added upfront. --- I am not oging to check all of the nits I raised in my previous review. I assume that the authors will want their document to read well and be of value to the comunity, and so will work to fix them all. However, given that the idnits line-too-long warning still exists, I have no confidence that any of the nits has been resolved so far. AC: I will check and fix them all. _______________________________________________ rtgwg mailing list -- [email protected] To unsubscribe send an email to [email protected]
_______________________________________________ rtgwg mailing list -- [email protected] To unsubscribe send an email to [email protected]
