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

[Martin Bjorklund <[e-mail]>]

Hi,


"Eric Voit (evoit)" <[e-mail]> wrote:
> 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.

Yes.

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

Agreed.  See also my reply to Alex.

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

Ok.


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

Ok.

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

Ok.

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

I understand what you want to achieve, but I don't think the text is
quite correct.  I'll think about this some more to see if I can come
up with a better wording.

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

This last sentence occurs twice.

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

Ok.

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

Or maybe

   In a periodic subscription, the data included as part of an update
   corresponds to data that could have been read using a retrieval
   operation over that subscription's transport.

(no need for MUST here)

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

Hmm, that paragraph is:

   It is not expected that implementations will support comprehensive
   filter syntax and boundless complexity.  It will be up to
   implementations to describe what is viable, but the goal is to
   provide equivalent capabilities to what is available with a GET.

What is a "comprehensive filter syntax"?  Maybe this paragraph can be
removed.

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

My problem with this style is that it seems to indicate that the
selection filter is only used for this use case ("replication of a
subset..."), but that would be a mistake.  I think you should explain
what these filters are first, and then (maybe) give examples of how
they can be used.

The title "Datastore selection filter" is also misleading; in fact the
best solution might be to keep the original text but change the title
of the section to "Subscription Policy".


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

The text for subtree filters use the words "When specified, updates
will only come from the data nodes of selected YANG subtree(s)."  If
you use the same words here, the text is consistent and easier to
understand.  Maybe:

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

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

But you are constraining the entire solution to support just this use
case, when a simpler unconstrained solution just as easily supports
this use case, *plus other use cases* - if a client does not want
these "fake deletes", they can simply construct filters based on keys
only, which they would in the current solution anyway.

Note also that b/c of the last paragraph in section 3.9, a client must
deal with such deletes anyway, even if it specificied only keys.

But I do understand your point.  This is also related to my comment
below about when a filter is evaluated.  Based on you reply to that
comment, I think the conceptual algorithm is like this:

  Just before a change, evaluate the filter and any access control
  rules.  The result is a set "A" of nodes (including subnodes).

  Just after a change, evaluate the filter and any (possibly new)
  access control rules.  The result is a set "B" of nodes (including
  subnodes).

  Construct a YANG patch record for going from A to B.   If the record
  is non-empty, send it to the subscriber.

(this is conceptual, an implementation can do lots of optimizations to
avoid evaluating filters on un-related changes)


> 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

What about this one:

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

it also filters on a key is it ok?

[if the answer is "yes" we have a problem; and if it is "no", we
need to tighten the text]


All this makes me wonder if it is correct to have "subtree" and
"xpath" filters at all.  If they only can match on keys, they become
severely constrained.

An alternative could be to specify filters as
"nacm:node-instance-identifier" instead; these are
instance-identitifiers that allow missing keys.  For example:

  /if:interfaces/if:interface/if:oper-status

This would be easier to understand and probably easier to optimize for
in the server code.

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

Maybe "patch-id" could be a sequential number, starting from 1 when
the first patch is sent?  Or simply any string that the server finds
appropriate.  In any case, "null" looks odd in the example.

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

Ok, but the error code is used as a return value for
"establish-subscription".  So if a server can't detect it at
"establish-subscription" it mean it will never be used.  Hence I
suggest you remove the text about "authorization privileges".

BTW, does this imply that I cannot create a filter for a currently
non-existing interface?   If this is true, my conceptual filter
evaluation algorithm above is not correct...

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

Aha, ok.  But then it looks really weird to have "scubscription-id"
in the notifications defined in this module.  It will be redundant.

Or do you suggest that *all* YANG modules that publish notifications
must have a leaf "subscription-id", until the notification header
document is done?  If not, why is this document special?

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

Ok.  So this means that when we have these new headers,
"time-of-update" is no longer needed, right?

But actually, currently we have "eventTime".  And since
"notification-time" is equivalent to "eventTime", and "time-of-update"
is "notification-time", it follows that "time-of-update" is redundant
and not needed today either.  Correct?

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

I suggest you remove the sentence instead, and some more redundant
words to get:

OLD:

   An "updates-not-sent" object, which indicates that the update record
   is incomplete.  If the application detects an informational
   discontinuity in either notification, the notification message MUST
   include "updates-not-sent".  This object indicates that not all
   changes which have occurred since the last update are actually
   included with this update.

NEW:

   An "updates-not-sent" object.  This object indicates that not all
   changes which have occurred since the last update are actually
   included with this 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.

No.  See RFC7950, section 8.3.1, bullet 4.  And compare with all other
modules that use if-feature; no other module has invented such error
codes.

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

Ok; i.e., the text should explain that it is used if the client asks
for an obejct that does not support on-change.  The if-feature case is
already handled as described above.

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

But in this case the error will be "on-change-unsupported", right?

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

Huh?  What does *that* mean?

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

It will use ds:datastore as base.  If we need some other generic base
from which custom datastores are derived, that identity should be
defined in a generic place (ietf-datastores probably), and not here.

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

Yes.  (At least from this document.)

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

You didn't reply to this.  My point is 



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

I think there's some confusion here.  How would the "excluded-change"
leaf-list be used in this case?  (the typedef we're discussing is only
used in "excluded-change")

If an application needs to know *every* change, it probably shouldn't
use a dampening period.

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

Well, yes this certainly needs to be described.  I will have to think
about the implications of this change.

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

So then that has to be explained.  Otherwise, one might think that
the filter is evaluated once, and then changes are reported whenever
any node in the selected subtrees are changed.

See also my conceptual algorithm above.  


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

I think it is important to try to design a standard so that it is
useful for more than one current use case.  In this case, it seems to
me that the qos parameters logically belong to the subscribed
notifications layer, and thus should be defined there.

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

Ok.

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

Ok.

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