Last Call Review of draft-ietf-netconf-yang-push-15
review-ietf-netconf-yang-push-15-yangdoctors-lc-bjorklund-2018-03-19-00

Request Review of draft-ietf-netconf-yang-push
Requested rev. no specific revision (document currently at 22)
Type Last Call Review
Team YANG Doctors (yangdoctors)
Deadline 2018-03-21
Requested 2018-03-07
Requested by Kent Watsen
Other Reviews Secdir Early review of -00 by Takeshi Takahashi (diff)
Yangdoctors Early review of -06 by Bert Wijnen (diff)
Yangdoctors Last Call review of -21 by Martin Björklund (diff)
Review State Completed
Reviewer Martin Björklund
Review review-ietf-netconf-yang-push-15-yangdoctors-lc-bjorklund-2018-03-19
Posted at https://mailarchive.ietf.org/arch/msg/yang-doctors/I1da98IeMu5ZFucOE3oL208rDGI
Reviewed rev. 15 (document currently at 22)
Review result Almost Ready
Draft last updated 2018-03-19
Review completed: 2018-03-19

Review
review-ietf-netconf-yang-push-15-yangdoctors-lc-bjorklund-2018-03-19

Hi,

Here are my WGLC comments and YANG doctor's review on
draft-ietf-netconf-yang-push-15.


o  On p. 13, theres a missing " (which messes up the colors in the
   emacs mode I use)

   OLD:

      responses to an establish-subscription request) or "modify-
      subscription-error-datastore (for error responses to a modify-

   NEW:

      responses to an establish-subscription request) or "modify-
      subscription-error-datastore" (for error responses to a modify-


o  Section 2

  I suggest you write that this document uses the terms defined in
  other docs:

  OLD:

   The terms below supplement those defined in
   [I-D.draft-ietf-netconf-subscribed-notifications].  In addition, the
   term "datastore" is defined in
   [I-D.draft-ietf-netmod-revised-datastores].

  NEW:

   This document uses the terminology defined in [RFC7950],
   [I-D.draft-ietf-netmod-revised-datastores], and
   [I-D.draft-ietf-netconf-subscribed-notifications].


  For clarity, I also suggest you introduce the new terms in a bullet
  list:

  OLD:

   Datastore node: An instance of management information in a datastore.
   Also known as "object".

  NEW:

   o  datastore node: An instance of management information in a datastore.
      Also known as "object".


  Also, in one place, you use the term "data resource".  Either import
  that term from the proper document, or change to "datastore node".



