IETF Last Call Review of draft-ietf-rtgwg-qos-model-13
review-ietf-rtgwg-qos-model-11-rtgdir-lc-farrel-2023-11-16-01

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.

===

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?

---

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.

---

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.

---

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.

---

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"?

---

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).

---

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.

---

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.

---

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.

---

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).

---

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?

---

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'

---

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.

---

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?

---

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.

---

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.