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

Hi Martin,

Thanks for the comments.   Many thoughts in-line...

> From: Martin Bjorklund, November 28, 2017 4:38 AM
> 
> 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:

In general we agree there are several alternatives to solve this issue.  And any mechanism supported by the WG would be fine.   

My read of your proposal below is that it does reuse elements of 3.10.  For example, I *think* you are proposing hierarchical inheritance of on-change property markings of the parent rather than making every instance.  Because marking every instance within a routing table would be prohibitively expensive.  So based on whatever commonality we can establish across the proposed alternatives, perhaps we can come to a common way to set this information.    If not, we can back out on-change marking as something implementation-specific for this current specification.

Stepping back a minute, it is good to think about who would use this on-change support data.  Application developers are going to make subscription requests.  And they are far more likely to want to design based on the schema level information exposed rather than instance data.   Driven by that constraint, the current proposal is framed as is -- mostly since there was no interest in Balazs'
https://tools.ietf.org/html/draft-lengyel-netmod-schema-annotation-00   
which was designed to support application developers needing this information.   Without this draft, at the time this was proposed the next-best thing was an extension which works based on the established pattern of deviations.   

To cover this issue, I have re-opened YP#10 to track and resolve...
https://github.com/netconf-wg/yang-push/issues/10 

Below are some specific pros/cons I see...

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

Fully agree this can be implementation level issue.  But then again, so is any deviation.  

So to cover "property of the implementation" case, the proposed extension would typically be included in deviations file per implementation.  Such an approach does mirror existing mechanisms for deviations, and has the benefit of not requiring another a new totally separate file to document the desired behavior.  

As for times this might be deployment specific, we saw potential capacity issues determining whether something might not be on-change subscribable.   Here it would be up to the implementation to determine whether to use the error "on-change-unsupported" from YANG Push, or the error "insufficient-resources" from subscribed notifications.  A choice between the two can be driven based on whether a deployment would *ever* have capacity for the requested subscription.

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

As the current solution was developed before NMDA, we didn't consider differences in on-change support for different datastores.  So this is a real consideration.

If for a minute we assume we are going to follow the current approach in the draft, the extension proposed in 3.10 could have a default of the operational datastore.  And we could add the option of including a list of datastores in the extension whenever on-change for something else is needed/supported..

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

Visibility to application developers was one reason we went down this path.   Any solution needs to frame this visibility as a key element.   Considering this, I actually see a need both for the schema based solution as framed, and the possibility of an instance based mechanism if someone wanted the lower level of granularity.  Per above, I don't think instance data within all instance objects like routing entries is possible anyplace datastore size becomes a factor.

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

This could work.   Several questions which would need to be addressed with this approach:

(1) how would application developers easily know what objects are on-change subscribable?

(2) If this is per-instance, is the property inherited from a parent?   If no, how do we deal with the size of the resulting information?

(3) If this is per-instance, how is this information populated?  Don't you need schema level guidance recorded somewhere anyway?

(4) Are there any issues with things like Schema mount?

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

If necessary, we can push this out.   I am hoping we can come up with something here, especially as the on-chance implementations I have seen have all used phased introduction of on-change for different schema objects.   

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

In general, I am fine with making the change to "Datastore" throughout the document.  For the title, I think it better to keep YANG to allow casual readers easy access to the context.

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

Can do.

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

Can do.

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

Should be 2, 12, & 22.   I have updated the definition (below) in a way which hopefully clarifies the proper behavior

         +  Dampening period: In an on-change subscription, detected object changes should be sent as quickly as possible.  However without adequate protections, a rapid series of object changes might exhaust of resources in the publisher or receiver.  In order to protect against that, a dampening period MAY be used to specify the interval which must pass before successive update records for the same subscription are generated for a receiver.  The dampening period collectively applies to the set of all data nodes selected by a single subscription and sent to a single receiver.  This means that when there is a change to a subscribed object, an update record containing that object is created either immediately when no dampening period is in effect, or at the end of a dampening period.  A dampening period is reset every time a new notification message is passed to transport.

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

I have updated the YANG module text as:

