Last Call Review of draft-ietf-rtgwg-qos-model-06
review-ietf-rtgwg-qos-model-06-yangdoctors-lc-schoenwaelder-2022-03-06-00

Document: draft-ietf-rtgwg-qos-model-06
Date: 2022-03-06

Summary: The document is not ready, essential parts are missing, other
parts are far from being done. Please do not ask me again to review
this document, YANG doctors are not here to step authors and WGs
through their YANG model writing and reviewing process.	The basic
quality	checks are to be done by the WGs and not YANG doctors. On the
technical side, I am concerned about way too many features and way too
many YANG modules. And it all feels like being designed to support
vendor specific diffserv models more than trying to converge on a
(straight-forward to implement) standard configuration model for
DiffServ.

* 1. Introduction

  s/is a preferred approach/is an approach/

  I asked before why you reference RFC 6020 (YANG 1). I think you
  should be using YANG 1.1 (RFC 7950) and not reference RFC 6020 at
  all.

  There are still some language quirks. My comments concerning
  "sectionitis" (one sentence sections) remain.

* 2. QoS Model Design

  The text defining terms like classifier has been revised but I am
  still not happy about it. I do not see why you not simply import
  terms from other documents. An example:

    A classifier consists of packets which may be grouped when a logical
    set of rules are applied on different packet header fields.

  A classifier consists of packets? Why not import and use the RFC
  2475 definition? Another one:

    A meter qualifies if the traffic arrival rate is based on agreed
    upon rate and variability.

  A meter qualifies? I do not think this is what RFC 2475 says a meter
  is - something that does metering, e.g., counting.

* 3. DiffServ Model Design

  "DiffServ architecture [RFC3289] and [RFC2475] describe" seems to be
  wrong, I think the architecture is in [RFC2475], why do you cite
  [RFC3289] here?

4. Modules Tree Structure

  s/ietf-qos-classifier consists/The ietf-qos-classifier module consists/

  Why do I have both ietf-qos-classifier:/classifiers/classifier and
  ietf-qos-policy:/policies/classifier? An explanation or justification
  seems to be missing here.

  s/ietf-qos-action module contains/The ietf-qos-action module defines/

  s/ietf-qos-target module/The ietf-qos-target module/

  "Many of the YANG types defined in [RFC6991] are represented as
  leafs in the classifier module." I am not sure what this tells me,
  perhaps simply remore this statement or replace it with something
  more useful.

  AQM pops up here our of the blue. At least expand the acronym on
  first use.

* 5. Modules

  Descriptions should be proper sentences.

* 5.1 ietf-qos-classifier

  I wonder whether 'logical-not' would not better be called 'negate'
  and whether this should have a default of false. My assumption here
  is that you evaluate the filter and then you negate the returned
  boolean value. The phrase "filter looks for absence of a pattern
  defined by the filter" seems to complicate things.

* 5.2 ietf-qos-policy

  Why is this YANG 1 and not YANG 1.1?

  The descriptions are rather poor and incomplete. Why was this sent
  to a yang doctor for review? At least one of the co-authors should
  know the YANG guidelines document...

* 5.3 ietf-qos-action

  There are several identities with the exact same description. Does
  this mean all these identities do the same thing? The quality of all
  description statements needs to improve. Also put the statements
  into canonical order (see grouping queue which has descriptions at
  the end instead of the top).

* 5.4 ietf-qos-target

  Why is this YANG 1 and not YANG 1.1?

  Where did you get the WG chairs from?? Please follow the template
  given in RFC 8407 appendix B. We did cut the template down...

* 5.5 ietf-diffserv

  Where did you get the WG chairs from?? Please follow the template
  given in RFC 8407 appendix B. We did cut the template down...

  Do you really want a list source-ipv4-prefix or do you want a
  leaf-list source-ipv4-prefix?

  Do you really want a list destination-ipv4-prefix or do you want a
  leaf-list destination-ipv4-prefix?

  Do you really want a list source-ipv6-prefix or do you want a
  leaf-list source-ipv6-prefix?

  Do you really want a list destination-ipv6-prefix or do you want a
  leaf-list destination-ipv6-prefix?

  For the protocol-min and protocol-max leafs you could use
  inet:protocol-number if you are OK to depend on RFC6991bis.

* 5.6 ietf-queue-policy

  Where did you get the WG chairs from?? Please follow the template
  given in RFC 8407 appendix B. We did cut the template down...

* 5.7 ietf-scheduler-policy

  Where did you get the WG chairs from?? Please follow the template
  given in RFC 8407 appendix B. We did cut the template down...

* 6. IANA Considerations

  Why is this still empty?

* 7. Security Considerations

  Why is this still empty?

* 8. Acknowledgement

   "MITRE has approved this document for Public Release, Distribution
    Unlimited, with Public Release Case Number 19-3027."

  What? Who cares about what MITRE approves? Do people understand the
  IETF rules and note well?

* Appendix

  I do not review this. Frankly, the purpose of standards is to try to
  find agreement on common solutions and to showcase how easy it is to
  write N different vendor specific Diffserv Models.