Subject: [Dots] Yangdoctors last call review of draft-ietf-dots-telemetry-14

From: Jan Lindblad via Datatracker <[email-masked]>
Date: Thu, 26 Nov 2020 03:36:41 -0800

Reviewer: Jan Lindblad
Review result: Almost Ready

Dear Dots Authors, NETMOD WG,

This is my YD LC review of draft-ietf-dots-telemetry-14. This draft contains
two YANG modules, ietf-dots-telemetry and module ietf-dots-mapping. Both
modules are unusual from a YANG perspective in that they consist of solely
typedefs, groupings and sx:structures, and that their purpose is to define
message bodies for a domain specific management protocol.

Since my previous review of this module (-09, June 2020) the underpinnings of
the modules have changed significantly with a new RFC in the works, addressing
the fundamental issues pointed out at that time. Very good & impressive. I
still think there are important issues to resolve, so I will call the current
state of -14 "Almost Ready".

Links to previous relevant reviews:
https://datatracker.ietf.org/doc/review-ietf-dots-rfc8782-bis-00-yangdoctors-early-aries-2020-09-23/
https://datatracker.ietf.org/doc/review-ietf-dots-telemetry-09-yangdoctors-early-lindblad-2020-06-30/

Because of this hitherto unusual application of YANG, the usual YD review
procedures are not really applicable. Whoever reads this should feel free to
comment on which perspectives should be included (or not) in an YD review of a
YANG module defining a new protocol.

I think the YD review should not be expected to cover the
usability/interoperability of the newly defined protocol as a whole. E.g. which
messages are sent when, what is the relevant/reasonable content in each
message, are there any races or scalability issues, etc. IMO, such an analysis
is essential, but too much work for an YD review (and outside my area of
expertise). I have placed my review focus on the clear interpretation of the
YANG modules, since that will be key to interoperability.

When YANG is mapped to NETCONF or RESTCONF, there are entire RFCs devoted to
describing how that mapping is done, from what "mandatory" actually means in
the protocol and how keys are sent, all the way to how the XML or JSON payloads
are constructed and potential error messages. A lot more of that mapping is now
present in this document, but I still find many aspects that are
undefined/unclear.

#1 mandatory true

A few elements are marked mandatory true in the model. The meaning of this is
not defined in the draft. I would expect this means clients as well as servers
MUST provide a value for the mandatory element in each message if the parent
element exists. This would be like how mandatory elements in YANG
actions/rpcs/notifications work (and different from how mandatory works with
configuration data in NETCONF/YANG).

Section 7.1 states: "DOTS telemetry attributes are optionally signaled and
therefore MUST NOT be treated as mandatory fields in the DOTS signal channel
protocol." This blanket statement makes things less clear for me. Can I trust
the YANG mandatory and key statements to indicate what has to be included, and
everything else is optional? If so, I would advise to state just that.

#2 key

There are many key statements around these modules. The exact meaning of the
key keyword in this dots-signal protocol is not defined. Are key sets always
unique in a list? Are keys mandatory? Are there any sequencing requirements on
keys, e.g. do keys need to be sent first (as in NETCONF) in a list element?
Such a requirement may not be unreasonable for servers with limited resources.

#3) if-feature

The ietf-dots-mapping.yang module defines one YANG feature and a few instances
of model statements conditional on it with an if-feature statement. There is a
description for how a server advertises support for this specific feature (by
including a very particular leaf in one of the messages). From a YANG mapping
perspective, it might have been better to have a general mechanism for
advertising features. With the current specification, no other features can
(ever) be added in a backwards compatible way.

#5) augment or sx:augment-structure

The draft is using augment (but means sx:augment-structure) and
sx:augment-structure. Are implementors allowed to use augment in their
implementations? If so, how would a client know if an augmented model is in
use? Section 6.1.2 states that servers MUST reject messages when "attribute
values are not acceptable". Does that have any implications for augment?

#6) deviations

