Hi all, FWIW, an updated version of the draft is available online: https://www.ietf.org/rfcdiff?url1=draft-ietf-dots-telemetry-14&url2=draft-ietf-dots-telemetry-15&difftype=--hwdiff This version takes into account Jan's review: * add new text to record why keys are not used for some lists + the meaning of keys when such statement are used. * add a reminder how mapping works in DOTS * add a warning against deviations for the signal channel + a reminder how such deviations (and alos features) can be learned for the data channel * further insist on unit auto-scaling * rename some groupings * explain the meaning of "mandatory" use * update some descriptions and some other edits. Cheers, Med > -----Message d'origine----- > De : supjps-ietf@xxxxxxxxxxxxx [mailto:supjps-ietf@xxxxxxxxxxxxx] > Envoyé : vendredi 4 décembre 2020 18:39 > À : 'Jan Lindblad' <janl@xxxxxxxxxx>; yang-doctors@xxxxxxxx; last- > call@xxxxxxxx; draft-ietf-dots-telemetry.all@xxxxxxxx; dots@xxxxxxxx > Objet : RE: [Dots] Yangdoctors last call review of draft-ietf-dots- > telemetry-14 > > Hi Jan, > > Thanks for the detailed review. > > As there has been some responses to different parts, I will try to > answer the parts not responded to, but indicate what has been > responded to. > > Please see inline. > > Regards > > Jon > > > -----Original Message----- > > From: Dots [mailto: dots-bounces@xxxxxxxx] On Behalf Of Jan > Lindblad > > via Datatracker > > Sent: 26 November 2020 11:37 > > To: yang-doctors@xxxxxxxx > > Cc: last-call@xxxxxxxx; draft-ietf-dots-telemetry.all@xxxxxxxx; > dots@xxxxxxxx > > Subject: [Dots] Yangdoctors last call review of > draft-ietf-dots-telemetry-14 > > > > 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. > > Answered by [Andy] [2] > Commented on by [Med] [3] > Answered by [Med] [4] > More clarifications by [Andy] [5] > > > > 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. > > Answered by [Andy] [2] > > > > 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. > > Answered by [Andy] [2] > > [Jon] The goal in the signal channel was to have the payload in CBOR > for packet size optimization, including mapping the CBOR parameter > names into integers to further reduce the data size. This channel is > run over CoAP over (D)TLS. The CBOR mappings are key here, but YANG > is used to describe the different CBOR entities / mappings as far as > possible. So, this is not really the classic YANG where other > mappings can easily be derived from it. > An agent needs to know how to process a particular CBOR > parameter/value and whether it can be safely ignored if it is not > known. > > [Jon] The data channel is essentially RESTONF/YANG over HTTPS where > the YANG in this draft is simply "augment"ing and hence follows the > classic YANG usage. For this draft, the data channel is only used > to exchange vendor mapping details (which the signal channel can > subsequently use with the information reduced as much as possible in > size). > > > > #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." > > Answered by [Med] [4] > > 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. > > [Jon] We have found some mandatory usage which is no longer required > - this will be corrected. > > > > #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? > > Answered by [Med] [4] > > Are keys mandatory? > > Answered by [Med] [4] > > Are there any sequencing requirements on keys, e.g. do keys need > to be > > sent first (as in NETCONF) in a list > element? > > Answered by [Med] [4] > > 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. > > Answered by [Med] [4] > > > > #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? > > Answered by [Med] [4] > > > > #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? > > Answered by [Med] [4] > > > > #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. > > Question raised by [Med] [4] > [Jon] For the DOTS data channel, the YANG is augment, and so config > true|false still stand for this. > > > > #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. > > Answered by [Med] [4] > > > > #9) Payload encoding references > > > > As far as I understand, the message payloads are meant to be > encoded > > in > JSON > > or > > CBOR. > > Answered by [Med] [4] > [Jon} CBOR is only used for the signal channel. However, JSON is > used for the payload over the data channel when updating the vendor > mappings. > > > 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). > > Answered by [Med] [4] > > > > 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). > > Answered by [Med] [1] > > > > #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. > > Answered by [Med] [1] > > > > #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?) > > Answered by [Med] [1] > > > > #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. > > Answered by [Med] [1] > > > > #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. > > Answered by [Med] [1] > > > > #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. > > Answered by [Med] [1] > > > > #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. > > Answered by [Med] [1] > > > > #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? > > In part answered by [Med] [1] > [Jon] If the server does not indicate that it supports any query- > type, then the client will be unable to use any of the potential > query-type to reduce the returned data content from the server. > > > #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. > > Answered by [Med] [1] > > > > #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. > > Answered by [Med] [1] > > > > #20) Section 7.1.1 mentions a "optional subattribute" Please > define > > this > term, > > or change to a defined term. > > Answered by [Med] [1] > > > > #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? > > Answered by [Med] [1] > > > > #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? > > Answered by [Med] [1] > > > > #23) Similarly for grouping percentile-config and grouping > percentile. > Names > > that better reflect their content and usage would be good. > > Answered by [Med] [1] > > > > #24) There are still a few instances of augment in the model. They > > should > be > > sx:augment-stucture. > > Answered by [Med] [1] > > > > #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. > > > > Answered by [Med] [1] > > [1] https://mailarchive.ietf.org/arch/msg/dots/VbVmlhvao- > kk_Z2GzTkV2QN7Qbw/ > [2] https://mailarchive.ietf.org/arch/msg/dots/JWg4NYsKXGzj_mGu- > TUsMf9E4kM/ > [3] https://mailarchive.ietf.org/arch/msg/dots/L5vEwPU5B9d- > srruoV26Wq6mQco/ > [4] > https://mailarchive.ietf.org/arch/msg/dots/qwO96u377lEdzx6G_HL_QppxE > Go/ > [5] > https://mailarchive.ietf.org/arch/msg/dots/1A8z9MiqDOQzPamYdHWqXvwQg > rk/ > > _________________________________________________________________________________________________________________________ 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. -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