o  Section 2

  I think you definition of "datastore node" can be made more clear:


  OLD:

   Datastore node: An instance of management information in a datastore.
   Also known as "object".

  NEW:

   o  datastore node: An instance of a data node in a datastore.

  NOTE: I think you should stick to the term "datastore node", and not
  use the term "object".  I know that "object" is used quite a lot in
  this document, but I still think we should change this.

  (the document also uses the term "data object" and "datastore
  object", these should be fixed)


o  Section 2

  You have:

   Datastore node update: A data item containing the current value/
   property of a datastore node at the time the datastore node update
   was created.

  This definition is not clear to me.  The term is used twice in the
  document so maybe it can be removed.  When it is used in section
  3.1, it is not clear that you mean a "data item".

  My suggestion is to remove this term from the terminology list, but
  use the words "datastore node update" as you already do.


o  Section 2

  You have:

   Update record: A representation datastore node update(s) resulting
   from the application of a selection filter for a subscription.  An
   update record will include the value/property of one or more
   datastore nodes at a point in time.  It may contain the update type
   for each datastore node (e.g., add, change, delete).  Also included
   may be metadata/headers such as a subscription identifier.

  s/A representation datastore node/A representation of datastore node/

  What is "value/property"; first, does "/" mean "and" or "or"?
  second, what is a "property" of a datastore node?

  Ditto for "metadata/headers".


o  Section 2

  The term "Update trigger" isn't used in the document.  I suggest you
  remove it.


o  Section 3

  You write:

   This document specifies a solution for a push update subscription
   service.

  I think this should be reworded; what exactly is a "push update
  subscription service"?

  The you have:

   This solution supports dynamic as well as configured
   subscriptions to information updates from datastores.

  Maybe s/information updates from datastores/updates to datastore
  nodes/


o  Section 3.1

      There are two
      types of triggers for subscriptions: periodic and on-change.

  I think you mean that there are two types of subscriptions:

      There are two
      types of subscriptions: periodic and on-change.

  B/c later in the document you use the terms "on-change subscription"
  and "periodic subscription".  I never see "on-change trigger".

  These terms should be defined in the terminology section, b/c they
  are central.


o  Section 3.4

  The title is "Promise-Theory Considerations".

  I have pointed this out before; either change the title, or have a
  reference to the promise-theory you relate to, and explain in more
  details how you relate to this theory.

  Also, this section says:

   If for
   some reason the publisher of a subscription is not able to keep its
   promise, receivers MUST be notified immediately and reliably.

  Is this intended as a requirement on the server, or on the solution?

  If it is the former, it shouldn't be defined here, but rather be
  explicit when the operations are defined (which I think it is).

  If it is the latter, you should make that clear, and remove the 2119
  language.


o  Section 3.5.2

  It is not clear how the server is supposed to construct the YANG
  patch record.  You have some text about what a client should do, but
  no instructions for the server.  When would a server use the
  "remove" edit-operation, rather than "delete"?

  Instead of having special rules for the client, wouldn't it be
  better to say that the server MUST NOT use "create" and "delete"
  (instead use "repalce" and "remove").  Then the YANG patch semantics
  is left intact.


o  Section 3.6

  (terminology)

  OLD:

   o  xpath: An xpath selection filter is an XPath expression that
      returns a node set.  When specified, updates will only come from
      the selected data nodes.

  NEW:

   o  xpath: An xpath selection filter is an XPath expression that
      returns a node set.  When specified, updates will only come from
      the selected datastore nodes.

  Also, when the word "xpath" is used in general text, it should be
  "XPath".  When it refers to a YANG node/type etc, it should be
  quoted:  "xpath".


o  Section 3.7

  In the examples, the XML namespace for RFC 7223 is wrong:

  s/http://foo.com/ietf-interfaces/
    urn:ietf:params:xml:ns:yang:ietf-interfaces/


o  Section 3.7

  You write:

   Also if used as a counter, the counter MUST be reset
   to '1' the after passing a maximum value of '99999'.

  Isn't a max of 65535 or 4294967295 more reasonable? (uint16, or uint32)


o  Section 3.8

  OLD:

   The RPCs defined within
   [I-D.draft-ietf-netconf-subscribed-notifications] have been enhanced
   to support datastore subscription negotiation.  Included in these
   enhancements are error codes which can indicate why a datastore
   subscription attempt has failed.

  NEW:

   The RPCs defined within
   [I-D.draft-ietf-netconf-subscribed-notifications] have been
   augmented to support datastore subscription negotiation.  Also, new
   error codes that indicates why a datastore subscription attempt has
   failed have been added.


o  Section 3.8

  You write:

   o  "error-app-tag" with the value being a string that corresponds to
      an identity with a base of "establish-subscription-error" (for
      error responses to an establish-subscription request), "modify-
      subscription-error" (for error responses to a modify-subscription
      request), "delete-subscription-error" (for error responses to a
      delete-subscription request), "resynch-subscription-error" (for
      error responses to resynch-subscription request), or "kill-
      subscription-error" (for error responses to a kill-subscription
      request), respectively.

  This is not how errors are handled in
  draft-ietf-netconf-subscribed-notifications.  Why do you have two
  different mechanisms?  And why is this identity needed; you already
  have the identity in the "establish-subscription-error-datastore".

  I think you should decide on one mechanism, and use it in both
  drafts (specifically, the error-app-tag handling.  BTW, *if* you
  decide to keep it, you need to clarify what "a string that
  corresponds to" means.  Maybe use the JSON encoding of identities in
  this case (<modname>:<identityname>)).

  Also, in the example in Figure 4, the "reason" leaf is missing.


