Re: [RTG-DIR] Rtgdir last call review of draft-ietf-rtgwg-qos-model-11

[tom petch <[e-mail]>]

From: rtgwg <[e-mail]> on behalf of Adrian Farrel via Datatracker <[e-mail]>
Sent: 16 November 2023 10:38
Reviewer: Adrian Farrel
Review result: Not Ready

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.

<tp>
I am not a YANG expert but have a passing acquaintance with YANG and would agree with Adrian's comment of 'Not Ready' based purely on YANG issues.

There are a number of minor glitches with one show-stopper which Adrian has identified, namely
'I may have misunderstood, but I though all modules named iana-* were
handed over to IANA to be maintained by them.'

I agree but this I-D seems confused.  The module name has IANA but the prefix does not, there is no identification of the initial contents, there is no guidance to IANA as to who can request an update to this and how; seriously inadequate.

I think that the choice of an IANA-maintained module here is unsuitable, leading to trouble down the line and I see no benefit it having such.

This needs fixing ie a consensus on whether or not to proceed down this route.  As I say, I think it a show-stopping mistake.  When that is resolved, with a revised I-D, I will make further comments, the first of which is that it needs examples, not of further modules but of XML using the existing modules.

Tom Petch





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.

---


_______________________________________________
rtgwg mailing list
[e-mail]
https://www.ietf.org/mailman/listinfo/rtgwg