Deviations are never mentioned. Are implementors allowed to deviate from the
protocol in any way, if declared using deviation statements? If so, how would a
client know if a deviated model is in use?

#7) config true|false

RFC 8791 (which defines sx:structure) expressly states that config true and
false statements have no defined meaning and will be ignored in sx:structure.
Therefore it's very good that this draft defines behavior for the GET operation
with respect to config true and config false elements. Nothing is said about
how the config attribute plays with PUT, POST or DELETE however.

#8) PATCH

Is the PATCH HTTP operation supported in this context? It is never mentioned.
If so, it's is unclear how it would work in relation to the protocol HTTP
headers, the tsid mechanism and the YANG mandatory concept. If not, that might
be good to point out somewhere.

#9) Payload encoding references

As far as I understand, the message payloads are meant to be encoded in JSON or
CBOR. This draft has no references to the relevant documents describing these
encodings (https://tools.ietf.org/html/rfc7951 and
https://tools.ietf.org/html/draft-ietf-core-yang-cbor-13).

And now a few minor comments on particular points or wordings.

#10) The tree diagram in Fig. 30 is not using the correct format (see the other
tree diagrams).

#11) Section 6.3.1 states "The overlapped lower numeric 'tsid' MUST be
automatically deleted and no longer be available." Since this mechanism is a
central part of the protocol, I would think specifying exactly what
"overlapped" means would improve interoperability.

#12) Section 6.3.1 states "If no target clause is included in the request, this
is an indication that the baseline information applies for the DOTS client
domain as a whole." Define "target clause" (= tsid?)

#13) Section 4.5 states "The use of "identities" is thus suboptimal from a
message compactness standpoint." Clearly, but they excel in forward
extensibility. There may be cases where the identities would be better and even
more compact than an alternative solution without them. To rule out all use of
identities based on a blanket statement like this does not seem like a good
design practice. I suggest to remove this sentence.

#14) In the YANG, there is a choice with a single case "case
server-to-client-only". This way of using choice is a little unusual, but it
works.

#15) In the YANG, there is a list telemetry without any key. This is allowed
for config false lists in NETCONF/YANG, and it could be allowed in this
protocol. For a constrained device, however, this situation may not be ideal.
Without a key, the only way for a client to read this information is to read it
all at once. If that is the only reasonable use case for this information, you
kan keep it as is. If this data may be large, a different way of modeling may
be in order so that data could be requested a little at a time.

#16) Section 6.1.1 states " 'server-originated-telemetry' under
'max-config-values' to 'true' ('false' is used otherwise).  If
'server-originated-telemetry' is not present in a response, this is equivalent
to receiving a request with 'server-originated-telemetry' set to 'false'. " In
YANG, this is usually expressed by adding "default false" to the leaf.

#17) There are many leafs in the models that have no default, no mandatory and
no description which tells what happens if the leaf is not provided. E.g. what
happens if "query-type" is not specified?

#18) Many lists have "unit" as a key. This can work, but is an unusual choice.
It opens up for sending the same information several times over with different
units.

#19) Section 7.1.1 states "At least one of the attributes 'target-prefix',
'target-fqdn', 'target-uri', 'alias-name', or 'mid-list' MUST be present in the
target definition." This may be a good thing to add in a must-statement, or at
least in a description statement in the module.

#20) Section 7.1.1 mentions a "optional subattribute" Please define this term,
or change to a defined term.

#21) leaf-list alias-name {type string; description "An alias name that points
to a resource."; Unclear; what is a resource here? What is the format of the
string? Could this be a leafref?

#22) There are typedefs called "unit-type" and "unit". I would suggest changing
at least one of those names so that readers have a chance at understanding how
they differ. Also, some of their content overlaps, so maybe one could
incorporate the other?

#23) Similarly for grouping percentile-config and grouping percentile. Names
that better reflect their content and usage would be good.

#24) There are still a few instances of augment in the model. They should be
sx:augment-stucture.

