Subject: Re: [netconf] Shepherd review of draft-ietf-netconf-notification-capabilities

From: Kent Watsen <[email-masked]>
Date: Sat, 6 Mar 2021 02:27:43 +0000

Balazs and Benoit,

Thanks for the update and the response.  Please see below for followups.

K.  // shepherd

> On Feb 22, 2021, at 4:20 PM, Benoit <[email-masked]> wrote:
> 
> Hi Kent,
> 
> Sorry it took us so long to update the draft.
> Below are our answer. We believe we have addressed all comments.
> 
> Title:
> - it would be best for the title to not use the “YANG-Push” term before it’s been imported.
> How about this?
> OLD: YANG Modules for describing System Capabilities and Yang-Push Notification Capabilities
> NEW: YANG Modules describing Capabilities for Systems and Notifications for Datastore Updates
> NEW2: YANG Modules describing Capabilities for Systems and Datastore Updates Notifications
> BALAZS: OK, used New2
> 
> Be sure to update any change in the title in the YANG modules as well!
> BALAZS: OK, used New2
> 
> Note: https://en.wikipedia.org/wiki/YANG#Standards-track_data_models <https://en.wikipedia.org/wiki/YANG#Standards-track_data_models> has
> a preference for "Data Models” over “Modules”, but I left that part alone
> since it’s not unheard of either...

You’re right, “Data Models” would be better.  But, like you say, both are seen in the wild, so do as you see fit.

PS: Thanks for fixing the plurality issue (Updates --> Update)

> Abbreviated title:
> - "YANG-Push Notification Capabilities” is not an accurate description...
> BALAZS: OK, changed to System and Notification Capabilities

Good.

> Abstract:
> 
> - module names should be in quotes when introduced
> BALAZS: OK

Thanks.  (Albeit, I didn’t intend a global replace.  No biggee, the copy editor will tweak as needed).

> - s/This document defines two YANG modules:/This document defines two YANG modules,"ietf-system-capabilities” and "ietf-notification-capabilities”.
> BALAZS: OK

Thanks.

> - s/YANG Instance Data/YANG Instance Data (I-D.ietf-netmod-yang-instance-file-format)/
> BALAZS: OK

Hmmm, now it’s renamed and moved to the end of the paragraph, but still without a reference.  Admittedly, I think references in Abstracts are discouraged but, as it stands, readers may be confused.  Note that searching for "YANG Instance Data File Format” doesn’t resolve any other matching because only other matching string is in the “Normative References” Section, where it line wraps.  If you don’t put a reference into the abstract, then import the term in the “Terminology” Section.

> - s/"Subscription to YANG Datastores" (YANG-Push)/Subscription
> to YANG Notifications for Datastore Updates (RFC 8641)/
> BALAZS: OK

Thanks, but not that the sentence ends 'augmenting the "ietf-system-capabilities”.’   This should either be 'augmenting "ietf-system-capabilities”.’ or 'augmenting the "ietf-system-capabilities” module.'

> - Actually (in ref to the above two comments), the sentence structure in p2 seems
> backwards. Better would be to say "The module can be used to report capability
> information from the server at run-time or implementation-time, per I-D.ietf-netmod-yang-instance-file-format.”
> BENOIT: idnits complains when we have references in the abstract. Removed the references 

Ah, yes, right, then import the term?

> Section 1:
> 
> - Why isn’t the “Introduction” Section ‘1’ as is common in RFCs?
> BALAZS: OK, changed order of sections

Thanks.

> - Each term should be in double-quotes when first introduced, or use hanging text like other RFCs do.
> BALAZS: OK

Good.

> - don’t capitalize terms unless they appear at the beginning of a sentence, unless they’re capitalized throughout the entire document consistently! Which they’re not here.  Personally, I can never capitalize them consistently, so I just follow normal sentence-capitalization rules…
> BALAZS: OK

Thanks (though I didn’t check the entire document for consistency)

> - Neither "Run-time information” nor "Implementation-time information”
> constitute terms. Seriously, no one is going to check this document for
> those terms. Also note that "Run-time information” is only used once, as
> "Implementation-time information” would be as well if the text in the two
> back-to-back paragraphs were rearranged slightly.
> BENOIT: in the context of this draft, we have 2 separate scenarios (run-time information and implementation-time information). Instead of re-explaining them in different sections and risking that the terms are misunderstood, we believe it's best to define them. We seem to recall that we also received that feedback from the WG (during LC, possibly)

Fine.

> - "On-change notification capability” is not a term and it’s not used anywhere in the document
> BALAZS: OK, removed

Thanks.