o  Section 3.9

  Because of the NACM rules you have here, should this document
  formally "Update" 6536bis?


o  Section 3.10

  (clarification)

  OLD:

   In some cases, a publisher supporting on-change notifications may not
   be able to push updates for some object types on-change.

  NEW:

   In some cases, a publisher supporting on-change notifications may not
   be able to detect changes in all datastore nodes.


o  Section 3.11.1

  You're using the term "MAY NOT", but this is not a 2119 term (BTW,
  you need to include the usual 2119 boilerplate)  I think you mean
  "MUST NOT".


o  Section 4

  You should remove the text describing the tree diagrams, and instead
  reference:

    Tree diagrams used in this document follow the notation defined in
    [I-D.ietf-netmod-yang-tree-diagrams].

o  Section 4.1

  The tree diagram is quite hard to read.  I suggest to prune the tree
  a bit by removing unnecessary nodes defined in
  ietf-subscribed-notifications:

module: ietf-subscribed-notifications
  ...
  +--rw filters
  |  ...
  |  +--rw yp:selection-filter* [identifier]
  |     +--rw yp:identifier                        sn:filter-id
  |     +--rw (yp:filter-spec)?
  |        +--:(yp:datastore-subtree-filter)
  |        |  +--rw yp:datastore-subtree-filter?   <anydata>
  |        |          {sn:subtree}?
  |        +--:(yp:datastore-xpath-filter)
  |           +--rw yp:datastore-xpath-filter?     yang:xpath1.0
  |                   {sn:xpath}?
  +--rw subscriptions
     +--rw subscription* [identifier]
        |  ...
        +--rw (target)
        |  +--:(stream)
        |  |  ...
        |  +--:(yp:datastore)
        |     +--rw yp:datastore
        |     |       identityref
        |     +--rw (yp:selection-filter)?
        |        +--:(yp:by-reference)
        |        |  +--rw yp:selection-filter-ref
        |        |          selection-filter-ref
        |        +--:(yp:within-subscription)
        |           +--rw (yp:filter-spec)?
        |              +--:(yp:datastore-subtree-filter)
        |              |  +--rw yp:datastore-subtree-filter?
        |              |          <anydata> {sn:subtree}?
        |              +--:(yp:datastore-xpath-filter)
        |                 +--rw yp:datastore-xpath-filter?
        |                         yang:xpath1.0 {sn:xpath}?
        |  ...
        +--rw (yp:update-trigger)?
           +--:(yp:periodic)
           |  +--rw yp:periodic!
           |     +--rw yp:period         yang:timeticks
           |     +--rw yp:anchor-time?   yang:date-and-time
           +--:(yp:on-change) {on-change}?
              +--rw yp:on-change!
                 +--rw yp:dampening-period?    yang:timeticks
                 +--rw yp:no-synch-on-start?   empty
                 +--rw yp:excluded-change*     change-type

  (This was generated with the --tree-line-length 69 option to pyang,
  then manually edited to add the "...")


  But I also think that the tree diagram is difficult to read b/c you
  include everything in one single diagram.  I suggest you put the
  config part of the diagram (shown above) in section 4.2, then a tree
  diagram snippet per notification in sections 4.3.1 and 4.3.2, and
  rpc tree snippets in 4.4.*.


o  Section 4.2

  (editorial)

  OLD:

   Both configured and dynamic subscriptions are represented within the
   list subscription.  New and enhanced parameters extending the basic
   subscription data model in
   [I-D.draft-ietf-netconf-subscribed-notifications] include:

  NEW:

   Both configured and dynamic subscriptions are represented within the
   list "subscription".  New parameters extending the basic
   subscription data model in
   [I-D.draft-ietf-netconf-subscribed-notifications] include:


o  Section 4.2

  You write:

   o  For periodic subscriptions, triggered updates will occur at the
      boundaries of a specified time interval.  These boundaries many be
      calculated from the periodic parameters:

   s/many/may/  or s/many be/are/