"Specifies the interval which must pass before successive update records for the same subscription are generated for a receiver.  The dampening period collectively applies to the set of all data nodes selected by a single subscription and sent to a single receiver.  This means that when there is a change to a subscribed object, an update record containing that object is created either immediately when no dampening period is in effect, or at the end of a dampening period.  A dampening period is reset every time a new notification message is passed to transport.  A dampening period is reset every time a new notification message is passed to transport.  A zero value indicates no dampening period, and all subscribed object changes are sent immediately."

> 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/ ?

Yes.  Will remove the SHOULD, showing just "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.

As this is informative, I can remove.  

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

ok

> 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?

As YANG Push is intended to be transport independent, it is better not to provide a specific transport.   Instead, how about the following text:

In a periodic subscription, the update record MUST include the datastore nodes which would have been retrieved using an equivalent datastore selection operation over that subscription's transport. 

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

If you are good with the text above, I will change 3.6 to:

" to what is available with a datastore selection operations".

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

We don't intend to define a new term in this paragraph.  And you are correct, policy only appears elsewhere in the document as part of the YANG model group names.    (more below)

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

What if I changes the first paragraph of Section 3.6 to:

An objective of YANG push is to allow the replication of a subset of a publisher's datastore within a receiver.  The datastore selection filter defines this subset.  Only a single selection filter can be applied to a subscription at a time.  The selection filter types defined in this include:

> 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?

There are plenty of valid xpath expressions which don't select data nodes.  How about: 

" xpath: An xpath selection filter is an XPath expression which references data nodes. When applied to a datastore, it is the results of this expression which will be pushed."

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

The issue with non-key properties is how to deal with the creation and deletion of objects when a non-key property goes in/out of the selection.   Others were uncomfortable with things like passing along a patch showing a node was deleted, when in reality the property simply changed from meeting the selection filter to failing the selection filter.  So people wanted to default to the simpler case of key properties for selection.

Based on that I can delete the words "but the goal is to provide equivalent capabilities to what is available with a GET".  

>   With the current text, is this filter ok:
> 
>      /interfaces/interface[contains(name, "eth")]
> 
>   It filters on a key, so it should be ok, right?

Yes

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

Will add how to populate.  In summary, sequential numbering of "edit-id" was from the RFC-8072.   And a null "patch-id" is because patch-id is mandatory in RFC-8072, and was originally supposed to be used for debugging of failed datastore write operations.  That really isn't the case here, plus we have subscription-id and other object available.

>   The "target" leaf is not specified correctly.  It should be:
> 
>          <target>/ietf-interfaces:interfaces-state</target>

Good catch.  Making change.

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

Yes

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

At the last IETF, Kent pointed us to YANG Lint which should allow us to do example checking in ways not previously known to us.    I will install and attempt to use these to clear the example items you have listed.  

> 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?

It cannot know this at "establish-subscription".   So a publisher will have to track whether the permissions on subscribed objects change.   How is left to implementations.

> 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/ ?

Will make that change.

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

Yes, good catch.  Will fix.

> o  4.3.2
> 
>      A subscription-id MUST be transported along with the subscribed
>      contents.
> 
>   Then the leaf "subscription-id" should be mandatory.

The requirement is that a subscription-id must be in the notification message, this doesn't mean that the subscription-id must be within the two new notifications.   The reason for this is that when we add support for the notification-messages draft, but having the subscription-id optional in the push-update and push-change-update, we can enable implementations which do not duplicate this header item.  (And a duplication would be forced with the future header if we made this 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?

Two reasons it is not:
(a) Other vendors have worried they can only support message-time, rather than notification-time.  
(b) Notification-time is the generalized name for "time-of-update" .  Having "time-of-update" as optional allows for eventual migration to common headers of  draft-ietf-netconf-notification-messages without information duplication

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

I think that answer is above.  But there is some good guidance here from draft-ietf-netconf-notification-messages.  The new draft ultimately will define and support the following times:

message-time: 
      "Header information consisting of time the message headers were placed generated prior to being sent to transport";

notification-time
      "Header information consisting of the time an originating process created the notification."

Notification-time should be equivalent to eventTime from RFC-5277.  But some other vendors have said they really only have message-time, and this is what they are populating in eventTime even though it doesn't explicitly fit the definition.  With the new definitions, at least they should be able to populate what they explicitly have.

> o  4.3.2
> 
>      If the application detects an informational discontinuity
> 
>   What is an "informational discontinuity"?