> Section 2:
> 
> - The first sentence uses the word “capabilities” without explanation (and it’s not a term). Given that this document defines the “capabilities”, I’m expecting a term to be defined for it. S3 provides examples, but it seems too far away…thoughts?
> 
> Maybe S1 and S2 could be collapsed:
> 
> OLD:
> Systems implementing a server and/or a publisher often have
> capabilities that are not defined by the YANG model itself. There is
> a need to publish this capability information as it is part of the
> contract between the server and client.
> 
> NEW:
> Servers and/or a publishers often have capabilities, values describing
> operational behavior, that need to be conveyed to clients, which is enabled by the YANG modules described in this document. <blank line>
> 
> BENOIT: DONE

Thanks.

> - the bullet list should be indented (i.e., put inside another list with symbols="empty”)
> BENOIT: please advice, the following doesn't work
>         <list style="empty">      
>             <list style="symbols">
>                   <t>vendor independent</t>
>                   <t>machine readable</t>
>                   <t>available in identical format both at implementation-time and run-time</t>
>             </list>
>         </list>

Nevermind.  Xml2rfc v2 / v3 behavior changed. 

> Section 2.1:
> - remove all instances of “we” throughout the document. s/we/this document/?
> BENOIT: done

Thanks.

> - why is there a “2.1” if there isn’t at least a “2.2”? (Is this meant to actually be a section 3?)
> BENOIT: corrected

Thanks.

> - the bullet lists should be indented (i.e., put inside another list with symbols="empty”)
> BENOIT: same remark;

;)

> - P4 (the paragraph beginning "Publishers have limitations”) doesn’t seem to say anything concrete (i.e., relavent to the draft). Actually, the same can be said for P5.
> BENOIT: OLD
>    Publishers have limitations in how many update notifications and how
>    many datastore node updates they can send out in a certain time-
>    period.
> 
>    Publishers might not support periodic subscriptions to all
>    datastores.
> 
> NEW: Publishers might have some capabilities (or limitations) to document.  
>    For example, how many update notifications and how many datastore node updates 
>    they can send out in a certain time-period. Other publishers might
>    not support periodic subscriptions to all datastores.

Okay.

> - P6 and P7 appear to be (unnecessary) elaborations of bullet-point #2 above.
> BENOIT: combined the paragraphs to make a longer series of capabilities/limitations to document. The entire section is:
>           Publishers might have some capabilities (or limitations) to document. 
>           For example, how many update notifications and how many datastore node 
>           updates they can send out in a certain time-period. Other publishers might 
>           not support periodic subscriptions to all datastores. In some cases, a publisher supporting 
>           on-change notifications will not
>           be able to push updates for some object types on-change. Reasons for
>           this might be that the value of the datastore node changes frequently
>           (e.g., in-octets counter), that small object changes are
>           frequent and irrelevant to the receiver (e.g., a temperature gauge changing 0.1
>           degrees within a predetermined and acceptable range), or that the 
>           notification for a particular object. In all those cases, 
>           it will be important for subscriber applications to have
>           a way to identify which objects on-change notifications are
>           supported and for which ones not.

Better.

> - s/Clients of a server, subscribers to a publisher/Clients of a server/ (note: that subscribers are clients)
> BENOIT: NEW
> Clients of a server (and subscribers to a publisher, as subscribers are also clients) need a method to gather capability information.
Much better!

> - the last 2-3 paragraphs seem to go into the weeds wrt the focus of this draft…and the whole
> Implementation-time vs Runtime tradeoff is repeated in Section 3. Honestly, I don’t think
> this draft needs to discuss this at all but, if you must, factor it out into a separate section
> and keep it short, e.g., as in Section 3.
> BENOIT: Actually, we believe that we must address the implementation-time vs runtime topics, as the two provides the background for two distinct use cases, both solved with the solution in this draft.
> However, we agreed with you that the section order was not right. We introduced the more generic text in the introduction and kept the specifiations for subsequent sections.

Fine, and Better.

> Section 3:
> - why doesn’t this section come before Section 2.1 (which maybe should actually be a Section 3)?
> 
> BENOIT: corrected

Thanks,

> - System-level capabilities is the “base” model, and hence should be introduced first…
> BENOIT: corrected

Thanks (note: my comment was in reference to my previous comment)

> - actually, after reading the entirety of Section 3, I don't understand the purpose of Section 2.1
> BENOIT: yes, there is some history behind the draft. First it was about the YANG push capabilities only. Then, as we foresaw that other type of capabilities would be needed, we added the generic system capabilities information. Reviewing the draft, the history still transpired through the section order. Hopefully, this is solved now.

Thanks for the background.

> - recommend 1) extracting the middle part out into a new “providing data” section where the
> Implementation-time vs Runtime options are discussed and 2) moving the remainder to End of the Introduction section…
> 
> - s/The module ietf-system-capabilities/The module "ietf-system-capabilities"/
> BALAZS: OK .

Thanks.

