Re: [netconf] [netmod] AD review of draft-ietf-netconf-notification-capabilities

Dear all,

I realized that the NETMOD WG was copied on this email thread, instead 
of NETFCONF.
Now corrected, for archiving purposes.

Regards, Benoit

On 7/5/2021 12:53 PM, Benoit Claise wrote:
> Hi Rob,
>
> Thanks for your detailed review.
> A new draft version has been posted.
> URL:https://www.ietf.org/archive/id/draft-ietf-netconf-notification-capabilities-17.txt
> Status:https://datatracker.ietf.org/doc/draft-ietf-netconf-notification-capabilities/
> Htmlized:https://datatracker.ietf.org/doc/html/draft-ietf-netconf-notification-capabilities
> Diff:https://www.ietf.org/rfcdiff?url2=draft-ietf-netconf-notification-capabilities-17
>
> See the justifications below.
>
> On 6/21/2021 10:45 AM, Rob Wilton (rwilton) wrote:
>> Hi,
>>
>> Here is my AD review of draft-ietf-netconf-notification-capabiltiies-16
>>
>> Thanks for this draft, sorry for the delay in reviewing.  It looks like it is in good shape.
>>
>> I think that most of my comments are minor or cosmetic suggestions to potentially improve the phrasing of the text.
>>
>>
>> 1.
>> Abstract:
>>
>>     The module "ietf-system-capabilities" provides a placeholder
>>     structure that can be used to discover YANG related system
>>     capabilities for servers.  The module can be used to report
>>     capability information from the server at run-time or implementation-
>>     time, per the YANG Instance Data File Format.
>>
>> Suggest "by making use of" rather than "per".
> DONE.
>> 2.
>>     1.  Introduction
>>
>>     There is a need to publish this capability information as it is part
>>     of the contract between the server and client.
>>
>> Suggest "contract" -> "API contract".
> DONE
>> 3.
>>     There is a need to publish this capability information as it is part
>>     of the contract between the server and client.  Examples include
>>     maximum size of data that can be stored or transferred, information
>>     about counters (whether a node supports "on-change" telemetry), etc.
>>     Such capabilities are often dependent on a vendor's implementation or
>>     the available resources at deployment.  Many such capabilities are
>>     specific to either the complete system, individual YANG datastores
>>     [RFC8342] or specific parts of the YANG schema, or even individual
>>     data nodes.  It is a goal of this document to provide a common way of
>>     representing such capabilities in a format that is:
>>
>> Suggest: maximum -> the maximum
>>           "or specific" -> ", specific"
> DONE
>> 4.
>>     o  available in identical format both at implementation-time and run-
>>        time
>> 	
>> Suggest: "in an identical", and a period at the end.
> DONE
>> 5.
>>     If the information is
>>     not documented in a way available to the NMS designer, but only as
>>     instance data from the network node once it is deployed, the NMS
>>     implementation will be delayed
>>
>> Suggest: "way available" => "way that is readily available"
> DONE
>> 6.
>>     The network operator needs to plan his
>>     management practices and NMS implementation before he even decides to
>>     buy the specific network node type.
>>
>> Suggest: "him" -> "their", "he even decides" -> "they decide"
> DONE
>> 7.
>>     Run-time information is needed:
>>     
>> Suggest: Run-time capability information is needed:
> DONE.
>> 8.
>>     o  to check that capability information provided earlier, at
>>        implementation-time is what the publisher has implemented.
>>
>> Suggest: "at implementation-time, is"
> DONE
>> 9.
>>       To find a capability value for a specific data node in a
>>       specific datastore the user SHALL:
>> 	
>> Please clarify that the capability value is selected by the relative path
>> to the datanode defining the capability.  i.e., the same name/path must be
>> used both under the system level and per datastore level capabilties.
> NEW SENTENCE.
> "When stating a specific capability, the relative path for any specific
> capability must be the same under the system-capabilities container and
> under the per-node-capabilities list: the same grouping for defining the
> capabilities MUST be used. "
>> 10.
>> 2) If the datastore entry is found within that entry, process all
>>       per-node-capabilities entries in the order they appear in the list.
>>       The first entry that specifies the specific capability and has a
>>       node-selector selecting the specific data node defines the
>>       capability value.
>>
>> I'm not sure this is required, but perhaps consider adding text to make it clear
>> that longest path matching can be achieved by ordering more specific
>> matches before less specific matches.
> ADDED "Note that longest path matching can be achieved by ordering 
> more specific matches before less specific ones"
> under
>      list per-node-capabilities {
>          description
>            "Each list entry specifies capabilities for the selected
>             data nodes. The same capabilities apply for the data nodes
>             in the subtree below the selected nodes.
>
>             The system SHALL order the entries according to their
>             precedence. The order of the entries MUST NOT change unless
>             the underlying capabilities also change.";
> We did NOT add next to "2) If the datastore entry is found within that 
> entry ..." because that section focuses on the user (as opposed to the 
> implementer on the server), as mentioned in "To find a capability 
> value for a specific data node in a
>       specific datastore the user SHALL:"
>> 11.
>>      // augmentation point for system level capabilities
>> Suggest: "Augmentation ... capabilities."  I would also suggest using a block style
>> comment so this doesn't get lost.	
> DONE.
>> 12.
>>             Only one specific datastore can be specified
>>             e.g., ds:conventional is not allowed.";
>> 		
>> Suggest changing to:
>>
>>             Only specific datastores can be specified.
>> 		   E.g., ds:conventional, which represents a
>> 		   set of configuration datastores, must not be
>> 		   used";
> DONE
>> 13.
>>            description
>>              "A method to select all or some nodes within a datastore.";
>>
>> "some or all" would flow better.
> DONE
>> 14.
>>          // augmentation point for datastore or data node level
>>          // capabilities
>>
>> Suggest: "Augmentation ... capabilities."  I would also suggest using a block style
>> comment so this doesn't get lost.
> DONE.
>> 15.
>> 5.2.  YANG Module (ietf-notification-capabilities)
>>
>>        - capabilities related to the throughput of notification data
>>        the publisher can support. (Note that for a specific
>>        subscription the publisher MAY still allow only longer periods
>>        or smaller updates depending on e.g., actual load conditions.)
>> 	
>> Suggest: "data that the publisher"
>>           "specific subscription, the"
>> 		 "still allow" -> "allow"
>>           "e.g., -> ", e.g., "		
> DONE.
>>   
>>
>>
>> 16.
>>             bit config-changes {
>>               description
>>                 "The publisher is capable of sending
>>                  notifications for 'config false' nodes for the
>>                  relevant scope and subscription type.";
>> 				
>> I presume that this should this be 'config true' nodes?
> GOOD CATCH+
>
>> 17.
>>           description
>>             "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.
>> 			
>> Suggest: "supported for 'config false'",
>>           "date" -> "data
>> As an optional minor nit, it might be worth putting 'all' at the beginning
>> of the list rather than the end.
> DONE.
>> 18.
>>               "Indicates the minimal update period that is
>>                supported for a 'periodic' subscription.
>>
>>                A subscription request to the selected data
>>                nodes with a smaller period than what this leaf
>>                specifies will result in a 'period-unsupported' error.";
>>
>> Is "will result" right here, or should it be "MAY result" or "is likely to result"?
>> I.e., is the server guaranteeing that it won't handle a smaller update for the
>> given capability under any circumstance?  The same question also applies
>> to the "supported-update-period".
> DONE. "is likely to result"
>> 19.
>>             "The change types that can be excluded in
>>              YANG-Push subscriptions.";
>> 			
>> Suggest adding something like "for the selected data nodes."
> DONE
>> 20.
>> 7.2.  The YANG Module Names Registry
>>
>>     This document registers two YANG modules in the YANG Module Names
>>     registry.  Following the format in [RFC7950], the the following
>>     registrations are requested:
>>     
>> This should be (along with a normative reference to RFC 6020):
>>
>>     This document registers two YANG modules in the YANG Module Names
>>     registry [RFC6020].  Following the format in [RFC6020], the the following
>>     registrations are requested:
> DONE.
>> 21.
>> 9.2.  Informative References
>>
>>     [RFC3688]  Mealling, M., "The IETF XML Registry", BCP 81, RFC 3688,
>>                DOI 10.17487/RFC3688, January 2004,
>>                <https://www.rfc-editor.org/info/rfc3688>.
>> 			
>> This needs to be a normative reference (for the IANA registry) definition.
> DONE
>> 22.
>> Appendix A.  Instance data example #1
>>
>> Suggest changing:
>> "the running, and operational datastores" => "the running and operational datastores".
>> '"on-change" only' -> '"on-change", only'
>> 'reported "on-change" as they' -> 'reported "on-change", as they'
> DONE
>> 23.
>> ========== NOTE: '\' line wrapping per BCP YYY (RFC YYYY) ===========
>>
>> This can be updated to "NOTE: '\' line wrapping per RFC 8792".
> DONE
>> 24.
>> "only has running, and operational" -> "only has running and operational"
> DONE.
>> 25.
>>      false" data from the operational datastore. Statistics are
>>      not reported on-change only two important counters, for these
>>      a smaller dampening period is possible.
>> 	
>> Suggest:
>> 	false" data from the operational datastore. Statistics are
>>      not reported on-change except for two important counters, where
>>      a small dampening period is mandated.
> DONE.
>> 	
>>
>> Spelling/grammar warnings (generated by tool):
>>
>> Potentially incorrect spellings: sheperds, getconfig
>>
>> Grammar Warnings:
>> Section: 1, draft text:
>>   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.
>> Warning:  The plural noun "publishers" cannot be used with the article "a". Did you mean a publisher or publishers?
>> Suggested change:  "a publisher"
> DONE
>> Section: 1, draft text:
>> It is a goal of this document to provide a common way of representing such capabilities in a format that is:
>> - vendor independent
>> - machine readable
>> - available in identical format both at implementation-time and run-time
>>
>> Warning:  This word is normally spelled with hyphen.
>> Suggested change:  "machine-readable"
> DONE
>> Section: 1, draft text:
>> It is a goal of this document to provide a common way of representing such capabilities in a format that is:
>> - vendor independent
>> - machine readable
>> - available in identical format both at implementation-time and run-time
>>
>> Warning:  Please add a punctuation mark at the end of paragraph.
>> Suggested change:  "run-time."
> DONE
>> Section: 1, draft text:
>> Moreover the decision to buy the node type sometimes depends on these management possibilities.
>> Warning:  Did you forget a comma after a conjunctive/linking adverb?
>> Suggested change:  "Moreover,"
> DONE.
>> Section: 1.1, draft text:
>>   "Implementation-time information": Information about the server's behavior that is made available during the implementation of the server, available from a source other then a running server.
>> Warning:  Did you mean other than?
>> Suggested change:  "other than"
> DONE.
>> Section: 3, draft text:
>> These include:
>> - Supported (reporting) periods for "periodic" subscriptions
>> - Maximum number of objects that can be sent in an update
>> - The set of datastores or data nodes for which "periodic" notification is supported
>>
>> Warning:  Please add a punctuation mark at the end of paragraph.
>> Suggested change:  "supported."
> DONE
>> Section: 7.2, draft text:
>> Following the format in [RFC7950], the the following registrations are requested:
>> Warning:  Maybe you need to remove one determiner so that only the or the is left.
>> Suggested change:  "the"
> DONE
>> Section: Appendix C, draft text:
>> In this latter case it is really the server functionality that is discussed
>>
>> Warning:  Please add a punctuation mark at the end of paragraph.
>> Suggested change:  "discussed."
> DONE
>> Thanks,
>> Rob
>> .
>
>
> _______________________________________________
> netmod mailing list
> netmod@ietf.org
> https://www.ietf.org/mailman/listinfo/netmod