Will change to:
  If the application detects any incompleteness in set of objects placed into a "push-update" or "push-change-update"

> o  4.4.1 / 4.4.2
> 
>   The XML examples are broken; compare with my comments for 3.8.

Will do.  I am hoping a new attempt using yanglint helps pick out everything I missed by hand previously.

> o  5
> 
>   OLD:
> 
>     "This module contains conceptual YANG specifications
>      for YANG push.";
> 
>   NEW:
> 
>     "This module contains YANG specifications for YANG push.";

ok

> 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?

It is possible to expose QoS explicitly as a feature.   I will make that addition if you are ok with my other QoS comment described below about subscribed-notifications.

But even in that case if QoS is not supported, and someone includes QoS objects in an "establish-subscription" we need this identity as an error.  This error allow an explicit identification of what was wrong in the RPC (such as a DSCP provided is not supported by the Publisher).  I will include this in the Identity description.

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

Per our discussion above, this can be used if an RPC asks for on-change for an object which is not available at a platform deployment (either marked in the schema, or a specific deployment doesn't support an object which is included).   I will enhance the description based on the discussion on this earlier in this thread.

>     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?

Can be used in two places:

(1) Will be used if an RPC asks to synch on start, but it can't be supported for any reason (e.g., no nodes identifiable within the selection filter will ever be support on-change).   Will enhance the definition.

(2) The resynch RPC is invoked on a periodic subscription-id, or on an on-change subscription which can't support synchronization.

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

Will clarify description to explain that the key provided with the filter does not match the data type of the referenced yang subtree

>      identity datatree-size {
> 
>   Should it be "result-too-big" or something?

Yes.   I will change the name.

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

Yes, any new custom datastore will get a new identity, but if that new identity uses this custom-datastore one as a base, then we have an umbrella identity which is a handle just for the custom ones.  That was the intent of this.   If you don't think that an identity which acts as a bundling mechanism is useful, we can remove.

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

This is to identify churn within a datastore during a dampening period.  Two examples of when this is needed:

(1) For security applications, it is an absolute requirement that you know if a node was added and then removed during a dampening period.   For an example, a hacker adds a "permit any any" which is then quickly removed before the dampening period ends.  Remote applications need to know that churn occurred.

(2) For network management applications when a physical interface is going down and then quickly back up.   If the up/down is transient, how do you know that this occurred if there is no notification that churn occurred?

To support this, the YANG patch definition is loosened to allow the new creation on an existing node (based on what resulted from the last patch), or a delete on a missing node.  This acts as an indication that some churn in the datastore happened even though the data is in the same state as a per the previous update.  And if an application get to sees this indication when comparing to the previous state, they have the option of looking more closely at some logs on the publisher to determine what actually happened.

I can see that this behavior is under-described in the text.   I will put a small section between sections 3.10 & 3.11 describing this.  The title will be "Identification of a transient change within dampening period"

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

I will update the descriptions to match your draft.

As for when the evaluation occurs, I am not sure what you mean.  Every time a change occurs, you need to see if the changed object would have fallen within the selection.  There is no attempt to support an event-condition-action context.

> o  5 - terminology
> 
>   The term "agent" is used a couple of times.  Use "publisher" or
>   "server" instead.

Will update

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

I like it, will update.

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

Will change to MUST NOT.

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

This question has come up before.  Basically current 5277 event streams haven't shown need for QoS to date.   We know that QoS has been requested to handle congestion issues for YANG push.  So for simplicities sake, we bundled all QoS elements together in YANG push where we knew there was demonstrable need.    If someone really is asking for these QoS constructs for events, we can revisit.

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

So this is saying that the named datastore must exist before allowing the augment.   Works for me.

> 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?

This is to indicate a system error where not all objects can be extracted from the datastore.   (e.g., the routing table got *lots* bigger, and the periodic extract cannot be performed as expected.)  At least some implementations have seen this.   I will add some clarifying text.

> 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/

I can change to "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.

Will make the fix.

Thanks again for all the excellent thoughts!
Eric

> 
> /martin
> 
> _______________________________________________
> Netconf mailing list
> Netconf@ietf.org
> https://www.ietf.org/mailman/listinfo/netconf