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

[Andy Bierman <[e-mail]>]

On Tue, Nov 28, 2017 at 1:37 AM, Martin Bjorklund <[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]
> https://www.ietf.org/mailman/listinfo/netconf
>