Re: [Netconf] mbj's WGLC review of yang-push-17
Alexander Clemm <alexander.clemm@huawei.com> Thu, 30 August 2018 01:09 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 53FAE130ED4 for <netconf@ietfa.amsl.com>; Wed, 29 Aug 2018 18:09:52 -0700 (PDT)
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 UVvFqCxUoNxE for <netconf@ietfa.amsl.com>; Wed, 29 Aug 2018 18:09:48 -0700 (PDT)
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 3AFBA130EC5 for <netconf@ietf.org>; Wed, 29 Aug 2018 18:09:48 -0700 (PDT)
Received: from lhreml709-cah.china.huawei.com (unknown [172.18.7.107]) by Forcepoint Email with ESMTP id 6A3A6DD7C881D for <netconf@ietf.org>; Thu, 30 Aug 2018 02:09:45 +0100 (IST)
Received: from SJCEML701-CHM.china.huawei.com (10.208.112.40) by lhreml709-cah.china.huawei.com (10.201.108.32) with Microsoft SMTP Server (TLS) id 14.3.399.0; Thu, 30 Aug 2018 02:09:46 +0100
Received: from SJCEML521-MBS.china.huawei.com ([169.254.2.188]) by SJCEML701-CHM.china.huawei.com ([169.254.3.173]) with mapi id 14.03.0415.000; Wed, 29 Aug 2018 18:09:42 -0700
From: Alexander Clemm <alexander.clemm@huawei.com>
To: Martin Bjorklund <mbj@tail-f.com>
CC: "netconf@ietf.org" <netconf@ietf.org>, "evoit@cisco.com" <evoit@cisco.com>
Thread-Topic: [Netconf] mbj's WGLC review of yang-push-17
Thread-Index: AQHUNImzU9I4weZMB0iH1L0LVTTudKTT9lwAgAFivoCAAJuIoIABSioAgABMJJA=
Date: Thu, 30 Aug 2018 01:09:42 +0000
Message-ID: <644DA50AFA8C314EA9BDDAC83BD38A2E0EB5B91A@sjceml521-mbs.china.huawei.com>
References: <644DA50AFA8C314EA9BDDAC83BD38A2E0EB59E44@sjceml521-mbs.china.huawei.com> <20180828.100917.925597025431754922.mbj@tail-f.com> <644DA50AFA8C314EA9BDDAC83BD38A2E0EB5B32B@sjceml521-mbs.china.huawei.com> <20180829.150740.1323981906990283714.mbj@tail-f.com>
In-Reply-To: <20180829.150740.1323981906990283714.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.88]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-CFilter-Loop: Reflected
Archived-At: <https://mailarchive.ietf.org/arch/msg/netconf/5Qh2Mcm9VIxv6p9nQfVPORgZqHM>
Subject: Re: [Netconf] mbj's WGLC review of yang-push-17
X-BeenThere: netconf@ietf.org
X-Mailman-Version: 2.1.27
Precedence: list
List-Id: Network Configuration WG mailing 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: Thu, 30 Aug 2018 01:09:52 -0000
Hi Martin, thank you for your new suggestion - I will go through the occurrences of "push update" to clean up / clarify which ones refer to update record (respectively update) versus the notification. Regarding your previous comments (or replies to my responses to your comments), indeed I did not add responses to all replies of yours and in the course missed two things; my apologies; please see <ALEX3> Thanks --- Alex > -----Original Message----- > From: Martin Bjorklund [mailto:mbj@tail-f.com] > Sent: Wednesday, August 29, 2018 6:08 AM > To: Alexander Clemm <alexander.clemm@huawei.com> > Cc: netconf@ietf.org; evoit@cisco.com > Subject: Re: [Netconf] mbj's WGLC review of yang-push-17 > > Alexander Clemm <alexander.clemm@huawei.com> wrote: > > Hi Martin, > > > > thank you for your replies! One inline reply, <ALEX2> > > Ok. What about my other comments? I see that some of them still applies to > -18 (the ones you didn't reply to below). > > Also, looking at the diff for 17-18, I found a new issue: > > > o Section 3 > > Subscriptions specify when notification messages (also referred to as > "push updates") should be sent and what data to include in update > records. > > > I accept that you use "update record" and "update" for the same > thing (namely "A representation of one or more datastore node > updates."). > > But what is a "push update"? From the quoted text, it seems to be > the *notifican message* rather than the "update record". But when > the term "push update" is used, it seems to be a third name for > "update record". (For example in 3.9: "A publisher MUST ensure > that no non-authorized data is included in push updates.") > > There is also the specific "push-update" notification (one form of > "update record", the other being "push-change-update", I assume). > > In order to avoid confusion, I suggest that you change the > occurances of "push update" to simply "update" or "update record" > (or "push-update" notification, when/if that is referred to). > > > > /martin > > > > > > > Thanks > > --- Alex > > > > > -----Original Message----- > > > From: Martin Bjorklund [mailto:mbj@tail-f.com] > > > Sent: Tuesday, August 28, 2018 1:09 AM > > > To: Alexander Clemm <alexander.clemm@huawei.com> > > > Cc: netconf@ietf.org; evoit@cisco.com > > > Subject: Re: [Netconf] mbj's WGLC review of yang-push-17 > > > > > > Hi, > > > > > > Thanks for addressing my comments. Some replies inline. > > > > > > > > > Alexander Clemm <alexander.clemm@huawei.com> wrote: > > > > Hi Martin, > > > > > > > > thank you for your comments. > > > > > > > > Please see my responses inline, <ALEX> > > > > > > > > (Apologies for the sluggish response; I have been travelling) > > > > > > > > --- Alex > > > > > > > > > -----Original Message----- > > > > > From: Netconf [mailto:netconf-bounces@ietf.org] On Behalf Of > > > > > Martin Bjorklund > > > > > Sent: Wednesday, August 15, 2018 4:18 AM > > > > > To: netconf@ietf.org > > > > > Subject: [Netconf] mbj's WGLC review of yang-push-17 > > > > > > > > > > Hi, > > > > > > > > > > Kent Watsen <kwatsen@juniper.net> wrote: > > > > > > This message starts a Last Call on draft-ietf-netconf-yang-push-17: > > > > > > > > > > I have reviewed this document, and I think it is almost ready. > > > > > Here are my > > > > > comments: > > > > > > > > > > > > > > > o 3.3 > > > > > > > > > > The text says: > > > > > > > > > > Putting it all together, following is the conceptual process for > > > > > creating an push-change-update notification: > > > > > > > > > > Up until this point, the text has just talked about "update > > > > > record". Here it says "an push-change-update notification". > > > > > > > > > > Maybe simply s/an push-change-update notification/an update > record/? > > > > > > > > > > > > > <ALEX> The process itself talk also about sending the record to > > > > the receiver. How about refining it to say " following is the > > > > conceptual process for > > > > creating a update record as part of an on-change subscription:"? > > > > </ALEX> > > > > > > Ok. (s/a update/an update/) > > > <ALEX3> OK, closed </ALEX3> > > > > > o 3.4 > > > > > > > > > > The text says: > > > > > > > > > > the solution that is defined in this document mandates that a > > > > > publisher notifies receivers immediately and reliably whenever it > > > > > encounters a situation in which it is unable to keep the terms of the > > > > > subscription, and provides the publisher with the option to suspend > > > > > the subscription in such a case. > > > > > > > > > > I think it would help if you could put a forward reference to the > > > > > mechanism that exists to do this immediate notification. > > > > > > > > > > > > > <ALEX> Adding the following forward reference: > > > > "This is described further in Section 3.11.1." > > > > > > Ok. > > > <ALEX3> OK, closed </ALEX3> > > > > > > > > > > > > > > o 3.5.2 > > > > > > > > > > The text describes incorrect usage of the "insert" operation; it is > > > > > only applicable to user ordered lists. Also, there is no reason for > > > > > special handling of deletion of list entries. (also use the term > > > > > "list entry" rather than "element"). Hence, I suggest: > > > > > > > > > > OLD: > > > > > > > > > > A publisher will indicate a change to the effect that a value of a > > > > > datstore node has been updated by indicating a "replace" operation > > > > > (applied to the datastore node) in the patch. When a new datastore > > > > > node was created (other than an element in a list), a publisher will > > > > > indicate a "create" operation in the patch. When a datastore node > > > > > was deleted (other than an element in a list), the publisher > > > > > indicates this by a "delete". When a new list element was created or > > > > > removed, the publisher indicates it by an "insert" or "remove", > > > > > respectively. > > > > > > > > > > NEW: > > > > > > > > > > A publisher will indicate a change to the effect that a value of a > > > > > datstore node has been updated by indicating a "replace" operation > > > > > (applied to the datastore node) in the patch. When a new datastore > > > > > node was created (other than an entry in a user ordered list), a > > > > > publisher will indicate a "create" operation in the patch. When a > > > > > datastore node was deleted, the publisher indicates this by a > > > > > "delete". When a new entry in a user ordered list was created, the > > > > > publisher indicates this by an "insert" operation. > > > > > > > > > > > > > <ALEX> OK, changed. </ALEX> > > > > > > > > > > > > > > o 3.5.2 > > > > > > > > > > In the last paragraph, s/"merge"/"replace"/ > > > > > since the paragraph before just describes "replace", not "merge". > > > > > > > > > > > > > <ALEX> done </ALEX> > > > > > > > > > Shouldn't the text also mention the operation "move" for user > > > > > ordered lists? > > > > > > > > > <ALEX> Thank you. Added the following text: " When an entry in a > > > > user-ordered list was moved, the publisher indicates this by a "move" > > > > operation. " > > > > > > Ok. <ALEX3> OK, closed </ALEX3> > > > > > > > > o 3.6 > > > > > > > > > > s/Xpath/XPath/ > > > > > > > > <ALEX> done </ALEX> > > > > > > > > > > > > > > > > > > > o 3.7 > > > > > > > > > > The examples are not quite correct, and I suggest they are modified > > > > > to not include deprecated nodes: > > > > > > > > > > OLD: > > > > > > > > > > <notification > > > > > xmlns="urn:ietf:params:xml:ns:netconf:notification:1.0"> > > > > > <eventTime>2017-10-25T08:00:11.22Z</eventTime> > > > > > <push-update xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-push"> > > > > > <subscription-id>1011</subscription-id> > > > > > <datastore-contents> > > > > > <interfaces-state xmlns="urn:ietf:params:xml:ns:yang:ietf- > > > interfaces"> > > > > > <interface> > > > > > <name>eth0</name> > > > > > <oper-status>up</oper-status> > > > > > </interface> > > > > > </interfaces-state> > > > > > </datastore-contents> > > > > > </push-update> > > > > > </notification> > > > > > > > > > > NEW: > > > > > > > > > > <notification > > > > > xmlns="urn:ietf:params:xml:ns:netconf:notification:1.0"> > > > > > <eventTime>2017-10-25T08:00:11.22Z</eventTime> > > > > > <push-update xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-push"> > > > > > <subscription-id>1011</subscription-id> > > > > > <datastore-contents> > > > > > <interfaces xmlns="urn:ietf:params:xml:ns:yang:ietf-interfaces"> > > > > > <interface> > > > > > <name>eth0</name> > > > > > <oper-status>up</oper-status> > > > > > </interface> > > > > > </interfaces> > > > > > </datastore-contents> > > > > > </push-update> > > > > > </notification> > > > > > > > > > > > > > > > OLD: > > > > > > > > > > <notification > > > > > xmlns="urn:ietf:params:xml:ns:netconf:notification:1.0"> > > > > > <eventTime>2017-10-25T08:22:33.44Z</eventTime> > > > > > <push-change-update > > > > > xmlns="urn:ietf:params:xml:ns:yang:ietf-yang- > > > > > push"> > > > > > <subscription-id>89</subscription-id> > > > > > <datastore-changes> > > > > > <yang-patch xmlns="urn:ietf:params:xml:ns:yang:ietf-yang- > patch"> > > > > > <patch-id>1</patch-id> > > > > > <edit> > > > > > <edit-id>edit1</edit-id> > > > > > <operation>merge</operation> > > > > > <target>/ietf-interfaces:interfaces-state</target> > > > > > <value> > > > > > <interfaces-state xmlns="http://foo.com/ietf-interfaces"> > > > > > <interface> > > > > > <name>eth0</name> > > > > > <oper-status>down</oper-status> > > > > > </interface> > > > > > </interfaces-state> > > > > > </value> > > > > > </edit> > > > > > </yang-patch> > > > > > </datastore-changes> > > > > > </push-change-update> > > > > > </notification> > > > > > > > > > > NEW: > > > > > > > > > > <notification > > > > > xmlns="urn:ietf:params:xml:ns:netconf:notification:1.0"> > > > > > <eventTime>2017-10-25T08:22:33.44Z</eventTime> > > > > > <push-change-update > > > > > xmlns="urn:ietf:params:xml:ns:yang:ietf-yang- > > > > > push"> > > > > > <subscription-id>89</subscription-id> > > > > > <datastore-changes> > > > > > <yang-patch xmlns="urn:ietf:params:xml:ns:yang:ietf-yang- > patch"> > > > > > <patch-id>1</patch-id> > > > > > <edit> > > > > > <edit-id>edit1</edit-id> > > > > > <operation>replace</operation> > > > > > <target>/ietf-interfaces:interfaces</target> > > > > > <value> > > > > > <interfaces xmlns="urn:ietf:params:xml:ns:yang:ietf- > interfaces"> > > > > > <interface> > > > > > <name>eth0</name> > > > > > <oper-status>down</oper-status> > > > > > </interface> > > > > > </interfaces> > > > > > </value> > > > > > </edit> > > > > > </yang-patch> > > > > > </datastore-changes> > > > > > </push-change-update> > > > > > </notification> > > > > > > > > > > (uses the "replace" operation, fixed xml namespace, don't use > > > > > interfaces-state) > > > > > > > > > > > > > <ALEX> Thank you. Updated. </ALEX> > > > > > > > > > > > > > > o 3.8 > > > > > > > > > > s/establish-subscription-datasore-error-info/ > > > > > establish-subscription-datastore-error-info/ > > > > > > > > > > > > > <ALEX> Fixed, thank you </ALEX> > > > > > > > > > > > > > > o 3.8 > > > > > > > > > > The text says: > > > > > > > > > > In the case of a rejected request for an establishment of a datastore > > > > > subscription, the hints MUST be transported within a yang-data > > > > > "establish-subscription-datastore-error-info" container > > > > > > > > > > Do you mean that *if* there are hints, they MUST be sent within this > > > > > container, or you mean that on failure, this container MUST be sent? > > > > > > > > > > (ditto for modify-subscription-datastore-error-info) > > > > > > > > > > (it seems 4.4.1 says that hints SHOULD be included, but Appendix A > > > > > that they MUST be included) > > > > > > > > > > > > > <ALEX> Changing "MUST" to "SHOULD". And yes, *if* there are > > > > hints. I don't think a change to Appendix A, is needed; it does > > > > not actually state that they must be included. (It says the > > > > yang-data with the hint MUST be sent if hints are included.) > > > > > > So then it seems that this text (3.8) says that if there are hints, > > > they SHOULD be transported witin the "establish-subscription-datastore- > error-info" > > > container, but the appendix says that if there are hints they MUST > > > be sent within the "establish-subscription-datastore-error-info" > container. > > > > > > I would prefer to have the normative language in just one place > > > (probably not in the appendix). But I also think that the current > > > text in the appendix is more clear, since it says that *if* there > > > are hints, they MUST be transported... > > > > > > > > > > </ALEX> > > > > > > > > > > > > > > > > > > o 3.9 > > > > > > > > > > s/RFC8342/RFC8341/ > > > > > s/rfc6536bis/RFC8341/ > > > > > > > > > > > > > <ALEX> Thank you, changed </ALEX> > > > > > > > > > > > > > > o 3.9 > > > > > > > > > > It is difficult to relate Figure 5 to the text. Should it be moved > > > > > to right after the first paragraph? I think it would be useful to > > > > > add a reference from the text that examplains the "updated access > > > > > control rules" to the figure. > > > > > > > > > > > > > <ALEX> I moved the paragraph preceding Figure 5, as well as Figure > > > > 5 itself, further to the top (behind the first paragraph). > > > > </ALEX> > > > > > > > > > Also, s/update message/update record/ in the figure. > > > > > > > > > > > > > <ALEX> Done </ALEX> > > > > > > > > > > > > > > o 3.9 > > > > > > > > > > The text says: > > > > > > > > > > A publisher MAY choose reject an establish-subscription request > which > > > > > selects non-existent or access-protected data. In addition, a > > > > > publisher MAY choose to terminate a dynamic subscription or > suspend a > > > > > configured receiver when the authorization privileges of a receiver > > > > > change, or the access controls for subscribed objects change. Such a > > > > > capability enables the publisher to avoid having to support a > > > > > continuous, and total filtering of an entire subscription's content. > > > > > > > > > > In these cases above, the error identity "unchanging-selection" > > > > > SHOULD be returned. > > > > > > > > > > "the cases above" refers to (i) terminating a dynamic subscription, > > > > > or (ii) suspend a configured receiver. What does it mean to > > > > > "return" an error identity when a subscription is terminated, or > > > > > suspended? > > > > > > > > > > Maybe you meant that the error identity "unchanging-selection" > > > > > SHOULD be sent in an "subscription-terminated" notification or > > > > > "subscription-suspended" notification, respectively. > > > > > > > > > > If so, the "unchanging-selection" identity should probably also > > > > > derive from "sn:subscription-suspended-reason". > > > > > > > > > > > > > <ALEX> Changed this section as follows: > > > > "A publisher MAY choose to reject an establish-subscription > > > > request which selects non-existent or access-protected data. In > > > > addition, a publisher MAY choose to terminate a dynamic > > > > subscription or suspend a configured receiver when the > > > > authorization privileges of a receiver change, or the access > > > > controls for subscribed objects change. As reason, the error identity > "unchanging-selection" SHOULD be returned. > > > > Such a capability enables the publisher to avoid having to support > > > > continuous and total filtering of a subscription's content for > > > > every update record. It also reduces the possibility of leakage > > > > of access-controlled objects." > > > > </ALEX> > > > > > > This new text doesn't address my concern, which is the usage of the > > > term "return". How can a server "return" anything when a > > > subscription is terminated? > > > <ALEX3> Ah, ok, I overlooked this one. So, when rejecting, we can return the error identity. When the subscription is terminated, a notification "subscription-terminated" is sent (per the Subscribed Notifications draft) that can include ungaging-selection as its reason. I will make this explicit in -19. </ALEX3> > > > > > > > <ALEX> On a separate note, the next paragraph states: "If read > > > > access into previously accessible nodes has been lost due to a > > > > receiver permissions change, this SHOULD be reported as a patch > "delete" > > > > operation for on-change subscriptions. If not capable of handling > > > > such receiver permission changes with such a "delete", publisher > > > > implementations MUST force dynamic subscription re-establishment > > > > or configured subscription re-initialization so that appropriate > > > > filtering is installed." > > > > > > > > I am wondering if one should actually report the > > > > "delete"-operation here. This is somewhat inaccurate, as the > > > > object may not have been actually deleted, only its access has > > > > been revoked. I am leaving this as is for now, but am wondering > > > > if we should change this instead as > > > > follows: > > > > " If read access into previously accessible nodes has been lost > > > > due to a receiver permissions change, publisher implementations > > > > MUST force dynamic subscription re-establishment or configured > > > > subscription re-initialization so that appropriate filtering is installed." > > > > > > > > Thoughts? > > > > > > Ok with me. > > > <ALEX3> OK, closed </ALEX3> > > > > </ALEX> > > > > > > > > > > > > > > o 3.11.1 > > > > > > > > > > The text says: > > > > > > > > > > It is not > > > > > required to merge pending update messages. > > > > > > > > > > This can be read as indicating that a server MAY merge pending > > > > > update messages. I assume that it should say that pending update > > > > > messages MUST NOT be merged. > > > > > > > > <ALEX> Hmm. I am not sure I agree. The server is not required to > > > > merge pending update messages - i.e. can send multiple messages > > > > each with a separate update record. However, there is no reason > > > > to preclude that they could be combined. So, I don't think an > > > > update is needed here. > > > > </ALEX> > > > > > > So you say that the text means that the server MAY merge pending > > > update messages in this case? If so, I think you should update the > > > text so that this is clear. > > > <ALEX3> OK, how about saying "The server MAY (but does not have to) merge pending update messages". </ALEX3> > > > > > > > > Also, s/update message/update record/ > > > > > > > > > > > > > <ALEX> Sure </ALEX> > > > > > > > > > > > > > > o 4.2 > > > > > > > > > > s/an "excluded-change" flag/an "excluded-change" parameter/ > > > > > > > > > > (it is more than just a flag...) > > > > > > > > > > > > > <ALEX> Updated </ALEX> > > > > > > > > > > > > > > o 4.3.2 > > > > > > > > > > The second paragraph is a bit confusing. I suggest to simplify: > > > > > > > > > > OLD: > > > > > > > > > > A "subscription-id" MUST be transported along with the subscribed > > > > > contents. An [RFC5277] Section 4 one-way notification MAY be > used > > > > > for encoding updates. Where it is, the relevant "subscription-id" > > > > > MUST be encoded as the first element within each "push-update" > or > > > > > "push-change-update". This allows a receiver to differentiate which > > > > > subscription resulted in a particular push. > > > > > > > > > > NEW: > > > > > > > > > > A "subscription-id" is transported along with the subscribed > > > > > contents. This allows a receiver to differentiate which > > > > > subscription resulted in a particular push. > > > > > > > > > > > > > <ALEX> Updated </ALEX> > > > > > > > > > > > > > > o 4.4.1 > > > > > > > > > > The examples are (still) wrong. > > > > > > > > > > OLD: > > > > > > > > > > <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> > > > > > <yp:source xmlns:ds="urn:ietf:params:xml:ns:yang:ietf- > datastores"> > > > > > ds:operational > > > > > </yp:source> > > > > > <xpath-filter > > > > > xmlns:ex="http://example.com/sample-data/1.0" > > > > > select="/ex:foo"/> > > > > > </yp:datastore> > > > > > <yp:periodic> > > > > > <yp:period>500</yp:period> > > > > > </yp:periodic> > > > > > </establish-subscription> > > > > > > > > > > NEW: > > > > > > > > > > <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:periodic> > > > > > <yp:period>500</yp:period> > > > > > </yp:periodic> > > > > > </establish-subscription> > > > > > > > > > > > > > > > > > > > > OLD: > > > > > > > > > > <rpc-reply message-id="101" > > > > > xmlns="urn:ietf:params:xml:ns:netconf:base:1.0"> > > > > > <subscription-result > > > > > xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed- > notifications"> > > > > > ok > > > > > </subscription-result> > > > > > <identifier > > > > > xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed- > notifications"> > > > > > 52 > > > > > </identifier> > > > > > </rpc-reply> > > > > > > > > > > NEW: > > > > > > > > > > <rpc-reply message-id="101" > > > > > xmlns="urn:ietf:params:xml:ns:netconf:base:1.0"> > > > > > <identifier > > > > > xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed- > notifications"> > > > > > 52 > > > > > </identifier> > > > > > </rpc-reply> > > > > > > > > > > > > > > > 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 netconf:type="xpath" > > > > > 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: > > > > > > > > > > <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> > > > > > > > > > > > > > > > > > > <ALEX> Updated </ALEX> > > > > > > > > > > > > > > > > > > > o 4.4.1 > > > > > > > > > > REMOVE: > > > > > > > > > > o "error-app-tag" with the value being a string that corresponds to > > > > > an identity with a base of "establish-subscription-error". > > > > > > > > > > (this app-tag thing was removed from subscribed-notifications) > > > > > > > > > > > > > > > And modify the example accordingly: > > > > > > > > > > OLD: > > > > > > > > > > <rpc-reply message-id="101" > > > > > xmlns="urn:ietf:params:xml:ns:netconf:base:1.0"> > > > > > <rpc-error> > > > > > <error-type>application</error-type> > > > > > <error-tag>operation-failed</error-tag> > > > > > <error-severity>error</error-severity> > > > > > <error-app-tag> > > > > > on-change-unsupported > > > > > </error-message> > > > > > <error-path > > > > > xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-subscribed- > notifications"> > > > > > /yp:periodic/yp:period > > > > > </error-path> > > > > > </rpc-error> > > > > > </rpc-reply> > > > > > > > > > > NEW: > > > > > > > > > > <rpc-reply message-id="101" > > > > > xmlns="urn:ietf:params:xml:ns:netconf:base:1.0"> > > > > > xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-subscribed- > notifications"> > > > > > <rpc-error> > > > > > <error-type>application</error-type> > > > > > <error-tag>operation-failed</error-tag> > > > > > <error-severity>error</error-severity> > > > > > <error-path>/yp:periodic/yp:period</error-path> > > > > > <error-info> > > > > > <yp:establish-subscription-error-datastore> > > > > > <yp:reason>yp:on-change-unsupported</yp:reason> > > > > > </yp:establish-subscription-error-datastore> > > > > > </rpc-error> > > > > > </rpc-reply> > > > > > > > > > > > > > > > > > > <ALEX> I think there are some mismatches in the NEW as indicated > > > > above (no error-info closing bracket, xmln:yp without container) - > > > > updated as follows (NEW2): > > > > > > > > <rpc-reply message-id="101" > > > > xmlns="urn:ietf:params:xml:ns:netconf:base:1.0" > > > > xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"> > > > > <rpc-error> > > > > <error-type>application</error-type> > > > > <error-tag>operation-failed</error-tag> > > > > <error-severity>error</error-severity> > > > > <error-path>/yp:periodic/yp:period</error-path> > > > > <error-info> > > > > <yp:establish-subscription-error-datastore> > > > > <yp:reason>yp:on-change-unsupported</yp:reason> > > > > </yp:establish-subscription-error-datastore> > > > > </error-info> > > > > </rpc-error> > > > > </rpc-reply> > > > > > > > > </ALEX> > > > > > > > > > > > > > o 4.4.2 > > > > > > > > > > The example is wrong: > > > > > > > > > > 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"> > > > > > <identifier>1011</identifier> > > > > > <yp:datastore > > > > > xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores"> > > > > > ds:operational > > > > > </yp:datastore> > > > > > <yp:datastore-xpath-filter > > > > > netconf:type="xpath" 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: > > > > > > > > > > <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"> > > > > > <identifier>1011</identifier> > > > > > <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> > > > > > > > > > > > > > <ALEX> Done </ALEX> > > > > > > > > > > o 4.4.2 > > > > > > > > > > REMOVE: > > > > > > > > > > o "error-app-tag" with the value being a string that corresponds to > > > > > an identity with a base of "modify-subscription-error". > > > > > > > > > > > > > <ALEX> Done </ALEX> > > > > > > > > > > > > > > o 4.4.5 > > > > > > > > > > s/YANG 1.0/YANG 1/ > > > > > > > > > > (the version is "1", not "1.0") > > > > > > > > > > > > > > > > > <ALEX> Done </ALEX> > > > > > > > > > > > > > > o 4.4.5 > > > > > > > > > > I suggest you remove the sentence: > > > > > > > > > > The > > > > > "/modules-state/module-set-id" leaf in the "ietf-yang-library" > module > > > > > can be used to cache the YANG library information. > > > > > > > > > > This is chnaged with yang-library-bis, and the sentence is not > > > > > really needed in this draft. > > > > > > > > > > > > > <ALEX> Agreed, removed </ALEX> > > > > > > > > > > > > > > o 4.4.5 > > > > > > > > > > I don't understand what the third paragraph is supposed to tell me. > > > > > Can it be removed? > > > > > > > > > > > > > > > > > > <ALEX> Greatly simplified and rephrased it as follows: > > > > " The set of modules, revisions, features, and deviations can > > > > change at run-time (if supported by the publisher implementation). > > > > For this purpose, the YANG library provides a simple "yang-library- > change" > > > > notification that informs the subscriber that the library has changed. > > > > In this case, a subscription may need to be updated to take the > > > > updates into account. The receiver may also need to be informed > > > > of module changes in order to process updates regarding datastore > > > > nodes from changed modules correctly." > > > > </ALEX> > > > > > > Ok. > > > <ALEX3> OK, closed </ALEX3> > > > > > o 5 > > > > > > > > > > In subscribed-notifications, the subscription identifier leaf is > > > > > called "identifier", in this model it is called "subscription-id" > > > > > and "identifier". > > > > > > > > > > I think the two models should use the same term. Either change this > > > > > model, or subscribed notifications. Remember to update the > > > > > examples. > > > > > > > > > > > > > <ALEX> I think what you refer to is the way that subscriptions are > > > > identified in push update notifications. In all other places, we > > > > consistently use subscription-id as the type, and "identifier" to > > > > identify a subscription (YANG-Push does not introduce a new leaf, > > > > but imports yp:identifier). > > > > > > > > Personally I find the term "identifier" a bit generic. That said, > > > > if you want to make it consistent, we should stick with what we > > > > have in subscribed notifications (to keep the impact low). I am > > > > not really convinced it is a change for the better, but I am > > > > updating the model to use "identifier" in the push update > > > > notifications (push-update and > > > > push-change-update) to identify subscriptions. Examples have been > > > > updated accordingly. > > > > > > > > </ALEX> > > > > > > Ok. (I agree that "subscription-id" is more descriptive...) > > > <ALEX3> OK, closed </ALEX3> > > > > > o 5 > > > > > > > > > > Is it ok to do: > > > > > > > > > > <establish-subscription> > > > > > <datastore>operational</datastore> > > > > > </establish-subscription> > > > > > > > > > > Probably not, so I suggest making this illegal in the model: > > > > > > > > > > augment "/sn:establish-subscription/sn:input" { > > > > > when "sn:target/yp:datastore"; // NEW statement > > > > > > > > > > description > > > > > "This augmentation adds additional subscription parameters that > > > > > apply specifically to datastore updates to RPC input."; > > > > > uses update-policy; > > > > > } > > > > > > > > > > and ditto for all of: > > > > > > > > > > augment "/sn:modify-subscription/sn:input" { > > > > > augment "/sn:subscription-started" { > > > > > augment "/sn:subscription-modified" { > > > > > augment "/sn:subscriptions/sn:subscription" { > > > > > > > > > > > > > > > and then modify update-policy-modifiable to make the update- > trigger > > > > > choice mandatory: > > > > > > > > > > grouping update-policy-modifiable { > > > > > description > > > > > "This grouping describes the datastore specific subscription > > > > > conditions that can be changed during the lifetime of the > > > > > subscription."; > > > > > choice update-trigger { > > > > > mandatory true; // NEW > > > > > > > > > > > > > > > > > > <ALEX> I am not sure I understand this comment fully. Can you > > > > please clarify? You should be able to subscribe to updates to > <operational>. > > > > I don't think this should be precluded. > > > > I guess you are referring to the fact that you would like to make > > > > update-trigger mandatory? > > > > > > Yes. > > > > > > > Arguably, the server should always be able to reject a request it > > > > cannot fulfill, which may include underspecification. > > > > > > Sure, but if a parameter is mandatory it is better to mark it as > > > such instead of letting implementors figuring this out on their own. > > > > > > > Let me add your suggested augmentation to the inputs for > > > > establish-subscription and modify-subscription. I don't think > > > > this would be needed for a notification, which is generated by the > > > > server anyway. Let me also add the mandatory statement to the > > > > grouping; however, I am not sure this is actually legal, as the > > > > grouping will be used in an augmentation. (Can an augmentation > contain a "mandatory" > > > > item?) > > > > > > Yes, in YANG 1.1, and it was added specifically for use cases like > > > this, where the mandatory node are "protected" by a "when" > > > expression. > > > > > > <ALEX3> OK, incorporated your suggestions, closed </ALEX3> > > > > </ALEX> > > > > > > > > > > > > > > o 5 > > > > > > > > > > I have made this comment before. The anydata node > > > > > "datastore-changes" should be a container that uses the grouping > > > > > "yang-patch". It is more precise than using anydata and in text > > > > > explain that the opaque anydata must be yang patch. > > > > > > > > > > > > > <ALEX> ok </ALEX> > > > > > > > > > > > > > > > o 5 > > > > > > > > > > identity result-too-big { > > > > > > > > > > identity synchronization-size { > > > > > > > > > > > > > > > Why do we need both these errors? Can't we just have a single one, > > > > > maybe "update-too-big"? > > > > > > > > > > (I think that *result-too-big* is a misnomer. If the result of an > > > > > rpc is too big, the standard error-tag "too-big" should be > > > > > used.) > > > > > > > > <ALEX> Renamed "result-too-big" to "update-too-big". However, > > > > leaving the synchronization-size identity. That one would be used > > > > for resynch only. While strictly speaking it may be possible to > > > > eliminate, it probably doesn't hurt either. > > > > </ALEX> > > > > > > > > > > > > > > o 5 > > > > > > > > > > Since 3.5.2 specifies that a subset of all operations from YANG > > > > > patch to be used in push update records, shouldn't the typedef > > > > > change-type only include this subset? > > > > > > > > > > Otherwise, why should a client be able to exclude "merge", when > > > > > "merge" can never be included? > > > > > > > > > > > > > <ALEX> Not sure what you would like me to change? remove "merge" > > > from > > > > the enum? > > > > </ALEX> > > > > > > Yes, my suggestion is to remove "merge" and "remove". > > > > > > > <ALEX2> ok </ALEX2> > > > > > > > o 9.2 > > > > > > > > > > The reference [bergstra2014] is not used and can be removed. > > > > > > > > > > > > > <ALEX> Removed > > > > </ALEX> > > > > > > > > > > > > > > o Comment from an earlier review: > > > > > > > > > > (the document also uses the term "data object" and "datastore > > > > > object", these should be fixed) > > > > > > > > > > These should both be changed to "datastore node" or "object". > > > > > > > > > > > > > <ALEX> Changed the last remaining instances </ALEX> > > > > > > > > > > > > /martin > >
- [Netconf] mbj's WGLC review of yang-push-17 Martin Bjorklund
- Re: [Netconf] mbj's WGLC review of yang-push-17 Alexander Clemm
- Re: [Netconf] mbj's WGLC review of yang-push-17 Martin Bjorklund
- Re: [Netconf] mbj's WGLC review of yang-push-17 Alexander Clemm
- Re: [Netconf] mbj's WGLC review of yang-push-17 Martin Bjorklund
- Re: [Netconf] mbj's WGLC review of yang-push-17 Alexander Clemm
- Re: [Netconf] mbj's WGLC review of yang-push-17 Martin Bjorklund
- Re: [Netconf] mbj's WGLC review of yang-push-17 Alexander Clemm
- Re: [Netconf] mbj's WGLC review of yang-push-17 Martin Bjorklund
- Re: [Netconf] mbj's WGLC review of yang-push-17 Kent Watsen
- Re: [Netconf] mbj's WGLC review of yang-push-17 Martin Bjorklund
- Re: [Netconf] mbj's WGLC review of yang-push-17 Alexander Clemm
- Re: [Netconf] mbj's WGLC review of yang-push-17 Martin Bjorklund
- Re: [Netconf] mbj's WGLC review of yang-push-17 Kent Watsen
- Re: [Netconf] mbj's WGLC review of yang-push-17 Andy Bierman
- Re: [Netconf] mbj's WGLC review of yang-push-17 Yutianpeng (Tim)
- Re: [Netconf] mbj's WGLC review of yang-push-17 tom petch
- Re: [Netconf] mbj's WGLC review of yang-push-17 Andy Bierman
- Re: [Netconf] mbj's WGLC review of yang-push-17 Alexander Clemm
- Re: [Netconf] mbj's WGLC review of yang-push-17 Eric Voit (evoit)
- Re: [Netconf] mbj's WGLC review of yang-push-17 Kent Watsen
- Re: [Netconf] mbj's WGLC review of yang-push-17 Alexander Clemm
- Re: [Netconf] mbj's WGLC review of yang-push-17 Kent Watsen
- Re: [Netconf] mbj's WGLC review of yang-push-17 Juergen Schoenwaelder
- Re: [Netconf] mbj's WGLC review of yang-push-17 Andy Bierman
- Re: [Netconf] mbj's WGLC review of yang-push-17 Kent Watsen
- Re: [Netconf] mbj's WGLC review of yang-push-17 Juergen Schoenwaelder
- Re: [Netconf] mbj's WGLC review of yang-push-17 Qin Wu
- Re: [Netconf] mbj's WGLC review of yang-push-17 Juergen Schoenwaelder
- Re: [Netconf] mbj's WGLC review of yang-push-17 Qin Wu
- Re: [Netconf] mbj's WGLC review of yang-push-17 Juergen Schoenwaelder
- Re: [Netconf] mbj's WGLC review of yang-push-17 Qin Wu
- Re: [Netconf] mbj's WGLC review of yang-push-17 Robert Wilton
- Re: [Netconf] mbj's WGLC review of yang-push-17 tom petch
- Re: [Netconf] mbj's WGLC review of yang-push-17 Reshad Rahman (rrahman)