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

[Kent Watsen <[e-mail]>]

I happened to have a reason to extract and validate the "ietf-system-capabilities” module (from the draft posted yesterday) and got the following errors when using `pyang -Werror --ietf --max-line-length=69`, as I always do, to ensure `xml2rfc` doesn’t emit any warnings:

[e-mail]:44: error: line length 70 exceeds 69 characters
[e-mail]:56: error: line length 70 exceeds 69 characters
[e-mail]:57: error: line length 72 exceeds 69 characters
[e-mail]:151: error: line length 70 exceeds 69 characters
[e-mail]:159: error: line length 71 exceeds 69 characters
[e-mail]:164: error: line length 70 exceeds 69 characters
[e-mail]:165: error: line length 70 exceeds 69 characters

K.



> On Jul 6, 2021, at 4:09 AM, Rob Wilton (rwilton) <[e-mail]> wrote:
> 
> Sorry, my bad for posting to the wrong alias originally.  Benoit, thanks for the correction.
>  
> Regards,
> Rob
>  
>  
> From: Benoit Claise <[e-mail]> 
> Sent: 05 July 2021 21:48
> To: Rob Wilton (rwilton) <[e-mail]>; [e-mail]; [e-mail]
> Cc: [e-mail]
> Subject: Re: [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 <https://www.ietf.org/archive/id/draft-ietf-netconf-notification-capabilities-17.txt>
> Status:         https://datatracker.ietf.org/doc/draft-ietf-netconf-notification-capabilities/ <https://datatracker.ietf.org/doc/draft-ietf-netconf-notification-capabilities/>
> Htmlized:       https://datatracker.ietf.org/doc/html/draft-ietf-netconf-notification-capabilities <https://datatracker.ietf.org/doc/html/draft-ietf-netconf-notification-capabilities>
> Diff:           https://www.ietf.org/rfcdiff?url2=draft-ietf-netconf-notification-capabilities-17 <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> <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
> [e-mail] <mailto:[e-mail]>
> https://www.ietf.org/mailman/listinfo/netmod <https://www.ietf.org/mailman/listinfo/netmod>