[netconf] Yangdoctors last call review of draft-ietf-netconf-yang-push
Martin Bjorklund <mbj@tail-f.com> Wed, 30 January 2019 15:51 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 1F091130EB9; Wed, 30 Jan 2019 07:51:28 -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 aKPd4P52dlq7; Wed, 30 Jan 2019 07:51:24 -0800 (PST)
Received: from mail.tail-f.com (mail.tail-f.com [46.21.102.45]) by ietfa.amsl.com (Postfix) with ESMTP id B2046130F4F; Wed, 30 Jan 2019 07:51:20 -0800 (PST)
Received: from localhost (h-4-215.A165.priv.bahnhof.se [158.174.4.215]) by mail.tail-f.com (Postfix) with ESMTPSA id 77CAE1AE012C; Wed, 30 Jan 2019 16:51:18 +0100 (CET)
Date: Wed, 30 Jan 2019 16:51:18 +0100
Message-Id: <20190130.165118.1684515639662649114.mbj@tail-f.com>
To: yang-doctors@ietf.org, draft-ietf-netconf-yang-push.all@ietf.org
Cc: netconf@ietf.org
From: Martin Bjorklund <mbj@tail-f.com>
X-Mailer: Mew version 6.7 on Emacs 25.2 / Mule 6.0 (HANACHIRUSATO)
Mime-Version: 1.0
Content-Type: Multipart/Mixed; boundary="--Next_Part(Wed_Jan_30_16_51_18_2019_579)--"
Content-Transfer-Encoding: 7bit
Archived-At: <https://mailarchive.ietf.org/arch/msg/netconf/rDk8FCRIqYyBdyQiK-SJ0uFlULw>
Subject: [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: Wed, 30 Jan 2019 15:51:29 -0000
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. 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. 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 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. 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> 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> 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> 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]. o import yang-patch OLD: reference "RFC 8072: YANG Patch"; NEW: reference "RFC 8072: YANG Patch Media Type"; 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"; 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. 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.) 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."; } 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) 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 o Section A.2 Similar to A.1, suggest you remove the sn: errors that are already covered by SN. /martin
- [netconf] Yangdoctors last call review of draft-i… Martin Bjorklund
- Re: [netconf] Yangdoctors last call review of dra… Alexander Clemm
- Re: [netconf] Yangdoctors last call review of dra… Martin Bjorklund
- Re: [netconf] Yangdoctors last call review of dra… Alexander Clemm