Re: [Netconf] review of draft-ietf-netconf-yang-push-11

[Alexander Clemm <[e-mail]>]

If we leave this to future work, ok, but what is the default behavior in that case?

Currently: on-change notifiable is supported only when specifically designated as such.

If we have no way to designate it, we can change it to assume it is supported, but reject on-change subscriptions that contain non-notifiable objects.  But how does a server know which ones they are?  We do not want to silently not send on-change updates, when the client assumes that we do.

One possibility would be to move the current solution to the appendix, defining this as one solution approach (and giving the YANG module for that).

--- Alex

From: Netconf [mailto:[e-mail]] On Behalf Of Andy Bierman
Sent: Tuesday, November 28, 2017 9:11 AM
To: Martin Bjorklund <[e-mail]>
Cc: Netconf <[e-mail]>
Subject: Re: [Netconf] review of draft-ietf-netconf-yang-push-11



On Tue, Nov 28, 2017 at 1:37 AM, Martin Bjorklund <[e-mail]<mailto:[e-mail]>> wrote:
Hi,


I have now reviewed draft-ietf-netconf-yang-push-11.  I have one
somewhat more important comment, and several others.

Important issue:

o  3.10

  I don't think the proposed YANG extension is the correct solution to
  the stated problem, for several reasons:

    1.  In most cases, this is not a property of the data model, but
        of the implementation, and possibly even the deployment.  So
        having a YANG extension statement is not a good solution.

    2.  With NDMA, the same schema node is present in different
        datastores.  It might be the case that an implementation
        supports on-change for the node in a configuration datastore,
        but not in operational.  Again, marking a node in the schema
        is not a good solution.

    3.  Since the on-change property is implementation dependent, it
        means the information will be available to clients only in
        deviation modules.  This is quite an expensive and complicated
        way to pass the information to the clients.


  An alternative solution could be to have an ordered list of
  instance-identifiers that list this property, per datastore, for
  example:

   <entry>
     <path>/sys:system/sys:system-time<path>
     <notifiable-on-change>false</notifiable-on-change>
   </entry>
   <entry>
     <path>/sys:system<path>
     <notifiable-on-change>true</notifiable-on-change>
   </entry>

  Yet another alternative would be to leave this to future work.


I would prefer to leave this to future work.
I agree this is an implementation property, and not a data model property.

Andy



Other comments:


o  2

  The document uses the term "YANG datastore" (it is even in the title
  of the document).  This term is not used elsewhere, and it is not
  defined in this document.  I suggest you change this to simply
  "datastore".

  Also, I think you should "import" the term "datastore" from
  draft-ietf-netmod-revised-datastores, instead of having a slightly
  different term in this document.

  It is also unfortunate that you use the term "data node" in a
  different meaning than RFC 7950.  Maybe use "datastore node"
  instead?


o  3.1

  I'm not sure I understand the dampening period concept.  Let's
  assume that the dampening period is 10s.  Then changes happen at
  times:

    2  3  4  11  13  14  15

  From the description, it seems I would receive 4 notifications, from
  times:

    2  (containing only change from 2)
    12 (containing change from 3,4,11)
    13 (containing only change from 13)
    23 (containing changes from 14,15)

  Is this correct?

  In any case, I suggest the description in the YANG module is
  clarified - currently the RFC text contains more details than the
  YANG module.


o  3.2

  The text says:

     Therefore, in order to minimize the number of subscription
     iterations between subscriber and publisher, dynamic
     subscriptions SHOULD support a simple negotiation between
     subscribers and publishers for subscription parameters.

  To whom is this "SHOULD" directed?  I mean, dynamic subscriptions as
  specified in this draft does indeed support simple "negotiation".
  Maybe s/SHOULD support/supports/ ?


o  3.4

  The text says:

    Please see [promise] for
    more on the transactional basis underlying the publisher and
    subscriber interactions within this document.

  So I did that.  But I didn't find any text in the referenced
  document that talked about "the transactional basis".

  I suggest you remove this sentence from the draft.


