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

Martin Bjorklund <mbj@tail-f.com> Tue, 05 February 2019 08:03 UTC

Return-Path: <mbj@tail-f.com>
X-Original-To: netconf@ietfa.amsl.com
Delivered-To: netconf@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 36C78130F4C; Tue, 5 Feb 2019 00:03:21 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.9
X-Spam-Level:
X-Spam-Status: No, score=-1.9 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id xBzqfilMvXlZ; Tue, 5 Feb 2019 00:03:17 -0800 (PST)
Received: from mail.tail-f.com (mail.tail-f.com [46.21.102.45]) by ietfa.amsl.com (Postfix) with ESMTP id 5AE26130E0E; Tue, 5 Feb 2019 00:03:17 -0800 (PST)
Received: from localhost (unknown [173.38.220.61]) by mail.tail-f.com (Postfix) with ESMTPSA id 0790D1AE0481; Tue, 5 Feb 2019 09:03:13 +0100 (CET)
Date: Tue, 05 Feb 2019 09:03:12 +0100
Message-Id: <20190205.090312.1916826348030549950.mbj@tail-f.com>
To: ludwig@clemm.org
Cc: yang-doctors@ietf.org, draft-ietf-netconf-yang-push.all@ietf.org, netconf@ietf.org
From: Martin Bjorklund <mbj@tail-f.com>
In-Reply-To: <004601d4bd1d$952072e0$bf6158a0$@clemm.org>
References: <20190130.165118.1684515639662649114.mbj@tail-f.com> <004601d4bd1d$952072e0$bf6158a0$@clemm.org>
X-Mailer: Mew version 6.7 on Emacs 24.5 / Mule 6.0 (HANACHIRUSATO)
Mime-Version: 1.0
Content-Type: Text/Plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://mailarchive.ietf.org/arch/msg/netconf/Gn3exZf2kKDdZfn1Z96vqapKZQM>
Subject: Re: [netconf] Yangdoctors last call review of draft-ietf-netconf-yang-push
X-BeenThere: netconf@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: NETCONF WG list <netconf.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/netconf>, <mailto:netconf-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/netconf/>
List-Post: <mailto:netconf@ietf.org>
List-Help: <mailto:netconf-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/netconf>, <mailto:netconf-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 05 Feb 2019 08:03:21 -0000

Hi,


"Alexander Clemm" <ludwig@clemm.org> 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 <netconf-bounces@ietf.org> On Behalf Of Martin Bjorklund
> Sent: Wednesday, January 30, 2019 7:51 AM
> To: yang-doctors@ietf.org; draft-ietf-netconf-yang-push.all@ietf.org
> Cc: netconf@ietf.org
> 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
>