Hi, I have been asked to provide a Routing Directorate review of this document as it is prepared for working group last call. This document provides a set of seven YANG modules that can be used for configuring and monitoring aspects of QoS within a packet-forwarder (router). It has no direct impact on routing or direcitonal forwarding, although the per-hop behavior determines the queuing, priority, and drop treatments received by packets. I am not a YANG expert, but have tried to look through the modules for issues of consisency. The issues reported below are classified as Major, Minor, and Nits. I believe more work is needed before this document advances. Regards, Adrian == Major == I believe it is your intention that this document is limited to applicability in IP networks (probably v4 and v6). The scoping, however, is only something that is picked up along the way (for example, no mention of MPLS or its TC bits). I suggest changing the Title, Abstract, and Introduction so that they all explicitly say, "... in IP Networks" --- 1.3 I was puzzled by the definition of PHB in this section. You have: * Per-Hop-Behavior (PHB): The externally observable forwarding behavior applied at a DS-compliant node to a DS behavior aggregate. But this seems very simplistic as compared to the discussion in 2474. I wondered whether you are deliberately restricting your definition (in which case it might be helpful to call this out), or accidentally not showing the full picture. --- 4.7 I may have misunderstood, but I though all modules named iana-* were handed over to IANA to be maintained by them. They normally contain enumerations that match protocol codepoints defined in related protocol specs and come with the instructions to IANA to update the module as and when additions are made to the protocol registry for the related codepoints. Such registries are clearly defined with instructions to IANA. Take as a reasonable example the iana-bfd-types module defined in RFC 9127. What you have here seems more generic - it looks like a collection of base types for use in the various modules defined in this document. That is a fine thing to package in a module, but I don't think it makes for an IANA module. Possibly you can just fix this by changing the module name to ietf-qos-types == Medium == 1. DiffServ is a preferred approach for network service providers to offer services to different customers based on their network Quality- of-Service (QoS) objectives. Well it may be. By whom is it prefered? By the people selling the kit, by the customers, by my mother? And anyway, can you substantiate this? Perhaps just cut out the marketing, and say what DiffServ can do and what this document does. == Minor == I am disapponted not to see an RFC 7942 implementation status section for this document. It is a big piece of work with a lot of detail, and a huge history (April 2016!), so it would be very helpful to understand whether parts or all of the YANG model have been implemented, and even what the experience is. Are there pieces that are not implemented and remain unlikely to be implemented? --- 1. 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? --- 1. 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. --- 2. 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"? --- 3. para 1 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). --- 4.1 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. --- 4.1 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. --- 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. --- 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). --- 4.2 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? --- 4.2 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' --- 4.4 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. --- 4.7 identity count { if-feature count; base action-type; description "Action type defined as count."; } Is this name a bit too close to the --- 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. == Nits == There is a line that is too long on page 104. idnits points this out, so you don't have many excuses. --- The document would benefit from an English language review by a native speaker. I think that the WG may be able to provide one: the authors' home organisations certainly contain such people. Note that, although the RFC Production Centre will do their best to clean up the document, you run a risk with their changes because they are not experts in the technology. I have picked up some of the issues during my review, but certainly not all of them. --- 1. s/parameters. Traffic/parameters. The Traffic/ s/target. QoS Action/target. The QoS Action/ s/in QoS Operational module/in the QoS Operational module/ --- 1. Traffic policy is augmented to include packet match and action parameters to define the Differentiated Services (DiffServ) policy, Queues policy and Scheduler policy. Should have a reference for DiffServ --- 1. OLD Each of these policies are defined as separate modules. NEW Each of these policies is defined as a separate module. END --- 1.3 and other documents This is not very helpful to the reader. Indeed, some of the definitions either need references or additional text to help explain them. For example, in "Policing" you mention "traffic profile" with no indication of what that might be. Another example is "WRED" which is completely unexplained. --- 1.3 s/Classifier: an/Classifier: An/ s/Internet protocol/Internet Protocol/ s/Marking: the/Marking: The / s/Metering: the/Metering: The/ --- Please check and decide on "codepoint" or "code point" --- 1.3 Is there intended to be a difference between * DS code point: A specific value of the DSCP portion of the DS field, used to select a PHB. and * DSCP: Differentiated Services Code Point --- 1.3 * Marking: the process of setting the DS codepoint in a packet based on defined rules; pre-marking, re-marking. Unclear what pre-marking and re-marking are meant to mean here. --- 2. para 1 Ends with a spurious " --- 2. s/The above diagram/Figure 1/ --- Figure 1 You might s/Oper/Operations/ There is space in the figure --- 2. You have source IPv4 address prefix Maybe source IP address prefix --- 2. s/to be referred in a policy/to be referrenced in a policy/ s/group and action parameters/group, and action parameters/ s/Queue Policy with it/a Queue Policy with them/ --- 3. the subset of traffic which may receive a DiffServ Just sounds odd because you have previously said that DiffServ means "Differentiated Services". Perhaps you could... the subset of traffic which may receive a differentiated service --- 3. para 3 I find this paragraph very hard to read. I'd like to be able to make suggestions for improvement, but sadly I am unable to extract enough meaning to do this. Could you have a look and see whether you can claify the text? --- 4.1 OLD Traffic Policy module contains list of classifiers identified by a classifier name. NEW The Traffic Policy module contains a list of classifiers identified by a classifier name. END --- I don't think that the YANG model definitions need to be presented as figures. --- Figure 4 The tabbing to "string" seems a bit excessive --- 4.2 grouping child-policy { container child-policy { if-feature qos-types:child-policy; leaf name { type string; description "A unique name for hierarchical policy."; } description "Hierarchical Policy configuration container."; } description "Grouping of Hierarchical Policy configuration."; } Would it be better if the grouping/container had a different name from the QoS type? --- 4.4 Metering statistics consist of meters identified by an identifier. Well, yes, that is what identifiers do. But is there anything more helpful you can say? --- 4.7 The QoS Types module contains all the type definitions used by the other QoS modules. Well, not "all" as some are imported from other places or are base YANG types. ---