Re: [netconf] Yangdoctors last call review of draft-ietf-netconf-yang-push

[Alexander Clemm <[e-mail]>]

Thanks, Martin!
--- Alex

> -----Original Message-----
> From: netconf [mailto:[e-mail]] On Behalf Of Martin
> Bjorklund
> Sent: Tuesday, February 05, 2019 12:03 AM
> To: [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [netconf] Yangdoctors last call review of draft-ietf-netconf-yang-
> push
> 
> Hi,
> 
> 
> "Alexander Clemm" <[e-mail]> wrote:
> > Hi Martin,
> >
> > Thank you for your review!  We have just posted an updated revision
> > (-22) per your comments.  (Please see replies inline, <ALEX>)
> 
> Thanks!  I have reviewed the changes, and the new version addresses all my
> comments.
> 
> FYI, I found a few nits that you can fix if you post a new version, or during
> AUTH48:
> 
> 
> OLD:
> 
>            Figure 9: Model structure (non-augmentation portions
> 
> NEW:
> 
>            Figure 9: Model structure (non-augmentation portions)
> 
> 
> And some formatting issues in the new text added to the YANG module.
> You can reach out to me directly if you want these, or just run pyang yourself.
> 
> 
> 
> /martin
> 
> 
> 
> 
> >
> > --- Alex
> >
> > -----Original Message-----
> > From: netconf <[e-mail]> On Behalf Of Martin Bjorklund
> > Sent: Wednesday, January 30, 2019 7:51 AM
> > To: [e-mail]; [e-mail]
> > Cc: [e-mail]
> > Subject: [netconf] Yangdoctors last call review of
> > draft-ietf-netconf-yang-push
> >
> > Hi,
> >
> > This is my YANG doctor's review of draft-ietf-netconf-yang-push-21.
> >
> > Reviewer: Martin Björklund
> > Review result: Ready with Issues
> >
> >
> > o  General
> >
> >   Check the output of idnits.  Specifically, add the 2119
> >   boilerplate.
> >
> >   Also, since the YANG module uses 2119 language, add the YANG-ified
> >   version of the boilerplate to the description statement in the
> >   module, just before the copyright.
> >
> >   <ALEX> Done </ALEX>
> >
> >
> > o  General
> >
> >   The module is inconsistently indented and formatted.  The RFC editor
> >   will enforce consistent formatting, so in order to make their life
> >   easier it is better to do this work before it reaches them.
> >
> >   Attached is a cleaned up version of the YANG module where I have
> >   done the following changes:
> >
> >     o  pyang -f yang --keep-comments --yang-line-length 69
> >
> >        (requires pyang 1.7.8)
> >
> >     o  added newlines in long descriptions, to make separate
> >        paragraphs
> >
> >     o  added extra space after "." in description statement (most of
> >        them already had two spaces)
> >
> >     o  formatted some paragraphs (I use fill-paragraph in emacs,
> > manually)
> >
> >     o  added the 2119 boilerplate
> >
> >   NOTE: I haven't done any other of the changes that I propose in
> >   this review.
> >
> >   <ALEX> Thank you </ALEX>
> >
> > o  Figure 6
> >
> >   The tree diagram for push-change-update doesn't match the module.
> >
> >   OLD:
> >
> >      notifications:
> >        +---n push-update
> >        |  +--ro id?      sn:subscription-id
> >        |  +--ro datastore-contents?   <anydata>
> >        |  +--ro incomplete-update?     empty
> >        +---n push-change-update {on-change}?
> >           +--ro id?     sn:subscription-id
> >           +--ro datastore-changes?
> >           |  +--ro yang-patch
> >           |     +--ro patch-id        string
> >           |     +--ro ypatch:comment?    string
> >           |     +--ro ypatch:edit* [edit-id]
> >           |        +--ro ypatch:edit-id      string
> >           |        +--ro ypatch:operation    enumeration
> >           |        +--ro ypatch:target       target-resource-offset
> >           |        +--ro ypatch:point?       target-resource-offset
> >           |        +--ro ypatch:where?       enumeration
> >           |        +--ro ypatch:value?
> >           +--ro incomplete-update?    empty
> >
> >   NEW:
> >
> >      notifications:
> >        +---n push-update
> >        |  +--ro id?                   sn:subscription-id
> >        |  +--ro datastore-contents?   <anydata>
> >        |  +--ro incomplete-update?    empty
> >        +---n push-change-update {on-change}?
> >           +--ro id?                  sn:subscription-id
> >           +--ro datastore-changes
> >           |  +--ro yang-patch
> >           |     +--ro patch-id    string
> >           |     +--ro comment?    string
> >           |     +--ro edit* [edit-id]
> >           |        +--ro edit-id      string
> >           |        +--ro operation    enumeration
> >           |        +--ro target       target-resource-offset
> >           |        +--ro point?       target-resource-offset
> >           |        +--ro where?       enumeration
> >           |        +--ro value?       <anydata>
> >           +--ro incomplete-update?   empty
> >
> > <ALEX> Updated </ALEX>
> >
> > o  Figure 6
> >
> >   The purpose of including tree diagrams is to give a easy-to-read
> >   overview to the reader.  When the diagram is spread out over
> >   several pages, it becomes more difficult to grasp.
> >
> >   In this case, I would suggest that you split the diagram into
> >   smaller pieces, and place them together with the text that describe
> >   them.
> >
> >   For example, in 4.2 you can have the first part of the tree that
> >   deals with subscription config; 4.3.1 the augmented notifs etc.
> >
> >   <ALEX>  I am splitting the tree into smaller pieces as requested,
> > although I am not sure this is really needed.  While I agree with you
> > in principle, the tree output is structured a bit (into rpcs,
> > notifications, etc), and has items that are non-applicable dotted out
> > ("...").  It is simply a big module, I guess...
> >   </ALEX>
> >
> > o  Figure 9
> >
> >   This example is difficult to read.  I suggest:
> >
> >   OLD:
> >
> >    <netconf:rpc message-id="101"
> >      xmlns:netconf="urn:ietf:params:xml:ns:netconf:base:1.0">
> >      <establish-subscription
> >      xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"
> >      xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push">
> >        <yp:datastore
> >        xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores">
> >          ds:operational
> >        </yp:datastore>
> >        <yp:datastore-xpath-filter
> >        xmlns:ex="http://example.com/sample-data/1.0">
> >          /ex:foo
> >        </yp:datastore-xpath-filter>
> >        <yp:on-change>
> >          <yp:dampening-period>100</yp:dampening-period>
> >        </yp:on-change>
> >      </establish-subscription>
> >    </netconf:rpc>
> >
> >   NEW:
> >
> >    <rpc message-id="101"
> >         xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
> >      <establish-subscription
> >          xmlns=
> >            "urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"
> >          xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push">
> >        <yp:datastore
> >            xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores">
> >          ds:operational
> >        </yp:datastore>
> >        <yp:datastore-xpath-filter
> >            xmlns:ex="http://example.com/sample-data/1.0">
> >          /ex:foo
> >        </yp:datastore-xpath-filter>
> >        <yp:on-change>
> >          <yp:dampening-period>100</yp:dampening-period>
> >        </yp:on-change>
> >      </establish-subscription>
> >    </rpc>
> >
> > <ALEX> Done </ALEX>
> > o  Figure 11
> >
> >   Similar in this example:
> >
> >   OLD:
> >
> >    <netconf:rpc message-id="102"
> >       xmlns:netconf="urn:ietf:params:xml:ns:netconf:base:1.0">
> >       <modify-subscription
> >       xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"
> >       xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push">
> >        <id>1011</id>
> >        <yp:datastore
> >        xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores">
> >          ds:operational
> >        </yp:datastore>
> >        <yp:datastore-xpath-filter
> >          xmlns:ex="http://example.com/sample-data/1.0">
> >          /ex:bar
> >        </yp:datastore-xpath-filter>
> >        <yp:periodic>
> >          <yp:period>250</yp:period>
> >        </yp:periodic>
> >       </modify-subscription>
> >    </netconf:rpc>
> >
> >   NEW:
> >
> >    <rpc message-id="102"
> >         xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
> >      <modify-subscription
> >          xmlns=
> >            "urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"
> >          xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push">
> >        <id>1011</id>
> >        <yp:datastore
> >            xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores">
> >          ds:operational
> >        </yp:datastore>
> >        <yp:datastore-xpath-filter
> >            xmlns:ex="http://example.com/sample-data/1.0">
> >          /ex:bar
> >        </yp:datastore-xpath-filter>
> >        <yp:periodic>
> >          <yp:period>250</yp:period>
> >        </yp:periodic>
> >       </modify-subscription>
> >    </rpc>
> >
> > <ALEX> Done </ALEX>
> >
> > o  Section 4.4.4
> >
> >   (The figure in this example doesn't have a number)
> >
> >   OLD:
> >
> >  <netconf:rpc message-id="103"
> >  xmlns:netconf="urn:ietf:params:xml:ns:netconf:base:1.0">
> >    <resync-subscription
> >    xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-push"
> >    xmlns:sn="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications">
> >      <id>1011</id>
> >    </resync-subscription>
> >  </netconf:rpc>
> >
> >   NEW:
> >
> >    <rpc message-id="103"
> >         xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
> >      <resync-subscription
> >          xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-push"
> >        <id>1011</id>
> >      </resync-subscription>
> >     </netconf:rpc>
> >
> > <ALEX> Done (no idea why no number is being generated for the number)
> > </ALEX>
> >
> > o  Section 5.
> >
> >   In order to get the RFC references right,  follow the pattern in
> >   other documents with YANG modules; e.g. RFC 8343.  Specifically,
> >   all references in RFCs must be found in the main text; this means
> >   that you need to add something like:
> >
> >    This YANG module imports typedefs from [RFC6991].
> >
> > <ALEX> Done </ALEX>
> >
> > o  import yang-patch
> >
> >   OLD:
> >
> >     reference
> >       "RFC 8072: YANG Patch";
> >
> >   NEW:
> >
> >     reference
> >       "RFC 8072: YANG Patch Media Type";
> >
> >       <ALEX> Done </ALEX>
> >
> > o  typedef change-type
> >
> >   The "reference" statement contains text that should be in the
> >   "description" statement (this comment has been raised before on the
> >   ML).
> >
> >   I suggest:
> >
> >   OLD:
> >
> >     description
> >       "Specifies different types of datastore changes.";
> >     reference
> >       "RFC 8072 section 2.5, with a delta that it is valid for a
> >        receiver to process an update record which performs a create
> >        operation on a datastore node the receiver believes exists,
> >        or to process a delete on a datastore node the receiver
> >        believes is missing.";
> >
> >    NEW:
> >
> >     description
> >       "Specifies different types of datastore changes.
> >
> >        This type is based on the edit operations defined for YANG
> >        Patch, with the difference that it is valid for a receiver to
> >        process an update record which performs a create operation on
> >        a datastore node the receiver believes exists, or to process a
> >        delete on a datastore node the receiver believes is missing.";
> >     reference
> >       "RFC 8072: YANG Patch Media Type, section 2.5";
> >
> > <ALEX> Done </ALEX>
> >
> >
> > o  wrong type used (timeticks)
> >
> >   This was discussed on the ML, but apparently the change wasn't
> >   introduced yet.
> >
> >   A couple of 'period' nodes use the type yang:timeticks, but it is
> >   not an appropriate type.  RFC 6991 describes it as:
> >
> >          The timeticks type represents a non-negative integer that
> >          represents the time, modulo 2^32 (4294967296 decimal), in
> >          hundredths of a second between two epochs.  When a schema
> >          node is defined that uses this type, the description of
> >          the schema node identifies both of the reference epochs.
> >
> >   As Juergen pointed out, it seems you want a TimeInterval - but this
> >   type is not present in RFC 6991.  So add:
> >
> >     type centiseconds {
> >       type uint32;
> >       description
> >         "A period of time, measured in units of 0.01 seconds.";
> >     }
> >
> >   and use that instead.
> >
> >   <ALEX> Done (added typedef centiseconds and updated the diagrams
> > accordingly)</ALEX>
> >
> > o  container datastore-changes
> >
> >   OLD:
> >
> >     container datastore-changes {
> >       description
> >         "This contains the set of datastore changes of the target
> >          datastore starting at the time of the previous update, per
> >          the terms of the subscription.  The datastore changes are
> >          encoded per RFC 8027 (YANG Patch).";
> >       uses ypatch:yang-patch;
> >
> >   NEW:
> >
> >     container datastore-changes {
> >       description
> >         "This contains the set of datastore changes of the target
> >          datastore starting at the time of the previous update, per
> >          the terms of the subscription.";
> >       uses ypatch:yang-patch;
> >
> >    (The reference was not correct, and it isn't really necessary
> >    anymore, since you use the grouping from yang patch.)
> >
> >    <ALEX> Done </ALEX>
> >
> > o  leaf datastore-xpath-filter
> >
> >   This leaf has the same problem that stream-xpath-filter had; it
> >   doesn't work for RESTCONF.  We need to apply the same solution as
> >   for stream-xpath-filter:
> >
> >   OLD:
> >
> >        leaf datastore-xpath-filter {
> >          if-feature "sn:xpath";
> >          type yang:xpath1.0;
> >          description
> >            "This parameter contains an XPath expression identifying
> >            the portions of the target datastore to retrieve.
> >            If the expression returns a node-set, all nodes in the
> >            node-set are selected by the filter.  Otherwise, if the
> >            expression does not return a node-set, the filter
> >            doesn't select any nodes.
> >            The expression is evaluated in the following XPath
> >            context:
> >             o  The set of namespace declarations are those in scope
> >                on the 'datastore-xpath-filter' leaf element.
> >             o  The set of variable bindings is empty.
> >             o  The function library is the core function library, and
> >                the XPath functions defined in section 10 in RFC 7950.
> >             o  The context node is the root node of the target
> >                datastore.";
> >        }
> >
> >   NEW:
> >
> >        leaf datastore-xpath-filter {
> >          if-feature "sn:xpath";
> >          type yang:xpath1.0;
> >          description
> >            "This parameter contains an XPath expression identifying
> >            the portions of the target datastore to retrieve.
> >
> >            If the expression returns a node-set, all nodes in the
> >            node-set are selected by the filter.  Otherwise, if the
> >            expression does not return a node-set, the filter
> >            doesn't select any nodes.
> >
> >            The expression is evaluated in the following XPath
> >            context:
> >
> >              o   The set of namespace declarations is the set of prefix
> >                  and namespace pairs for all YANG modules implemented
> >                  by the server, where the prefix is the YANG module
> >                  name and the namespace is as defined by the
> >                  'namespace' statement in the YANG module.
> >
> >                  If the leaf is encoded in XML, all namespace
> >                  declarations in scope on the 'stream-xpath-filter'
> >                  leaf element are added to the set of namespace
> >                  declarations.  If a prefix found in the XML is
> >                  already present in the set of namespace declarations,
> >                  the namespace in the XML is used.
> >
> >              o  The set of variable bindings is empty.
> >
> >              o  The function library is the core function library, and
> >                 the XPath functions defined in section 10 in RFC 7950.
> >
> >              o  The context node is the root node of the target
> >                 datastore.";
> >        }
> >
> > <ALEX> Done </ALEX>
> >
> > o  Section A.1
> >
> >   I have trouble parsing this paragraph:
> >
> >    References to specific identities within the either the subscribed-
> >    notifications YANG model or the yang-push YANG model may be returned
> >    as part of the error responses resulting from failed attempts at
> >    datastore subscription.  Following are valid errors per RPC (note:
> >    throughout this section the prefix 'sn' indicates an item imported
> >    from the subscribed-notifications.yang model):
> >
> >   "the either the" needs to be fixed.
> >
> >   Suggest you spell ou the real names of the YANG modules you refer
> >   to (s/subscribed-notifications.yang
> >   model/"ietf-subscribed-notifications" YANG module/  etc)
> >
> > <ALEX> Done.  (s/within the either the/in the/ and spelled out the
> > names of the YANG modules) <ALEX>
> >
> > o  Section A.1
> >
> >   I don't think this document should repeat what's already specified
> >   in the SN draft.  Specifically, I think this section should list
> >   the *additional* errors that this document introduces.  I also note
> >   that the list in this document is not the same as in the SN draft...
> >
> >   So I suggest:
> >
> >   OLD:
> >
> >    establish-subscription         modify-subscription
> >    ----------------------         -------------------
> >     cant-exclude                   sn:filter-unsupported
> >     datastore-not-subscribable     sn:insufficient-resources
> >     sn:dscp-unavailable            sn:no-such-subscription
> >     sn:filter-unsupported          period-unsupported
> >     sn:insufficient-resources      update-too-big
> >     on-change-unsupported          sync-too-big
> >     on-change-sync-unsupported     unchanging-selection
> >     period-unsupported
> >     update-too-big                resync-subscription
> >     sync-too-big                  --------------------
> >     unchanging-selection           no-such-subscription-resync
> >                                    sync-too-big
> >
> >    delete-subscription            kill-subscription
> >    ----------------------         -----------------
> >     sn:no-such-subscription        sn:no-such-subscription
> >
> >   NEW:
> >
> >    establish-subscription         modify-subscription
> >    ----------------------         -------------------
> >     cant-exclude                   period-unsupported
> >     datastore-not-subscribable     update-too-big
> >     on-change-unsupported          sync-too-big
> >     on-change-sync-unsupported     unchanging-selection
> >     period-unsupported
> >     update-too-big                resync-subscription
> >     sync-too-big                  --------------------
> >     unchanging-selection           no-such-subscription-resync
> >                                    sync-too-big
> >
> > <ALEX>Done, and changed the introductory paragraph accordingly.
> > </ALEX>
> >
> >
> > o  Section A.2
> >
> >   Similar to A.1, suggest you remove the sn: errors that are already
> >   covered by SN.
> >
> > <ALEX> Done, with corresponding changes to the introductory paragraph.
> > </ALEX>
> >
> >
> > /martin
> >
> _______________________________________________
> netconf mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/netconf