Re: [yang-doctors] Yangdoctors last call review of draft-ietf-rtgwg-qos-model-06

[Mahesh Jethanandani <[e-mail]>]

Hi Juergen,

I would agree that the document is not ready for a LC, or for a YANG doctors review at this time. If a YANG doctors review was requested, it was in error.

The authors are working to address the YANG doctors review comments you provided from your last review. The next version (-07) that is going to be published would have only partially addressed those comments. To that list we will add the comments you provide below. Once we believe we have addressed all the comments, we will ask for a review.

Cheers.

> On Mar 6, 2022, at 9:26 AM, Jürgen Schönwälder via Datatracker <[e-mail]> wrote:
> 
> Reviewer: Jürgen Schönwälder
> Review result: Not Ready
> 
> 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.
> 
> 
> 


Mahesh Jethanandani
[e-mail]