Reviewer: Jürgen Schönwälder
Review result: Not Ready
- You may want to align the document title with common popular titles,
see https://en.wikipedia.org/wiki/YANG#Standards-track_data_models,
e.g.:
A YANG Data Model for Quality of Service (QoS)
Perhaps it also makes sense to expand the abstract a bit. What is
the difference between 'configuration' and 'operational parameters'?
- What is a 'data module'? We usually have 'data models' and 'YANG
modules'.
- There are language quirks (e.g. missing articles, singular/plural
confusion) that someone should look at and fix. An example:
[...] Differentiated
Services (DiffServ) module is an augmentation of the base QoS model.
Remote Procedure Calls (RPC) or notification definition is not part
of this document. QoS base modules define a basic building blocks to
define a classifier, policy, action and target.
- There is always the question of how many modules does it take to do
something.
- I find subsection 1.1 weird (I dislike one sentence (sub)sections
from a stylistic point of view), simply move the statement into the
section where you define terminology. Well since we are it, I am
surprised that there is no content specific terminology defined or
imported. I assume you want to import basic DiffServ terminology
from somewhere, perhaps other terminology as well. You obviously
assume that people know what a classifier etc. is.
- "A classifier consists of packets which may be grouped" What? A
classifier consists of packets? Please import definitions instead
of making up your own ones. RFC 2475 for example says:
Classifier an entity which selects packets based on
the content of packet headers according to
defined rules.
- "A meter qualifies if the traffic arrival rate is based on agreed upon
rate and variability." What? RFC 2475:
Meter a device that performs metering.
Metering the process of measuring the temporal
properties (e.g., rate) of a traffic stream
selected by a classifier. The
instantaneous state of this process may be
used to affect the operation of a marker,
shaper, or dropper, and/or may be used for
accounting and measurement purposes.
- "This module imports definitions from "Common YANG Data Types"
[RFC6991] and "A YANG Data Model for Interface Management" [RFC8343]."
What is "this module" referring to? There are several modules. Do we
really need that many? Should the decision to break things in to rather
small pieces be justified somewhere?
In general, the statement about where definitions are imported from is
in the modules sections, not in the terminology section.
- There are a number of MIB modules for DiffServ and it might be
useful to explain how the YANG modules related to the MIB modules.
- The module names suggests that QoS == DiffServ, which I am not sure
is necessarily true. Given that these modules seem to be DiffServ
specific, it may be better to use the prefix ietf-diffserv- instead
of ietf-qos-
- The authors should consider their naming scheme. Repeating words in
identifiers makes instance documents verbose and hard to read:
<classifiers>
<classifier-entry>
<classifier-entry-name>foo</classifier-entry-name>
<classifier-entry-descr>some foo</classifier-entry-descr>
...
<classifier-entry>
<classifiers>
vs
<classifiers>
<classifier>
<name>foo</name>
<description>some foo</description>
...
<classifier>
<classifiers>
- In section 5 first module, you should explain where the filter
entries are coming from. Apparently the idea is that the filters
are augmented in but that should be explained.
- It might be useful to explain which features are defined.
- Why do you label some rw objects as -cfg? Is not everything rw here
config?
- Why is a classifier-action-entry-cfg associated to a classifier and
not to a policy entry or a list of classifiers? I do not recall
enough about DiffServ to recall the underlying model, it just feels
a bit inflexible.
- You seem to be inconsistent with nodes for lists. In ietf-qos-classifier
you have
+--rw classifiers
+--rw classifier-entry* [classifier-entry-name]
but in ietf-qos-policy you have
+--rw classifier-entry* [classifier-entry-name]
- Named meters are called meter templates? Named classifiers are named
classifiers??
- Should ietf-qos-target be renamed to ietf-interface-diffserv and
should it not be
augment /if:interfaces/if:interface:
+--rw diffserv-policy* [direction type name]
+--rw direction identityref
+--rw type identityref
+--rw name string
instead of
augment /if:interfaces/if:interface:
+--rw qos-target-entry* [direction policy-type]
+--rw direction identityref
+--rw policy-type identityref
+--rw policy-name string
(Also note that I added the policy name to the key!) Of course, if
one would assume that names are unique across all policies, then the
type leaf would not be needed but you seem to assume that it is a
feature that I can have multiple policies with the same name and
different types.
- Why is this: "Any queuing, AQM and scheduling actions are part of
vendor specific augmentation."?
- It needs to be clear how filters combine.
- Should some of the lists that have a single leaf be turned into
leaf-lists? Or is the reason to enable augmentation in those lists?
- Calling a leaf -addr but using inet:ipvX-prefix is inconsistent. I
think you want to use inet:ipvX-prefix and hence you should adapt
the names to source-ipv4-prefix etc.
- I suggest to not UPPERCASE the module names in section titles.
- Copyright says 2019 - time to move to 2021...
- I am not sure I understand feature classifier-template-feature from
the description of it. I am actually lost on the possible
combinations of policy-inline-classifier-config,
classifier-template-feature, match-any-filter-type-support, perhaps
because the descriptions are messed up.
- Please try to pick good names. I find
'classifier-entry-filter-operation-type' overly complicated. Perhaps
'match-filter-operation' is better matching the derived identities...
I guess I would even go for
filter-match-operation
+- filter-match-all
+- filter-match-any
Finding good names is hard but matters a lot.
I am lost on the match-any-filter-type-support, why is
filter-match-any a feature but filter-match-all not?
- Descriptions need to improve. Some have language issues, others are
simply too short and not very descriptive. I think we like to have
full descriptive sentences in standard YANG modules.
- I do not understand filter-logical-not. This seems to negate
filter-match-all and filter-match-any. But what do the combinations
really mean? I assume 'not any' means none, I am not sure what 'not
all' means, perhaps 'any or none'? Why not simply add another
filter-match-operation, e.g., filter-match-none?
- Why do we need two groupings for named and inline classifiers? If we
need two groupings and they are only used once, why are they defined
as groupings? Why do I need this classifier-entry-inline boolean?
What does it mean to have this boolean set to false in an inline
classifier??
- Why once classifier-entry-filter-operation and then
classifier-entry-filter-oper? Why is the inline classifier defining
the list filter-entry but the named classifier is not? Perhaps there
are reasons but then they should be documented.
- I am not sure about the choice of prefixes, but there is perhaps no
consensus on how to choose prefixes. That said, prefixes like
"target" seem rather generic (but as long as others do not pick such
generic prefixes...).
- Do not write "Latest revision for qos actions" if it is the initial
revision. Ideally, revision statements do not need to change when a
new revision is created. If you write 'latest revision', well, you
have to go and make changes.
- "This feature allows support of meter-template." OK, but what is a
meter template?? How do the features meter-template-support,
meter-inline-feature, meter-reference-feature interact? Can I
implement meter-reference-feature without meter-template-support?
Does it make sense to implement meter-template-support without
meter-reference-feature? The descriptions do not really help in
describing things.
- Is there a way to reduce the number of features? Is it possible to
define a baseline that everybody commits to implement? Client
complexity grows quickly with the number of possible feature
combinations.
- For some of the groupings, it is not clear why they exist. Where is
the grouping drop used? Oh, you refer to it using action:drop in
the appendix, i.e., its only there for extensions??
- I am not an expert qualified to check whether the various
meter-action-params make sense. This should be verified by people in
the WG.
- What does priority level 7 compare to 8? What is higher and what is lower
priority?
- Putting the description statements at the end of nested
container/choice/ case definitions looks pretty weird since you get
a sequence of closing curly braces with descriptions.
- You seem to support an inbound policy and an outbound policy. Can
there be more? I am asking since you use a construction with an
identity. If its only inbound or outbound, you could instead simply
do
inbound-diffserv-policy
+-- type
+-- name
outbound-diffserv-policy
+-- type
+-- name
and if the names would be unique this would further reduce to
augment /if:interfaces/if:interface:
+-- inbound-diffserv-policy string
+-- outbound-diffserv-policy string
- You probably want to look at all places where you have name
references and detail what happens if the names do not resolve to
something. Do you require referential integrity or will policies not
be applied or applied in a different manner if referential integrity
is not given?
- I just now realize that qos-target-entry is a list. So I can apply
multiple inbound and multiple outbound policies. What is the result
of this?
- The source and destination port groupings have a reference to UDP
and TCP. Does this imply that these groupings do not work for DCCP
or SCTP or any other future transport protocol using port numbers?
Perhaps simply drop the reference and rely on the type definition.
- I wonder to what extend protocol number ranges practically make
sense, but perhaps it is OK to keep the ranges for consistency.
It is unfortunate that we do not have an inet:protocol type defined
in the ietf-inet-types module... Your definition talks about the
'upper-layer' header but I think there is no such thing. I
understand what you mean (ignore all extension headers) but the text
seems to be referring to something that does not really exist.
- Is it possible to reduce the duplication of the filter cases for
named classifiers and inline classifiers? The constraints on both
also seem to differ.
- It seems some of the complexity of the model comes from the attempt
to be extendible and in particular to support vendor extensions. I
guess it would have helped me a lot if the introduction would have
spelled out what the objectives of the data model are. There is a
lot of reverse engineering going on in my head and this is painful.
It would be much simpler if the document would have stated the
objectives of the model.
- I did not run any compilers etc. but I think this is fine since the
modules are not ready anyway.
- IANA considerations missing.
- Security considerations missing.
- The MITRE approval is funny given the IETF process and the note
wells around in every corner.
- The text refers to [RFC3289] for the diffserv architecture but given
that [RFC3289] defines the MIB module, this seems somewhat surprising.
- Why does this document need a normative reference to RFC 6020? Why
is RFC 6020 referred to in a revision statement??
- I ignored the extension examples in the appendix.
- I wonder, has someone tried to implement this? Or is every vendors
doing his own thing anyway and this is more of a standardization
exercise? How does all of this relate to say the open config model?
OK, ignore the last question. ;-)