o  3.5

  The text says:

     A publisher MUST support XML encoding and MAY support other
     encodings such as JSON encoding.

  I don't think this is correct.  A RESTCONF sever is not required to
  support XML.  I think you should remove this sentence.


o  3.5.1

     In a periodic subscription, the data included as part of an update
     corresponds to data that could have been simply retrieved using a get
     operation and is encoded in the same way.

  What is "a get operation"?  Do you mean RESTCONF GET or NETCONF
  <get/> or something else?  Whatabout other protocols?

  (In 3.6, you use simply "a GET" for probably the same thing.)


o  3.6

     Subscription policy specifies both the selection filters and the
     datastores against which these selection filters will be applied.
     The result is the push of information necessary to remotely maintain
     an extract of the publisher's datastore.

  It seems this paragraph defines the term "Subscription policy".  But
  this term is not used in the document.  What does it means that the
  result of a policy is "the push of information"?

  I think I don't understand what this paragraph tries to tell me.


o  3.6

     o  xpath: An xpath selection filter is an XPath expression which may
        be meaningfully applied to a datastore.

   What does "meaningfully applied" mean?


o  3.6

     Selection filters are not intended to be used to filter objects
     based on a non-key property.  Supporting non-key property
     filtering so would have a number of implications that would
     result in significant complexity.

     [...]

     the goal is to
     provide equivalent capabilities to what is available with a GET.

  In GET you can filter on "non-key properties".

  I think you should remove the text about "non-key properties".  If
  anything, I think you can allow an implementation to reject a filter
  that would be too complex to implement / evaluate (in fact I think
  the text already allows a server to reject such filters.)

  With the current text, is this filter ok:

     /interfaces/interface[contains(name, "eth")]

  It filters on a key, so it should be ok, right?


o  3.7

  The XML examples are not using the correct XML namespace for the
  nodes from the "ietf-interface" module.

  The YANG Patch example also shows an interesting effect in the
  "patch-id" and "edit-id" leafs.  I think the draft should mention
  how implementations are suppose to fill in these leafs.


  The "target" leaf is not specified correctly.  It should be:

         <target>/ietf-interfaces:interfaces-state</target>


o  3.8

  The example has:

   <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="urn:ietf:params:xml:ns:yang:ietf-datastores">

  This doesn't match the YANG model; there is no container called
  "datastore" in the model.

  Further is has:

        <yp:subtree-filter netconf:type="xpath"
            xmlns:ex="http://example.com/sample-data/1.0"
            select="/ex:foo"/>

  a subtree-filter of type xpath?  I think ot should be:

        <yp:xpath-filter xmlns:ex="http://example.com/sample-data/1.0">
           /ex:foo
        </yp:xpath-filter>

  Also, it has:

        <yp:source xmlns="urn:ietf:params:xml:ns:yang:ietf-datastores">
          operational
        </yp:source>

  which should be:

        <yp:source xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores">
          ds:operational
        </yp:source>


o  3.9

  This section lists three cases for which:

     the error identity "data-unavailable" SHOULD be returned.

  One of the cases is:

    o  the authorization privileges of a receiver change over the course
       of the subscription.

  But how can a server know this when "establish-subscription" is sent?


o  3.9

     The contextual authorization model for data in YANG datastores is
     the NETCONF Access Control Model [RFC6536bis], Section 3.2.4.

  Did you mean s/contextual/conceptual/ ?


o  3.11.1


  OLD:

       If
       this is not possible and the synch-on-start option is configured,

  NEW:

       If
       this is not possible and the "no-synch-on-start" option is not
       present for the subscription.


  (fixes incorrect name of leaf, and also covers dynamic
  subscriptions)


o  4.3.2

     A subscription-id MUST be transported along with the subscribed
     contents.

  Then the leaf "subscription-id" should be mandatory.


     A "time-of-update" which represents the time an update record
     snapshot was generated.  A receiver MAY assume that a publisher's
     objects have these pushed values at this point in time.

  Should "time-of-update" be mandatory?

  How is "time-of-update" different from "eventTime" in the
  notification?


o  4.3.2

     If the application detects an informational discontinuity

  What is an "informational discontinuity"?


