[Netconf] mbj's WGLC review of yang-push-17

[Martin Bjorklund <[e-mail]>]

Hi,

Kent Watsen <[e-mail]> 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/?


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.


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.


o  3.5.2

  In the last paragraph, s/"merge"/"replace"/
  since the paragraph before just describes "replace", not "merge".

  Shouldn't the text also mention the operation "move" for user
  ordered lists?


o  3.6

  s/Xpath/XPath/


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)


o  3.8

  s/establish-subscription-datasore-error-info/
    establish-subscription-datastore-error-info/


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)


o  3.9

  s/RFC8342/RFC8341/
  s/rfc6536bis/RFC8341/


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.

  Also, s/update message/update record/ in the figure.


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".


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.

  Also, s/update message/update record/


o  4.2

  s/an "excluded-change" flag/an "excluded-change" parameter/

  (it is more than just a flag...)


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.


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>




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>


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>


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".


o  4.4.5

  s/YANG 1.0/YANG 1/

  (the version is "1", not "1.0")


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.


o  4.4.5

  I don't understand what the third paragraph is supposed to tell me.
  Can it be removed?


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.


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



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.



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.)

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?



o  9.2

  The reference [bergstra2014] is not used and can be removed.


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".



/martin