Re: [yang-doctors] Yangdoctors last call review of draft-ietf-dots-telemetry-14

[Andy Bierman <[e-mail]>]

On Thu, Nov 26, 2020 at 3:36 AM Jan Lindblad via Datatracker <
[e-mail]> wrote:

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

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

Agreed.  The protocol document review is a separate matter.



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

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



> #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.
>
> ><
>
>
> _______________________________________________
> yang-doctors mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/yang-doctors
>