> - s/is defined to provide a structure/provides a structure/
> - s/any YANG related system capability/YANG-defined capabilities/
> BENOIT: It might be best to keep it aligned with the abstract.
> The last two points become:
> The module "ietf-system-capabilities" provides a placeholder structure to be used to specify any YANG related system capability.
Good catch.

> - s/The module ietf-notification-capabilities/The module "ietf-notification-capabilities”/
> BALAZS: OK

Thanks.

> - s/"Subscription to YANG Datastores" (YANG-Push)/Subscription to YANG Notifications for Datastore Updates [RFC8641]/
> BENOIT: done

Thanks,

> - Re: "on system/publisher level” - why is “publisher” mentioned in Section 4 at all?
> BENOIT: removed

Thanks,

> Section 4:
> - s/The module ietf-system-capabilities/The module "ietf-system-capabilities"/
> BENOIT: DONE

Thanks,

> - s/is defined to provide a structure/provides a structure/
> BENOIT: DONE

Thanks,

> - s/that can be used to specify/that can be used to discover (as read-only operational state)/
> BENOIT: DONE

Good, but searching for "hat can be used to specify” resulted in finding two other instances that I believe would benefit from the same update.

> - s/any YANG related system capability/YANG-defined capabilities/
> BENOIT: see above

Okay.

> - P3 should be P2 (like they are in the YANG module)
> BENOIT: right

Thanks.

> - the presence of P2 is confusing, if “this module itself does not contain any capabilities”
> BENOIT: See below.
> 
> - I think you want to say something like the “structure” provides augmentation points
> for capabilities to be defined at different levels: a) system, when foo, b) datastore,
> when bar, and c) specific data nodes, the baz. (where foo/bar/baz describe the
> augmentation points used) - right?
> BENOIT: right. 

Better.

>     This module itself does not contain any capabilities; it provides augmentation points 
>     for capabilities to be defined in subsequent YANG modules. 
> - s/config=false/“config false”/
> BENOIT: DONE

Thanks.

> Section 4.2: ([email-masked] <mailto:[email-masked]>)
> 
> - should the “description” statement for "import ietf-yang-library”
> 1) s/a revision/a backwards-compatable revision/ and 2) state that
> the module must be implemented? (Better, see my other comment
> regarding the YANG Doctor;s comment)
> BENOIT: Done

Thanks.

> - s/RFC RFC8525/RFC 8525/
> BENOIT: Done

Thanks,

> - OLD:
> This module specifies a module intended to contain system
> capabilities. System capabilities may include capabilities of a
> NETCONF or RESTCONF server or a notification publisher.
> NEW:
> This module specifies a structure to specify system
> capabilities for s server or a publisher.
> BENOIT: OK

Thanks,

> - s/capabilities it only/capabilities, it only/
> BENOIT: OK

Thanks.

> - s/should be augmented in/are augmented in/
> BENOIT: OK

Thanks.

> - be sure there is a blank line between paragraphs and numbered
> list items, and/or list items are indented (this comment applies to
> *all* “description” statements)
> BENOIT: OK

Thanks (though I didn’t check every instance)

> - list item (4) is missing punctuation and the first “sentence” doesn’t
> form a sentence.
> BENOIT: OK

Thanks.

> - "RFC XXX: YANG-Push Notification Capabilities” has both an incorrect number of X’s as well as incorrect document title.
> BENOIT: OK

Thanks.

> - rather than have the “description” statement "// augmentation
> point for system level capabilities”, why not have an empty
> “choice” statement with the same description statement?
> BALAZS: choice would not be a good solution because it would only allow one set of capabilities, while our goal is to allow different capability set side by side.

You misunderstand.  Adding an empty choice preserves the data model as is.  It’s just a more "YANG way” to do what you have:

OLD:
    // augmentation point for system level capabilities

NEW:
   choice system-capabilities {
      // mandatory true;  // must at least one exist?
      description 
         "augmentation point for system level capabilities.”;
   }

> - re: "Only individual datastores”, is “individual” the right word? Would “specific” or “leaf” be better?
> BALAZS: we used "specific"

Thanks.

> - rather than have the “description” statement "// augmentation
> augmentation point for datastore or data node level capabilities”,
> why not have an empty “choice” statement with the same
> description statement?
> BALAZS: see above.

Also see above  ;)

> - is the comment "The special value '/' denotes all data nodes in the
> datastore.” should state if it’s a CLR or simply consistent with XPath 1.0 expressions.
> BALAZS: we are consistent witjh the NACM RFC, which says:
>                     The special value '/' refers to all possible
>                     datastore contents.";

Then the comment should say “As specified in NACM Section ???, the special value…”, right?