o  4.4.1 / 4.4.2

  The XML examples are broken; compare with my comments for 3.8.


o  5

  OLD:

    "This module contains conceptual YANG specifications
     for YANG push.";

  NEW:

    "This module contains YANG specifications for YANG push.";

o  5 - identities

    identity qos-unsupported {
      base sn:error;
      description
        "Subscription QoS parameters not supported on this platform.";
    }

  This identity is not mentioned anywhere in the text.  Instead of
  having this identity, wouldn't it be better to define a feature for
  "qos", and mark the nodes you have in mind with an if-feature?


    identity on-change-unsupported {
      base sn:error;
      description
        "On-change not supported.";
    }

  When will this identity be used?  There is already a feature
  "on-change" and corresponding if-feature statements.

    identity on-change-synch-unsupported {
      base sn:error;
      description
        "On-change synch-on-start and resynchonization not supported.";
    }

  The leaf is called "no-sync-on-start", which implies that sync on
  start is the default.  So when will this identity be used?


    identity reference-mismatch {
     base sn:error;
      description
       "Mismatch in filter key and referenced yang subtree.";
    }

  I don't understand the description of this identity.  Please
  clarify.


     identity datatree-size {

  Should it be "result-too-big" or something?


    identity no-such-datastore {

  I think this one should be removed.  The normal "invalid-value"
  error-tag covers this error.


      identity custom-datastore {
        base ds:datastore;
        description
          "A datastore with boundaries not defined within
           draft-ietf-netmod-revised-datastores";
      }

  This identity needs to be removed.  If someone defines a custom
  datastore, it would get a specific identity, and that identity can
  be used as "source".


o  5 - "change-type"

  The descriptions of the enums need to be improved.  This is about
  reporting a change in a datastore.  The value "create" is described
  as:

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

  Also, the description of the typedef has:

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

  But what does this mean?  The type is used to *exclude* some changes
  from a yang patch record.


o  5 - selection filter

  The XPath expression is not properly defined.

  OLD:

          "This parameter contains an XPath expression identifying the
          portions of the target datastore to retrieve.";

  NEW:

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

           FIXME: (*)

           The expression is evaluated in the following XPath context:

             o  The set of namespace declarations are those in scope on
                the '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.


  (*) - We need to describe when the filter is evaluated.  This is
  also true for the subtree filter.  Is it evaluated when the
  subscription is started and explicitly modified, or everytime a
  change is detected?

  [Side note: this description is also missing from the "xpath-filter"
  leaf in "get-data" in draft-ietf-netconf-nmda-netconf.  I have
  updated the description in that draft.]


o  5 - terminology

  The term "agent" is used a couple of times.  Use "publisher" or
  "server" instead.


o  5 - dampening

          leaf dampening-period {
            type yang:timeticks;
            mandatory true;

   Should this instead be:

          leaf dampening-period {
            type yang:timeticks;
            default "0";

   So that the request is the same if the "on-change" feature is
   support or not.


o  5 - no-synch-on-start

             When
             present, pushing a full selection per the terms of the
             selection filter MAY NOT be done for this subscription.

   Did you mean s/MAY NOT/MUST NOT/ ?

   MAY NOT is not a 2119 term.


o  5 - QoS

  Why are the qos parameters defined in yang push, and not in
  subscribed notifications?  It seems that they are generic.


o  5 - establish-subscription params

  I think a "when" expression should be added to the first augment:

    augment "/sn:establish-subscription/sn:input" {
      when "sn:target/yp:datastore/yp:source";


o  5 - push-update

  What does the presence of "updates-not-sent" mean in "push-update"?
  "push-update" contains a snapshot, not a diff, so why is
  "updates-not-sent" present?


o  5 - push-change-update

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

   What is an "incremental set"?  Probably s/incremental set/set/



o  General

  Use double quotes for node names.  "push-update", "subscription-id"
  etc.  Quotes are currently used inconsistently, and it makes the
  text harder to read.




/martin

_______________________________________________
Netconf mailing list
[e-mail]<mailto:[e-mail]>
https://www.ietf.org/mailman/listinfo/netconf