#25) I didn't go through the entire table in section 11, but I looked at a few
random leafs from the model. A couple seem to be missing in section 11, e.g.
last-updated and rate-limit. Maybe good to go through the latest version to
really check that all relevant elements are listed.

><

---

From: Andy Bierman <[email-masked]>
Date: Thu, 26 Nov 2020 09:45:45 -0800

> message bodies for a domain specific management protocol.

This is consistent with the intent of RFC 8791.

People have realized that a formal message definition that can be integrated
into the tool chain is much better than 100 pages of ASCII art.  Augments,
annotations, and deviations are not only possible, but critical to
deployment.

But you are raising an important point -- it is difficult to review
groupings without any context.
(Without any "uses").

Since my previous review of this module (-09, June 2020) the underpinnings
> of

Agreed.  The protocol document review is a separate matter.

> present in this document, but I still find many aspects that are

But this is related to the message encoding, not the YANG definitions.
YANG is (trying to be) encoding-independent, and the YANG to CBOR and SID
drafts explain
how to encode any type of YANG data in CBOR.

Andy

> _______________________________________________
> yang-doctors mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/yang-doctors
>

---

From: <[email-masked]>
Date: Fri, 27 Nov 2020 06:22:13 +0000

Hi Andy, all

Thanks.

One clarification in reference to this comment:

« But you are raising an important point -- it is difficult to review groupings without any context.
(Without any "uses"). »

Please note that all defined groupings are used in this draft.

Cheers,
Med

De : Andy Bierman [mailto:[email-masked]]
Envoyé : jeudi 26 novembre 2020 18:46
À : Jan Lindblad <[email-masked]>
Cc : YANG Doctors <[email-masked]>; [email-masked]; [email-masked]; [email-masked]
Objet : Re: [yang-doctors] Yangdoctors last call review of draft-ietf-dots-telemetry-14

On Thu, Nov 26, 2020 at 3:36 AM Jan Lindblad via Datatracker <[email-masked]<mailto:[email-masked]>> wrote:
Reviewer: Jan Lindblad
Review result: Almost Ready

People have realized that a formal message definition that can be integrated
into the tool chain is much better than 100 pages of ASCII art.  Augments,
annotations, and deviations are not only possible, but critical to deployment.

But you are raising an important point -- it is difficult to review groupings without any context.
(Without any "uses").

But this is related to the message encoding, not the YANG definitions.
YANG is (trying to be) encoding-independent, and the YANG to CBOR and SID drafts explain
how to encode any type of YANG data in CBOR.

Andy

_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.

---

From: Andy Bierman <[email-masked]>
Date: Sat, 28 Nov 2020 04:59:11 -0800

> Please note that all defined groupings are used in this draft.

I meant "visible" uses-stmt.

   - The sx:structure statement is external to the YANG language. It is
   (literally) not part of YANG.
   - The mapping of YANG restrictions in the description-stmt for this
   structure is intentionally vague.
   - There is no mapping to NMDA or any concept of a datastore
   - There is no mapping to any YANG defined content for NETCONF or RESTCONF
   - There is no mapping to NACM or any concept of access control

Jan is assuming that this YANG module needs to be reviewed within the
context of specific
(standard) management protocols. I don't think YANG Doctors ever discussed
the guidelines
and review requirements for standard structures. IMO structure-only
documents are OK
and can be reviewed for YANG correctness, consistency, and maintainability.

I would expect any protocol that uses the structure extension to address
all the missing mappings
listed above.

Cheers,

Andy

>
>
> *De :* Andy Bierman [mailto:[email-masked]]
> *Envoyé :* jeudi 26 novembre 2020 18:46
> *À :* Jan Lindblad <[email-masked]>
> *Cc :* YANG Doctors <[email-masked]>; [email-masked];
> [email-masked]; [email-masked]
> *Objet :* Re: [yang-doctors] Yangdoctors last call review of
> draft-ietf-dots-telemetry-14
>
>
>
>
>
>
>