Re: [yang-doctors] [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: yang-doctors@ietfa.amsl.com
Delivered-To: yang-doctors@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, 05 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/yang-doctors/xucGBll4KoylKTZyt0i2AibtaXI>
Subject: Re: [yang-doctors] [netconf] Yangdoctors last call review of draft-ietf-netconf-yang-push
X-BeenThere: yang-doctors@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Email list of the yang-doctors directorate <yang-doctors.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/yang-doctors/>
List-Post: <mailto:yang-doctors@ietf.org>
List-Help: <mailto:yang-doctors-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/yang-doctors>, <mailto:yang-doctors-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
- [yang-doctors] Yangdoctors last call review of dr… Martin Bjorklund
- Re: [yang-doctors] [netconf] Yangdoctors last cal… Kent Watsen
- Re: [yang-doctors] [netconf] Yangdoctors last cal… Alexander Clemm
- Re: [yang-doctors] [netconf] Yangdoctors last cal… Martin Bjorklund
- Re: [yang-doctors] [netconf] Yangdoctors last cal… Alexander Clemm