o  Section 4.2

  You write:

      *  a "period" which defines duration between period push updates.

  s/period push updates/push updates/


o  Section 4.3.1

  You write:

   Subscription state notifications and mechanism are reused from
   [I-D.draft-ietf-netconf-subscribed-notifications].  Some have been
   augmented to include the datastore specific objects.

  Instead of writing "some have been augmented", be explicit and list
  the ones that have been updated (I think it is just two).


o  Section 4.3.2

  The first paragraph:

  OLD:

   Along with the subscribed content, there are other objects which
   might be part of a "push-update" or "push-change-update"

  NEW:

   Along with the subscribed content, there are other objects which
   might be part of a "push-update" or "push-change-update"
   notifications.


o  Section 4.3.2

  (terminology)

  OLD:

   An "incomplete-update" object.  This object indicates that not all

  NEW:

   An "incomplete-update" leaf.  This leaf indicates that not all


o  Section 4.4.1

  The XML example is not correct.  Please ensure that all examples are
  validated by some tool.   Also I suggest you ensure the examples are
  consistently indented, and that they use "xmlns" consistently.  As
  it looks a bit messy.

  In this case, there's an element <yp:source> in the XML that doesn't
  exist in the datamodel; the element <xpath-filter> is placed within
  a leaf, and incorrectly named (it should be
  <yp:datastore-xpath-filter>); and it uses an incorrect "select"
  attribute.


o  Section 4.4.1

  You write:

   The publisher MUST respond explicitly positively (i.e., subscription
   accepted) or negatively (i.e., subscription rejected) to the request.

  (similar text in a couple of other places as well)

  What exactly does this tell me?  What does "explicitly" mean?  Is
  this telling me that the server MUST NOT contain any bugs?

  I suggest you remove this sentence (and the other similar ones).


o  Section 5

  (fix syntax so that extraction tools work properly)

  OLD:

    <CODE BEGINS>; file "ietf-yang-push@2018-02-23.yang"

  NEW:

    <CODE BEGINS> file "ietf-yang-push@2018-02-23.yang"


  In general, the module is quite hard to understand, due to the usage
  of all groupings.


o rpc resynch-subscription

    description
      "This RPC allows a subscriber of an active on-change
       subscription to request a full push of objects in their current
       state. A successful result would invoke a push-update of all
       datastore objects that the subscriber is permitted to access.
       This request may only come from the same subscriber using the
       establish-subscription RPC.";

  Consider removinging the words "in their current state"; a
  'push-update' always send nodes in their current state.

  Rephrase "invoke a push-update"; maybe :result in the publisher sending a
  'push-update' notification".

  Rephrase the last sentence; you probably mean that it must be sent
  on the same session where the subscription was established.

  Also, include text in the rpc description that explains that the
  resynch-subscription-error is sent on error.



o  error identities

  When reading the YANG module, it is not really clear how these
  identities are supposed to be used.  For example, one of the first
  identities is:

    identity cant-exclude {
      base sn:establish-subscription-error;
      description
        "Unable to remove the set of 'excluded-changes'.  This means the
         publisher is unable to restrict 'push-change-update's to just the
         change types requested for this subscription.";
    }

  Since this is derived from sn:establish-subscription-error, it seems
  that the idea is to use it anywhere an
  sn:establish-subscription-error is expected, e.g., as the "reason"
  in "establish-subscription-error-stream".  But this is probably not
  true?

  I think that since you define new yang-data structure for errors
  related to push-subscriptions, you shouldn't use the
  sn:establish-subscription-error at all; instead you should define
  a new set of base identities in this module.



o  typedef change-type

  The description in this type talks about "data resource" (see my
  comment on terminology above).  The description doesn't quite match
  how this type is used.  For example:

      enum "create" {
        description
          "Create a new data resource if it does not already exist.  If
          it already exists, replace it.";
      }

  This type is used only in "excluded-change", and as such it is
  used to inform the server about which changes the client doesn't
  want.  So shouldn't the description text describe this?  Something
  like this for "create":

    "Instructs the server that if a datastore node is created, it
     should not trigger an update."

  Maybe also change the name of the typedef to "excluded-change-type".

  See also my comment on 3.5.2 above.


