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

Alexander Clemm <alexander.clemm@huawei.com> Tue, 05 February 2019 18:00 UTC

Return-Path: <alexander.clemm@huawei.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 7E2141311F3; Tue, 5 Feb 2019 10:00:02 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -4.2
X-Spam-Level:
X-Spam-Status: No, score=-4.2 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RCVD_IN_DNSWL_MED=-2.3, 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 H8sAa4WDHjUw; Tue, 5 Feb 2019 09:59:59 -0800 (PST)
Received: from huawei.com (lhrrgout.huawei.com [185.176.76.210]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 13D8313119F; Tue, 5 Feb 2019 09:59:59 -0800 (PST)
Received: from lhreml704-cah.china.huawei.com (unknown [172.18.7.106]) by Forcepoint Email with ESMTP id CA049A4BCBCC288490A1; Tue, 5 Feb 2019 17:59:56 +0000 (GMT)
Received: from SJCEML701-CHM.china.huawei.com (10.208.112.40) by lhreml704-cah.china.huawei.com (10.201.108.45) with Microsoft SMTP Server (TLS) id 14.3.408.0; Tue, 5 Feb 2019 17:59:56 +0000
Received: from SJCEML521-MBX.china.huawei.com ([169.254.1.95]) by SJCEML701-CHM.china.huawei.com ([169.254.3.144]) with mapi id 14.03.0415.000; Tue, 5 Feb 2019 09:59:50 -0800
From: Alexander Clemm <alexander.clemm@huawei.com>
To: Martin Bjorklund <mbj@tail-f.com>, "ludwig@clemm.org" <ludwig@clemm.org>
CC: "yang-doctors@ietf.org" <yang-doctors@ietf.org>, "draft-ietf-netconf-yang-push.all@ietf.org" <draft-ietf-netconf-yang-push.all@ietf.org>, "netconf@ietf.org" <netconf@ietf.org>
Thread-Topic: [netconf] Yangdoctors last call review of draft-ietf-netconf-yang-push
Thread-Index: AQHUuLPBeoXN9KuEZUWoi0wHTEIcS6XRT9yAgAAXUwCAACA70A==
Date: Tue, 5 Feb 2019 17:59:49 +0000
Message-ID: <644DA50AFA8C314EA9BDDAC83BD38A2E0EBF0119@sjceml521-mbx.china.huawei.com>
References: <20190130.165118.1684515639662649114.mbj@tail-f.com> <004601d4bd1d$952072e0$bf6158a0$@clemm.org> <20190205.090312.1916826348030549950.mbj@tail-f.com>
In-Reply-To: <20190205.090312.1916826348030549950.mbj@tail-f.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-originating-ip: [10.193.35.84]
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-CFilter-Loop: Reflected
Archived-At: <https://mailarchive.ietf.org/arch/msg/netconf/iQUnSJqCDWsFs6FGiYGZLnp4NV0>
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 18:00:06 -0000

Thanks, Martin!
--- Alex

> -----Original Message-----
> From: netconf [mailto:netconf-bounces@ietf.org] On Behalf Of Martin
> Bjorklund
> Sent: Tuesday, February 05, 2019 12:03 AM
> To: ludwig@clemm.org
> Cc: yang-doctors@ietf.org; draft-ietf-netconf-yang-push.all@ietf.org;
> netconf@ietf.org
> Subject: Re: [netconf] Yangdoctors last call review of draft-ietf-netconf-yang-
> push
> 
> 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
> >
> _______________________________________________
> netconf mailing list
> netconf@ietf.org
> https://www.ietf.org/mailman/listinfo/netconf