Re: [yang-doctors] [netconf] Yangdoctors last call review of draft-ietf-netconf-yang-push
"Alexander Clemm" <ludwig@clemm.org> Tue, 05 February 2019 06:40 UTC
Return-Path: <ludwig@clemm.org>
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 8419A131056; Mon, 4 Feb 2019 22:40:02 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.899
X-Spam-Level:
X-Spam-Status: No, score=-1.899 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RCVD_IN_DNSWL_NONE=-0.0001, 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 uVj93L0OxJF1; Mon, 4 Feb 2019 22:39:59 -0800 (PST)
Received: from mout.perfora.net (mout.perfora.net [74.208.4.194]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 44DDA130DEC; Mon, 4 Feb 2019 22:39:59 -0800 (PST)
Received: from LAPTOPR7T053C2 ([73.189.160.186]) by mrelay.perfora.net (mreueus001 [74.208.5.2]) with ESMTPSA (Nemesis) id 0MXG1D-1gea3t0nB7-00WOkt; Tue, 05 Feb 2019 07:39:48 +0100
From: Alexander Clemm <ludwig@clemm.org>
To: 'Martin Bjorklund' <mbj@tail-f.com>, yang-doctors@ietf.org, draft-ietf-netconf-yang-push.all@ietf.org
Cc: netconf@ietf.org
References: <20190130.165118.1684515639662649114.mbj@tail-f.com>
In-Reply-To: <20190130.165118.1684515639662649114.mbj@tail-f.com>
Date: Mon, 04 Feb 2019 22:39:43 -0800
Message-ID: <004601d4bd1d$952072e0$bf6158a0$@clemm.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
X-Mailer: Microsoft Outlook 16.0
Thread-Index: AQHJlRXNo2tCszHCez6iSUCatlerEKXnEDng
Content-Language: en-us
X-Provags-ID: V03:K1:fAKHgEHKzEyrRltZVgM94AqjgPcNvUfk4nLOZk8B9KhJhQv8TA7 ZP2umLsrZ49SFxb9v7HAYAaIiub8uizb4SJ8i55OBzOODuLJ7oQZf40yoNmSxOaUZy0ngpX 4s5afny/nVK1NjasmCWufIoxE69XyL6ZbnDVv87fl0qJGg2RsxCInvM3rUxqKDl15mGO5BB vPlfYCpw/kUY78/zrA/RA==
X-UI-Out-Filterresults: notjunk:1;V03:K0:+TzXWIC/0jI=:VSK9ASYlJlKULFyeemyit7 mIqyWMogDQbJRG8FD0wEeYQRWan/iy0OndTmvbN7MT+4KndIpAcdjpNMUFyQUJ0lW3GBgWKE9 T3oE9LZrcr7b4jjWdvqcA+RYKs9yqaRdguUPKlDnEEigMK+xpj5FlqvHYUtRdLOywyaEpXH75 suRkGJzhBZthkppL28h3dBSFQvk5KzzMfqoGDIl/q83tjadFyNsRwNcdN1CkaeIMbnrkubFl7 aIlA8/8UJxA5mEGVLB0pxykhE9ySdtKp7cRmoavQcWs0iNrQ/clGSzTENyEa3zGpCus9gVIvJ +ZGKhRu4xgE/Ulhm/Scc5KQyNwO+WBH5G4IbMHIEmJRB0XCE17v59X8izAd7UBFIZUAoqALqz d1OXP558hkf3Lm6ZE7Q+BKfG/QefQTHahrA3NKfHvzaBR7fsP+Yw00K9diUQn3frvc5dgT1tw piV8mxDSik016kWJ74qEZaDygCdSU2n1kHB7FGA8sbCForwBcVaXff7CVSLpzBGKyA4q3/ORS Rr84gK3wK5UpkUzColfLa8wkXB3KMZsvRadseTjralvflMbEHhWtz88TEzfH1J1+QdD53r8Ny Tx716gkL6KcuSjy73KFJxd/O+OqTHAIh9oPGV3IkzHBANbjVUnf0kzJhK+IuBUYYu04xjUucG fubaKM0K6CYL+wg37Guhbyfo6W5/rr9VLZDfC9yxogLbC+RCN6l4R/XklEBYvYBv4bbhfC3dG F3wGcaT2WuSF/gUkeA/aYDtdEsGz0kQgvhPS0tdisHbHGXIUJeSdJqAPLms=
Archived-At: <https://mailarchive.ietf.org/arch/msg/yang-doctors/saMYwUiQInoLcUvGNv4ymzErFGo>
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 06:40:03 -0000
Hi Martin, Thank you for your review! We have just posted an updated revision (-22) per your comments. (Please see replies inline, <ALEX>) --- 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
- [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