o  leaf datastore-xpath-filter

  In the description, do:

    s/'xpath-filter' leaf/'datastore-xpath-filter' leaf/


o  list selection-filter

  The description says:

        "A list of pre-positioned filters that can be applied
         to datastore subscriptions.";

  What is a "pre-positioned" filter?  I suggest:

        "A list of filters that can be applied
         to datastore subscriptions.";

  (I just noticed that the same words exists in the
  subscribed-notifications document; I suggest the same change is
  applied there as well.)


o  leaf selection-filter/identifier

  This has:

      leaf identifier {
        type sn:filter-id;

  I think this is a bit over-engineered.  I suggest to simply use
  "type string" here.   And the same in ietf-subscribed-notifications;
  the type filter-id should be removed.

  Also, I think all existing modules have followed some kind of
  unspoken convention that arbitrary named list entries have a key
  called "name" of type string.  When there's some kind of restricted
  identifier used, the key leaf is called "id" or "<foo>-id"
  (e.g. "router-id", "session-id" etc).


o  notifications push-update and push-change-update

  Both these have this text:

       This notification shall only be sent to receivers of a
       subscription; it does not constitute a general-purpose
       notification.

  Why do you have this text?  It seems to imply that it would be
  illegal for a server to ever send these notifs on some "stream".
  Why is this necessary?  If I have a clever use case for this, why is
  it made illegal?


o  anydata datastore-contents in notification push-update

      description
        "This contains the updated data.  It constitutes a snapshot
         at the time-of-update of the set of data that has been
         subscribed to.  The format and syntax of the data
         corresponds to the format and syntax of data that would be
         returned in a corresponding get operation with the same
         selection filter parameters applied.";

  This description is not precise enough.  What is "a corresponding
  get operation"?  Does this text mean that I will get different
  contents if I use NETCONF than if I use RESTCONF?



o  anydata datastore-changes in notification push-change-update

  You write:

        "This contains the set of datastore changes needed
         to update a remote datastore starting at the time of the
         previous update, per the terms of the subscription.

  Isn't this backwards?  Shouldn't it be:

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

  With the current text, it seems the server must have knowledge about
  the remote datastore (if it even exists).


  Further, it says:

         Changes
         are encoded analogous to the syntax of a corresponding yang-
         patch operation, i.e. a yang-patch operation applied to the
         datastore implied by the previous update to result in the
         current state.

  I don't understand what this text says.  Looking at the example, I
  think that what you really should do is remove that sentence, and
  change this "anydata" node to a container:

     container datastore-changes {
       uses ypatch:yang-patch;
       ...
     }

  With this approach, you can even "refine" the description statement
  of some nodes (e.g. the "operation" leaf) and explain the
  create/delete semantics.


  This node also has this reference:

      reference
        "RFC 8072 section 2.5, with a delta that it is ok to receive
         ability create on an existing node, or receive a delete on a
         missing node.";

   The reference statement is just supposed to contain the formal
   reference.  If descriptive text is needed it should go into the
   "description".  But in this case I think you should simply remove
   the text in the reference.


o  augment "/sn:subscription-modified"
    description
      "This augmentation adds many datastore specific objects to
       the notification that a subscription has been modified.";


  Please be more specific than "adds many nodes".  How many is many?


o  augment "/sn:subscription-modified/sn:target"

    case datastore {
       uses datastore-criteria {
          refine "selection-filter/within-subscription" {
          description
            "Specifies where the selection filter, and where it came
            from within the subscription and then populated within this
            notification.

   This sentence doesn't parse.



o  The YANG module is inconsistently indented.  To get some help, you
   can use the latest version of pyang on github and run:

     $ pyang -f yang --keep-comments ietf-yang-push.yang > x
     $ diff ietf-yang-push.yang x



/martin