> - this command detected an issue: pyang --ietf ietf-system-capabilities\@2020-03-23.yang.
> (I’m unsure why, exactly, as I see the keywords block in the module…suggest copying
> text directly from `pyang --ietf-help`.

No response?

> Section 5
> - s/The YANG module ietf-notification-capabilities is defined to provide/The YANG module "ietf-notification-capabilities” provides/
> BENOIT: done

Thanks.

> Section 5.2: ([email-masked] <mailto:[email-masked]>)
> - P1 doesn’t specify all the normative imports
> BENOIT: done

Thanks.

> - be sure there is a blank line between paragraphs and numbered
> list items, and/or list items are indented (this comment applies to
> *all* “description” statements)
> BENOIT: done

Thanks.

> - s/If, different/If different/
> BENOIT: done

Thanks.

> - "RFC XXX: YANG-Push Notification Capabilities” has both an incorrect number of X’s as well as incorrect document title.
> BENOIT: done

Thanks.

> - s/config=true/‘config true’/
> BENOIT: done

> - s/config=false/‘config false’/
> BENOIT: done

Thanks.

> - "supported for none” - nonsensical, rephrase.
> BENOIT:
>               "Type for defining whether 'on-change' or
>            'periodic' notifications are supported 'config false' 
>            data nodes, 'config true' date nodes, no data nodes, 
>            or all data nodes.

Better!

> - s/ or all data nodes/, or all data nodes/ (also try to fix the sentence so it makes sense)
> - s/ or a set of/, or a set of/
> - s/update-too-big/'update-too-big’/
> BENOIT: done

Thanks.

> - many of the “reference” statements are being misused. Move content
> information into the “description” and only set the reference perhttps://tools.ietf.org/html/rfc7950#section-7.21.4 <https://tools.ietf.org/html/rfc7950#section-7.21.4>
> BENOIT: all corrected

Thanks.

> - s/and the subtree below them/including any subtrees that may exist below them/
> BENOIT: done

> - s/on-change notifications/'on-change' notifications/
> BENOIT: done

Thanks.

> - this command detected a number of issues: pyang -f yang --keep-comments --yang-line-length 69 ietf-notification-capabilities\@2020-03-23.yang > tmp; diff ietf-notification-capabilities\@2020-03-23.yang tmp. (whitespace at end of lines)

No response?

> Section 7.2 (really 5.2, but I didn’t catch it until here)
> 
> - the “inc” prefix seems odd, perhaps wrong, how about “notc” (to match “sysc”)?:
> BENOIT: You're right, changed

Thanks.

> Appendix A
> 
> - the example is wrapped in the <CODE BEGINS>/<CODE ENDS>, but now that `rfcstrip` has the “-a” parameter, these are no longer needed - agreed? Please move the filename augment to the “name” attribute in the <artwork> element for both examples.
> BALAZS: OK

Thanks.

> - ideally there are subsections (i.e., A.1 and A.2) delineating each example
> BENOIT: done

Thanks.

> - s/config=false/“config false”/
> BENOIT: done

Thanks.

> - please reorder the examples so that the system-capability examples precedes the notification-capability example 
> BENOIT: done

Thanks.

> - in both examples:
> - s/following instance-data/following instance data/ (or "instance data set”?)
> BENOIT: done

Thanks.

> ^-- also add a reference to I-D.ietf-netmod-yang-instance-file-format
> - please remove the "<!-- revision date, contact, etc. —>” comment
> BENOIT: done

Thanks.

> - s/config=false/“config false”/
> BALAZS: OK
> - s/config=true/"config true”/
> BALAZS: OK

Thanks.

> - consider putting quotes around words like “on-change” and “periodic”
> BENOIT: Done
> - s/only 2/only two/
> BENOIT: Done

Thanks.

> 
> 
> Other:
> 
> - nowhere in the document is there a request to the RFC Editor to set the “XXXX”, “YYYY”, and “YYY" strings. Perhaps other strings need to be converted as well...
> BENOIT: done

Thanks.

> - through draft (in both body and YANG modules), bracket datastore names with
> angle brackets (e.g., s/running/<running>/) wherever the datastore name is NOT
> followed by word “datastore”. When the datastore name is NOT followed by the
> word “datastore", the datastore name should be in quotes, where it makes sense
> to do so. BTW, I note the none of the datastore names are imported as terms or
> otherwise introduced where first used...
> Benoit: Done

I don’t see these changes...

> - the YANG Doctor review suggested adding comments regarding
> if imports needed to be implemented. I see such comments were
> added in -06 but they’ve subsequently been removed. I strongly
> second Lada’s suggestion to include a minimal YANG Library
> instance somewhere, not only to document which modules need to be implemented, but also to document the totality of them, as I
> was unpleasantly surprised by how many there were (12 in total!)
> BALAZS: OK, Included for the 3 imports that need implemented. They were removed due to comments, but I like the sentences.

Better, but no YANG Library instance?  Oh well.

K.