Re: [Netconf] Martin's thoughts on subscribed-notifications

["Eric Voit (evoit)" <[e-mail]>]

> From: Martin Bjorklund, October 16, 2017 8:40 AM
>
> Hi,
> 
> In general, do you keep track of issues that the WG needs to provide feedback
> on?  If so, where is that list?  (for example, compare wih
> https://github.com/netmod-wg/datastore-dt/issues).   There are a
> number of issues in this review that need WG feedback.

A similar process has been used here...
From April 2016 through February 2017, we tracked issues on the drafts a netconf-wg github issues list for each draft 
e.g., https://github.com/netconf-wg/yang-push/issues

But that created issues as going to multiple locations across the multiple drafts made it difficult to see everything at once.  So we migrated to more centralize tracking across the set of drafts in places like: 
 - https://github.com/netconf-wg/yang-push/wiki/Minutes 
 - weekly digests on the opening or closing of issues came to the WG mailer. 
To keep track of the longer term issues, these were also listed and summarized within the drafts themselves in the appendices

At this point for the issues below, we should return to using the issue tracker for each git repository.  

> "Eric Voit (evoit)" <[e-mail]> wrote:
> 
> [...]
> 
> > > draft-ietf-netconf-subscribed-notifications-05
> > > ----------------------------------------------
> > >
> > > A general comment is that this document is quite hard to read.  The
> > > terminology is not used consistently, and often new terms are used
> > > without explanation.
> > >
> > > As an example, section 4.1 describes the "stream" input parameter to
> > > "establish-subscription" as:
> > >
> > >    o  A stream which identifies the domain of events against which the
> > >       subscription is applied.
> > >
> > > This sounds complicated, what is a "domain of events"?
> >
> > A publisher designated feed of asynchronous information.  Will change
> > domain to "continuous feed".
> 
> I don't think that would be correct in this case. 

Are you saying a stream isn't a continuous feed of asynchronous information?

>  If you define what a stream is, then you can use those words.  But in this case, the "stream" parameter is the
> name of a particular stream on the server.

"A stream which identifies" can be easily be matched by a reader into stream name as an input parameter.  The rest of the text is trying to help by explaining the purpose of stream.  This is a stylistic choice I made so that readers who are to get an idea of why parameters are being used without having to jump back and forth to the descriptions in the yang model.
 
> > > Isn't the parameter simply the name of a stream?
> >
> > Yes.
> 
> Then you should say so.   Try to make the text clear.

This is my goal.

> > > I suggest the text is cleaned up wrt. terminology.  Below I also
> > > suggest to remove some text that is not really needed, to make the
> > > document easier to read.
> >
> > We have done our best to do this over iterations of the document.  And
> > we will continue to do this for specific examples raised (like you
> > will see below as identified by your thorough review here).
> >
> > > The relationship to RFC 5277 is not clear.  This document provides a
> > > conflicting definition of streams.  Is this new definition supposed
> > > to replace the old one, or are both supposed to co-exist?  The
> > > stream mechanism in RFC 5277 works, is implemented in many systems,
> > > and is used quite a lot in real operations.  I object to replacing
> > > it with something new.  Specifically, the stream name should
> > > continue to be a string, not an identity, in order to contine to
> > > support dynamically created streams.
> >
> > The authors discussed this often.  We simply had not heard of 5277
> > implementations where streams are dynamically created after system
> > definition.  So we were waiting to see if someone actually used this
> > capability before we wanted to include support, as it creates more
> > validation complexity.
> >
> > In other words, we are ok with moving to "string".  And the version
> > available Monday will do this now that we know the capability is
> > actually used.
> 
> Good.
> 
> But also note that I wrote "The relationship to RFC 5277 is not clear".  I suggest
> you add such a section to the draft.

Relationship to 5277 is covered extensively in Section 7.

> > > General stylistic suggestion: don't use angle brackets for YANG
> > > definitions.  For example, don't write:
> > >
> > >   <subscription-result>
> > >
> > > but instead:
> > >
> > >   "subscription-result".
> >
> > Can do.
> >
> > > o  Section 1
> > >
> > >   What is a "subscription control plane operations binding"?
> >
> > I can shift it just to "bindings".  The extra words above help a
> > subset of people who work with routers.
> 
> This is the complete paragraph:
> 
>    While the functionality defined in this document is transport-
>    agnostic, subscription control plane operations bindings exist for
>    both NETCONF [RFC6241] and RESTCONF [RFC8040].  In addition, bindings
>    for the pushed event instances have been defined for protocols such
>    as NETCONF and HTTP2 [RFC7540].
> 
> [some time later...]
> 
> Aha - now I get it!  You mean that:
> 
>   (1) NETCONF or RESTCONF can be used to configure the
>       subscriptions.
> 
>   (2) NETCONF and HTTP2 can be used for notification delivery.
> 
> 
> Is this correct?

Absolutely correct.

> (Hmm, then I wonder what the document
> draft-ietf-netconf-restconf-notif does)

There are a number elements which are necessary for that draft.  But the goal always has been to make the protocol dependent part as small as possible. 

> > >   In general, are dynamic subscriptions for NETCONF only?  This needs
> > >   to be clarified.
> >
> > The paragraph says transport-agnostic.  Why might this be read as
> > NETCONF only?
> 
> Dynamic subscriptions uses the "establish-subscription" rpc.  How can this RPC
> be used for RESTCONF to actually get notifications?

The RPC Is never used in with either NETCONF or RESTCONF for the sending of subscribed content.

> Maybe add something similar to section 3.7 from RFC 5277?

This information is transport protocol specific, and does exist.  But in the transport drafts:

i.e., draft-ietf-netconf-netconf-event-notification section 3.3.2.4, 3.4.3, 3.4.4.1, 3.4.7.4

ie., draft-ietf-netconf-restconf-notif -- section 3.1.1, 3.1.2, 3.1.3


> 
> > > o  Section 1.1
> > >
> > >     o  multiple subscriptions on a single transport session
> > >
> > >   How is this achieved?  As far as I can see, this is not specified.
> >
> > The details are not specified in the introduction for sure.  I could
> > add an extra sentence in section 4.1 which says: "Multiple establish
> > subscription RPC requests can be made using the same transport
> > session."  Would this satisfy your concern?
> 
> Ok.
> 
> > >     o  negotiation of subscription parameters
> > >
> > >   There is no negotiation going on.  The RPC succeeds or fails.  I
> > >   suggest this is removed.
> >
> > The effective result of providing hints (as described later in the
> > document) is the enablement of negotiation phase of subscription
> > establishment.  It is just that this negotiation spans multiple RPC
> > requests.
> >
> > I could add "through the use of hints when a requested subscription is
> > unsupportable".
> 
> The output from a failing establish-subscription is:
> 
>     |        |  +--ro filter-failure?           string
>     |        |  +--ro replay-start-time-hint?   yang:date-and-time
> 
> 
> Filter failure is a free form string, this can't be used for any
> (automatic) negotiation.

Agree that a free-form string is not a good basis for negotiation.  More negotiation elements do come with yang-push.   And requirements for the general negotiation interaction model can be seen in RFC 7923, section 4.2.2.

> replay-start-time-hint can be used for this purpose, but as I wrote earlier, I
> don't think establish-subscription should fail if the start-time is "too early".

There was a discussion on the WG mailing list a while ago on this.  Yves Beauville started a thread on this and explicitly brought the behavior you are describing as problematic.  See the following...
https://www.ietf.org/mail-archive/web/netconf/current/msg12154.html

This hits the Promise Theory topic below.  With subscriptions, we need to give the subscriber *exactly* what they ask for.   We cannot provide "kind-of" what they ask for, or else we do not know if what is provided is really what the consuming application needs.  By forcing the asking of what is needed, we can be sure the network device is not serving up a set of information which doesn't really meet the application needs.

This interaction model might seem marginally important to some for replay start time.  But even then, it is not as meaningful blocks of events can get overlooked.   But as we extend towards yang push, what if a publisher arbitrarily decides to only push periodic counters only once every minute versus a subscriber request of once every 10 seconds.  Or what if a security application can only send on-change delta updates every hour versus the every 10 minutes needed by the application?  Applications across the network can fall apart if network elements feel they are free to deliver less than what was requested.  Strict compliance to requests is a way to avoid this.

> > >     o  promise theory based interaction model
> > >
> > >   Why is this listed here?  I suggest this is removed.
> >
> > This is a basic design consideration of the underlying interaction
> > model upon which the document is based.  It differentiates the
> > solution from others which can fail without performing the essential
> > step of telling anyone they have failed.
> 
> I think you should remove this, or if you feel strongly that it actually provides
> useful information to the reader, write text that explains this.  

See yang-push, section 3.4.  

> It would be interesting to hear what the WG thinks about this.  

Others are free to comment for sure.  The promise theory basis of the interaction model has been documented since the first releases of the set of documents, and the purpose/need negotiation socialized since IETF 95.  The design basis for this decision is the reasoning for the interaction model above.

> > > o  Section 2.1
> > >
> > >   The document adds a stream called SYSLOG, which:
> > >
> > >    mirrors the
> > >    discrete set entries which are concurrently being placed into a
> > >    device's local Syslog
> > >
> > >   I don't understand what this means, and it is not clear how the
> > >   syslog messages are to be transported in XML or JSON.
> > >
> > >   I suggest this stream is removed.
> >
> > In talking with authors of RFC-5277, and looking at early drafts of
> > that RFC, they made a strong push for the inclusion of a SYSLOG
> > stream.  The reason they did not include it was that it was not
> > possible to agree across vendors on the way to format or encode SYSLOG
> > entries.  Every vendor did it uniquely.
> >
> > Looking at v05 of this draft, when stream is an identity, there at
> > least there can be an identity which is sharable across vendors.  But
> > If we move to using string for stream, that value pretty much goes
> > away, and I can remove SYSLOG.
> >
> > So unless there is someone who wants to fight for stream as an
> > identity, I will remove.
> 
> Ok.
> 
> > > o  Section 2.2
> > >
> > >   The first paragraph is difficult to parse.
> >
> >
> > How about:
> > Events where the applied filter evaluates to "true" MUST traverse that
> > filter.  Similarly, events where the applying the filter return a
> > non-null selection MUST traverse the filter.
> 
> I don't think this helps.  I think what you want to say is:
> 
>   - The solution has an extensible filter concept
>   - Two filter syntaxes are defined, subtree filter and xpath filter,
>     both MUST be supported by a server (?)

I have found in the IoT space, people are comfortable with xpath, but not subtree.  Others more comfortable with YANG might like subtree.   And it is possible that for some IoT low-event publishers, no filtering is desirable.

>   - Only notifications that match the filter are sent to the receiver.

The two examples of text both should get to the desired result.  The text I proposed explicitly covers both boolean and selection behaviors. 

> > > o  Section 3
> > >
> > >   The data model should use a NMDA-style single tree.
> >
> > This model has been in existence well before NMDA existed.
> 
> I know.
> 
> > We are
> > hoping that the YANG doctors will make the final determination if we
> > need to make this conversion.  If they require it, we will make the
> > change.
> 
> The recommendation from the YANG doctors and the AD is to follow the
> NMDA style for new data models.

And we will make the change if required.

> > > o  establish-subscription
> > >
> > >   The input parameters to this operation are weird:
> > >
> > >    +---x establish-subscription
> > >     |  +---w input
> > >     |  |  +---w encoding?            encoding
> > >     |  |  +---w (target)
> > >     |  |  |  +--:(stream)
> > >     |  |  |     +---w (event-filter)?
> > >     |  |  |     |  +--:(by-reference)
> > >     |  |  |     |  |  +---w event-filter-ref     event-filter-ref
> > >     |  |  |     |  +--:(within-subscription)
> > >     |  |  |     |     +---w (filter-spec)?
> > >     |  |  |     |        +--:(subtree-filter)
> > >     |  |  |     |        |  +---w subtree-filter?      <anydata>
> > >     |  |  |     |        +--:(xpath-filter)
> > >     |  |  |     |           +---w xpath-filter?        yang:xpath1.0
> > >     |  |  |     +---w stream               stream
> > >     |  |  |     +---w replay-start-time?   yang:date-and-time {replay}?
> > >     |  |  +---w stop-time?           yang:date-and-time
> > >
> > >   Note how the "stream" name is not even mandatory, it is down in a
> > >   case.  I assume this is not intentional?
> >
> > This is intentional, and is a result of the structure of YANG-Push,
> > and a desire to re-use the RPCs for both datastore and stream
> > subscription.
> >
> > An early design goal was that subscription can be made to either to a
> > stream or to a datastore target.
> 
> Then you need to explain this.  Currently, section 4.1 says:
> 
>    The <establish-subscription> operation allows a subscriber to request
>    the creation of a subscription via RPC.
> 
>    The input parameters of the operation are:
> 
>    o  A stream which identifies the domain of events against which the
>       subscription is applied.
> 
> You need to explain that the subscription mechanism is extensible.
> Explain the common behavior of a subscription, regardless of what has been
> subscribed to.

I will enhance the subscription state model in Section 2.3 to add the following at the end:

The interaction model described in this section is mirrored in the RPCs and Notifications later in the document.  It should be noted that these RPCs and Notifications have been designed to be extensible and allow subscriptions into targets other than event streams.   [I-D.ietf-netconf-yang-push] provides an example of such an extension.

I will also add some words to the end of Section 4 intro.  "Dynamic subscriptions are managed via RPC.  These RPCs have been designed to be extensible for contexts beyond event streams."

> > As the YANG-Push augments the target
> > node, the structure above allows a stream to appear only when it is a
> > stream which is being subscribed.
> >
> > An RPC request which includes neither a stream nor datastore target
> > should be rejected.  I will update the definition of "target" to
> > indicate that some form of target is mandatory.
> 
> In the data model, the choice "target" is already marked as mandatory.

Yes, saw this was covered during the re-read.

> However, the descripton needs some rewording:
> 
>     choice target {
>       mandatory true;
>       description
>         "A filter must be applied against some source of information.
>         This identifies the target for the filter.";
> 
> This description is not correct.

Propose changing the description to:

"Identifies the source of information against which a subscription is being applied, as well as specifics on the subset of information desired from that source."

> > > o  4.1
> > >
> > >     Optionally,
> > >     the <subscription-result> MAY include one or more hints on
> > >     alternative input parameters and value which would have resulted in
> > >     an accepted subscription.
> > >
> > >   I think these hints have been removed, so this sentence should also
> > >   be removed.
> >
> > These hints have not been removed.  See in the yang model
> > "subscription-response-with-hints"
> 
> See above!

Beyond Yves request for this described above in:
https://www.ietf.org/mail-archive/web/netconf/current/msg12154.html 
the need for negotiation is identified in RFC7923, section 4.2.2.   This was driven by I2RS Interim 15Dec2014

The result has been readouts NETCONF face-to-face sessions including as an element needing WG feedback in IETF 95, IETF 96,  IETF 97, IETF 98, and the NETCONF Interim 18May2016.   Materials are on the IETF site.  

Generalizing a bit, there has been quite a bit of thought on the best way to do this.  Some of the problem elements can be seen in places like:
https://github.com/netconf-wg/yang-push/wiki/Minutes-2016-05-12 

And the result that negotiation is needed for a set of use cases as described at the bottom of:
https://github.com/netconf-wg/yang-push/wiki/Minutes-2016-06-02 

While it is possible to compromise the hint based interaction model away from the request Yves makes just for replay (i.e., so that just replay doesn't meet promise theory), such an exception is not at all advisable. 

> > > o  4.1.1
> > >
> > >     The presence of a replay-start-time within an <establish-
> > >     subscription> RPC is the way a replay may be been requested.
> > >
> > >   This sentence looks a bit odd.
> >
> > Agree.  How about:
> > The inclusion of a replay-start-time within an
> > <establish-subscription> RPC indicates a replay request.
> 
> Ok.
> 
> > >     The provided start time MUST be earlier than the current time.
> > >
> > >   I know that this text is also present in RFC 5277, but I think it
> > >   needs to be changed.  Which current time?  Probably the server's,
> > >   but how would a client know that?  This is a problem that we faced
> > >   when implementing 5277.   I think we should remove this
> > >   requirement, since it doesn't add any value anyway.
> >
> > Can remove.
> 
> Ok.  But we should specify that if the replay-start-time is later than any
> notification found in the replay buffer, then the server MUST
> immediately send a "replay-completed" notification.   I.e., the client
> cannot send a replay-start-time in the future, and expect the subscription to
> start at that time.

Agree that is the intended behavior.  Will add that text.

> > >     If
> > >     the start time points earlier than the maintained history of
> > >     Publisher's event buffer, then the subscription MUST be rejected.
> > >
> > >   This sentence is also odd.  Also, why is this restriction added?
> >
> > An underlying design goal of subscribed-notifications and yang-push is
> > to deliver no less than what subscriber explicitly requested.
> > Especially when YANG-Push is layered in, if we start delivering less
> > for some combination of parameters, we have no certainty that the
> > subscriber is getting what it needs.
> >
> > For this parameter, if we start replaying more recently than what has
> > been requested, the subscriber might believe it is receiving for a
> > time-period from which events have already been purged.  As we don't
> > signal back a time when the replay actually began on success, we are
> > not delivering on the implicit promise of the subscription "ok".  This
> > is the reason the no-success result includes a
> > "replay-start-time-hint".
> 
> IMO this is unnecessary complexity.  Anyway, since this is a change of behavior
> from 5277, I think you should treat this as an open issue, and check what the
> WG thinks.
> 
> (If the result is that we do what you suggest, then please rewrite the quoted
> sentence.)

For the reasons above, I see negotiation via hints as something which has been documented and socialized.  

Farther down in this email, you will see issues I am comfortable with adding to the issues tracker.   But this one is so deeply ingrained and revisited, I don't see a purpose in going there again.  Especially as we have proof points of people who want this behavior.
 
If you want to re-open the topic as a separate issue, we should jointly work with the WG chairs on the proper procedure on how to handle something which the authors believe has been re-visited, re-analyzed, and re-closed.

> > >   RFC 5277 says:
> > >
> > >          If the <startTime> specified is
> > >          earlier than the log can support, the replay will begin with
> > >          the earliest available notification.
> > >
> > >   which allows a client to replay the entire history by specifying an
> > >   early time.  I think we should stick to the old behavior.  (Then you
> > >   can also remove the "history-unavailable" identity).
> >
> > As the old behavior can result in unknown gaps in events, the newer
> > behavior is preferable in my opinion.
> >
> > >     Not all streams will support replay.  Those that do MUST include the
> > >     <replay-supported> flag.
> > >
> > >   The last sentence lacks context.  What <replay-supported> flag?  I
> > >   think you mean "replay-support" leaf in the "/streams" list.
> >
> > Yes, will fix.
> >
> > > o  4.2
> > >
> > >     The <delete-subscription> operation permits canceling an existing
> > >     subscription previously established on that transport session.  If
> > >     the publisher accepts the request, it immediately stops sending
> > >     events for the subscription.
> > >
> > >   I think we should add "After a successful reply has been sent, the
> > >   server MUST NOT send any more notifications for this subscription.".
> > >
> > >   (same language for "modify-subscription")
> >
> > This alternate text also works.  Will update.
> >
> > > o  4.4
> > >
> > >     The <kill-subscription> operation permits an operator to end any
> > >     dynamic subscription.  A publisher MUST terminate any dynamic
> > >     subscription identified within a properly authenticated RPC
> > >     request.
> > >
> > >   What does the last sentence mean?  I suggest you clarify.
> >
> > Will clarify that this means only a system administrator should have
> > the ability to delete a subscription which they did not create.
> 
> Hmm.  What is a "system administrator" - my point is, don't introduce new
> terms/concepts; explain this in terms of NACM or just access rights.

Will use access rights.

> > >   How is a client supposed to know which subscription-id to use?
> >
> > This is for an Operator who is able to read from the subscriptions
> > container.  (There is no rw node to delete a dynamic subscription.)
> > Will add a sentence.
> 
> Ok.
> 
> >
> > > o  5.
> > >
> > >     o  One or more receiver IP addresses (and corresponding ports)
> > >        intended as the destination for push updates for each
> > >        subscription.  In addition, the transport protocol for each
> > >        destination MAY be defined.
> > >
> > >   Is seems odd that the transport protocol MAY be defined.
> > >
> > >   I see what you mean, since "protocol" has a default value, it
> > >   doesn't have to be explicitly configured.  But it is still
> > >   defined...  I would actually suggest that the default value is
> > >   removed, and "protocol" made mandatory.  Or, to also solve the issue
> > >   that Tim Jenkins reported, make the "protocol" leaf part of the key.
> >
> > I do like what Tim is suggesting.  It just makes things a little more
> > complex.  If the WG is comfortable with what Tim wants, I would be
> > very happy to make this change.  And this would fix the item you point
> > out above.
> 
> I'm not sure it will, but I'll wait for the resolution of Tim's issue and check this
> afterwards.

ok

> > > o  5.1
> > >
> > >   This section contains some obvious and redundant text and examples,
> > >   but misses to explain some important concepts.  First of all, the
> > >   first paragraph doesn't really say anything and can be removed.
> >
> > As an expert, you for sure know all this.  Others from the Network
> > Element development domain learning some of this for the first time
> > seem to find the intro text useful to help them understand.  If the WG
> > wants this removed, I can do so as it makes the document smaller
> > (which is good).  But we do lose something for newer readers.
> 
> Ok.
> 
> > >   Second, the example shows how to do an edit-config; this is not very
> > >   interesting, so I think it should be removed as well.
> >
> > Same comment as above.  We know this.  But newer people to this might
> > find value.  Especially as it shows that even configured subscriptions
> > ultimately use NETCONF RPCs.
> 
> Hmm, isn't this supposed to be protocol agnostic?  I.e., it can also be installed
> using RESTCONF etc?

This is true, it is protocol agnostic.  There is always a balance of providing examples so that readers can make the connection.
 
> > That said I wouldn't lose sleep if it is dropped.
> 
> 
> > >   But I think it instead should explain what is supposed to happen
> > >   during and after the call home.  The fact that these subscriptions
> > >   relies on call home is currently hidden at the end of a single
> > >   sentence.   This should be made much clearer, and the call flow
> > >   explained.  Once the call home has been made for NETCONF, are
> > >   "notification" elements just started to be sent?    How are
> > >   notifications sent over a RESTCONF call home - the server can't just
> > >   send a HTTP repsonse, right?
> >
> > This overall topic was moved to the transport drafts as the need and
> > approach for call home is fully dependent on the desired transport.
> 
> Then this should be explained, as a requirement on the transport specification.
> Write that a transport document MUST specify how the server calls home for
> configured subscriptions.
> 
> And, make sure that the current transport documents explain this!

+1.  I agree.

> > Call home is often not desirable with HTTP2 transport, as this impacts
> > scale, simplicity, and which side is the HTTP client or server.
> 
> I'm confused; the text clearly states that the server calls home:
> 
>    In this case, when there is something to transport
>    for an active subscription, transport protocol specific call-home
>    operations will be used to establish the connection.
>
> Are you saying that HTTP2 must not be used for configured subscriptions, since
> it doesn't work well with call home?

HTTP2 can be used with configured subscriptions.

I am adjusting the text to indicate that call-home is only for protocols needing the receiver to initiate the transport session.   In the case of HTTP2, this is not the case as the HTTP client is the publisher.
 
> 
> > >   Will the server buffer notifications during call home setup?
> >
> > Once the subscription is established via successful edit-config, or
> > system initialization, it is required that events must be sent only
> > from the subscription-started notification.  This is unless a
> > replay-start-time has been configured.  I.e., there is no buffered
> > event push unless there is a replay-start-time configured for that
> > subscription.
> >
> > This reminded me of something which is not adequately defined in the
> > existing text.  In the case a replay for a configured subscription,
> > the replay-start-time in the subscription-started notification should
> > be the latest of: replay-log-creation-time, replay-log-aged-time, or
> > replay-start-time.
> 
> Is replay really needed for configured subscriptions?  If it is specified, when will
> it be used; during the first session, or any subsequent sessions?

I didn't think it was needed for configured subscriptions at first either.  But over the last few years I have run into customers who wanted to track boot-up events which are recorded prior to the establishment of NETCONF transport.  So this becomes relevant at any boot time.

> > This description is what is supposed to be placed within the
> > "replay-start-time" of the subscription-started notification.  But I
> > didn’t refine that node with this description under the notification.
> > I will make that update in the YANG file.
> >
> > To let people know this capability is there, I will add some text to
> > section 5.1.  The net result is that this allows a configured
> > subscription to exist on a system so that events discovered during
> > boot can be buffered and pushed as soon as the call home is
> > established.
> >
> >
> > >   What happens if a transport session goes down?  Will the server
> > >   immediately try to re-establish?  What if that fails?  What happens
> > >   if the call home setup due to authentication issues, will it still
> > >   try to re-establish?
> >
> > I will clarify that if the transport fails for a configured
> > subscription, the subscription MUST be moved to a suspended state if
> > it cannot be re-established before events are dropped.
> 
> But there can be multiple receivers per subscription.  If one receiver's session
> goes down, will the entire subscription be suspended?

Configuration operations are against the set of receivers, using the subscription-id as a handle for that set.

But for streaming up dates, state change notifications are local to a receiver.    A goal of the design is that receivers get no information from the publisher about the existence of other receivers.   But if the system operator wants to let the receivers correlate results, having a common subscription-id across the receivers to allow that correlation is *very* useful.

> > And I will
> > tweak the last sentence of section 2.3 to indicate that the system may
> > automatically move configured subscriptions to/from the suspend state
> > based on the availability of transport.
> 
> So while the subscription is suspended (b/c the transport closed), the server
> will still try to re-connect?  And if that succeeds, it will automatically resume?

Yes.   And there is no need to force any replay interactions for earlier events, as from the perspective of the receiver this is just continuity of the existing subscription.

> > As for the specifics of the connection, these need to be addressed in
> > each transport draft, as it is not specific to the subscription model.
> > So for NETCONF, these topics need to be fully covered in section 3.4.2
> > of netconf-netconf-event-notifications.
> >
> > I will propose text to Alberto for that draft indicating that a
> > persistent connection is needed per RFC 8071, Section 4.1, step S7.
> > And details on attempts at re-establishment.
> 
> Ok.
> 
> > > o  7.
> > >
> > >     Once a subscription has been set up, the publisher streams
> > >     asynchronously push subscribed content via notification messages per
> > >     the terms of the subscription.
> > >
> > >   What is "asynchronously push subscribed content"?
> > >
> > >   Please reword.
> >
> > Will delete the " asynchronously push" words.  I was trying to be more
> > explicit, but the words ultimately are unnecessary.
> >
> > >      This notification message MAY be encoded as one-way notification
> > >      element of [RFC5277], Section 4.
> > >
> > >    Ok, I think I know what you mean; you want this spec to use 5277
> > >    for now, but when we define a new notification message, that one
> > >    should be used instead.  But the current text with MAY is too lax,
> > >    imo. I think it should be a MUST, and when the new notification
> > >    message document is published, it can update this document.
> >
> > Yes, this is what is intended.  I was hoping to use MAY so that we
> > didn't have to revisit this document.  I am happy to go with whatever
> > approach works for everyone.
> 
> Ok, please track this issue.

Makes sense.   I have added a new issue on this to the git repository
https://github.com/netconf-wg/rfc5277bis/issues/3 
 
> > >    Or maybe you meant that the format of the messages might vary?
> >
> > It didn't mean that.
> 
> Ok.
> 
> > >    If so, I suggest that we specify that the messages are encoded in a
> > >    transport-protocol specific manner.  For NETCONF and RESTCONF over
> > >    XML it MUST be ... (5277).   Etc.
> > >
> > >
> > >     These drawbacks, along with other useful common headers and the
> > >     ability to bundle multiple event notifications together is being
> > >     explored within [I-D.notification-messages].  When this draft is
> > >     supported, it will be possible to support the encoding of YANG
> > >     modeled notification messages with JSON via [RFC7951].
> > >
> > >   JSON is already supported for notifcations over RESTCONF, so the
> > >   last sentence should be removed.
> >
> > Yes.  But what about JSON over other transports such as NETCONF?
> 
> There is no JSON encoding for NETCONF.

Not now for sure, I am trying not to preclude such a future in certain cases.  Specifically extending the RESTCONF definition of "yang-data" so that it is transportable  over NETCONF.  And with this, and embedded anydata object that is encoded via RFC7951.   This is all future stuff for the Notification-messages draft.

> > > o  9.2
> > >
> > >     Capabilities are advertised in messages sent by each peer during
> > >     session establishment [RFC6241].  Publishers supporting the RPCs and
> > >     Notifications in this document MUST advertise the capability
> > >     "urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications:1.0".  In
> > >     addition support for optional features: json, configured-
> > >     subscriptions, and replay MAY also be advertised.
> > >
> > >   I don't think a new protocol capability is needed; the normal YANG
> > >   module advertisment is enough.  This normal module advertisment also
> > >   has the benefit of being protocol neutral; if you want to use
> > >   protocol capabilities, you need to register one per protocol with
> > >   IANA.
> >
> > Yes, this is supposed to be normal advertisement.  I was just trying
> > to educate new readers.
> 
> Ok, that would be fine with me.  But note that YANG 1.1 modules are not
> advertised as protocol capabilites, and also note that the URL above is
> incorrect.

Removed the ":1.0"

> > >     If a publisher supports this specification but not [RFC5277], the
> > >     publisher MUST support the one-way notification element of [RFC5277]
> > >     Section 4 or [I-D.notification-messages].
> > >
> > >   I think this MUST makes I-D.notification-messages a normative
> > >   reference, which probably is not what you want.
> >
> > Normative is not what I want.  I wonder if I can say:
> > "the publisher MUST support some one-way notification message defined
> > within the NETCONF WG".  Likely this won't fly with the IETF
> > reviewers, and we will need to revise the document in the future (per
> > your comment above).
> 
> Revise the document or publish a new RFC which "Updates" this one.
>

Yes.  Likely one of those.  I will update the doc based on the assumption that it get revised with the subscribed-notifications publication.
 
> In any case, I think you should track this issue.

Issue created.  
https://github.com/netconf-wg/rfc5277bis/issues/3 

> >
> > > o  10 - module meta data
> > >
> > >      module ietf-subscribed-notifications {
> > >      ...
> > >
> > >      description
> > >        "Contains a conceptual YANG specification for subscribing to events
> > >        and receiving matching content within notification
> > > messages.";
> > >
> > >   s/conceptual YANG/YANG/
> > >
> > >      revision 2017-10-02 {
> > >        description
> > >          "Filtering and stream structures updated, replay a
> > > feature.";
> > >
> > >   Change to: "Initial revision."
> >
> > Yes
> >
> > >
> > > o  10 - encoding
> > >
> > >   I don't think the "encoding" leaf is done correctly.  Each
> > >   subscription has an encoding, but the same subscription may serve
> > >   receivers with different protocols.   But since NETCONF doesn't
> > >   use JSON, this isn't really feasible.
> >
> > When we have the new notification message, I have been hoping that we
> > could support an Anydata event encoded in JSON over NETCONF.  Whether
> > this has market demand is unknown to me.
> 
> I think Andy pointed out that mixing XML and JSON in the same message
> doesn't make much sense.

Totally agree the mix isn't very clean.  At all.  That said, should it categorically be precluded?  

For example, in yang-push document there is an example of a push-change-update which follows the yang-patch JSON encoding (this is within the anydata of datastore-changes).  Likely that yang-push example should be moved to the Media Type "application/yang-patch+xml" as described in RFC8072, section-4.2.1.   But this at least show that it is possible.

> > >   Maybe the "encoding" should
> > >   be moved down to the "receiver" list?
> >
> > In earlier discussions, others in the WG didn't want to support a
> > subscription with different encodings to different receivers.
> 
> Can you point me to that discussion?

One proponent of the was Einar Nilsen-Nygaard (cc'ed).  I will let him dig up his old messages.  Basically he didn't believe developers want to create a data extract, which then would go later into different encodings.

As you can see I created an issue for this below.  In the end, I am good with any answer on this.  I just want the simplest configuration model.

> > That
> > would have been a complexity.  As different subscriptions can be
> > configured, that seemed more straight-forward.
> 
> I think that "protocol" and "encoding" should be defined together (at the same
> level).  So either move encoding down, or protocol up.
> Please track this issue for WG comments.

https://github.com/netconf-wg/rfc5277bis/issues/4 

> > It would seem better to have an implementation simply restrict what
> > could be configured for now without explicitly disallowing this
> > potential future capability
> >
> > > o  10 - features
> > >
> > >     feature configured-subscriptions {
> > >       description
> > >         "This feature indicates that management plane configuration
> > >          of subscription is supported.";
> > >     }
> > >
> > >   s/management plane //
> >
> > Will do
> >
> > >     feature replay {
> > >       description
> > >         "This feature indicates that historical event replay is
> > >         supported.  With replay, it is possible for past events to be
> > >         will be streamed in chronological order.";
> > >
> > >
> > >   s/will be//
> >
> > Will do
> >
> > > o  10 - extension
> > >
> > >      extension subscription-state-notif {
> > >
> > >   I think you should spell out notification; i.e., use
> > >   subscription-state-notification.
> > >
> > >        description
> > >          "This statement applies only to notifications. It indicates that
> > >           the notification is a subscription state notification. Therefore
> > >           it does not participate in a regular event stream and does not
> > >           need to be specifically subscribed to in order to receive
> > >           notifications.";
> > >
> > >   I suggest you write:  "This statement can only occur as a
> > >   substatement to the 'notification' statement."  Or something along
> > >   that line.
> >
> > Will do
> >
> > > o  10 - Identities for subscription results
> > >
> > >   You define identities for 'ok' and 'error'.  I think Andy already
> > >   pointed out that errors are reported in the protocol; in NETCONF
> > >   using <rpc-error> and in RESTCONF by using an approprioate HTTP
> > >   error code.
> >
> > While perhaps we could place returned information in Error App Tag
> > strings instead of returned data, declining an RPC isn't really an
> > error.
> 
> So why do you have an identity called "error"?

So that only a subset of the subscription-result identities can be sent within a subscription-terminated or subscription-suspended notification

> I don't agree with the statement that if an RPC returns "internal-error" (which
> is one of your identites) then it isn't really an error.

I have no problem deleting the internal-error identity, those could be covered by errors covered with the universe of errors associated with the transport protocol.

> > Plus often this information transported could be in one-way
> > subscription state-change notifications (like subscription-suspended),
> 
> The identities are not used in any notification.  They are only used for RPC
> replies.

See error-id in subscription-terminated and subscription-suspended.

> > so there is not a protocol construct for errors appropriate for
> > binding.  So it seemed more reasonable not to bind, and keep this info
> > within the application interactions.
> >
> > >
> > > o  10 - Identities for subscription stream status
> > >
> > >   Should subscription status really be identities?   Shouldn't these
> > >   values correspond to the state machine in section 2.3?  If so, does
> > >   that mean that the state machine is extensible?  I think this just
> > >   adds complexity, and that using an enumeration is better.
> >
> > Either way is fine with me.
> 
> Please track this issue.

It is faster for me to convert the model to an enumeration versus creating and tracking an issue in the git page.  So I will do that.

It is quite possible the IoT people will regret this simplification, but that is their problem now unless they chime in.
 
> > > o  10 - Identities for transports
> > >
> > >   Shouldn't RESTCONF be present?
> >
> > Events are only transported over HTTP2, RESTCONF is not used for
> > streaming updates.
> 
> Ok.
> 
> >
> > >   You define an identity for 'http2', but there is no reference and no
> > >   details about this transport.
> >
> > The details are in the RESTCONF HTTP2  draft.
> 
> Wait, you just wrote that RESTCONF is *not* used for streaming updates?

Absolutely.  Even in the RESTCONF draft, updates are not streamed over RESTCONF.   They are streamed via SSE.

> > >   BTW, terminology issue, you use the terms "transport",  "protocol",
> > >   and "transport protocol" for the same thing.  I suggest you pick
> > >   one.
> >
> > Will tweak to transport where there might be confusion.
> 
> Ok.
> 
> > > o  10 - filter-id
> > >
> > >   Why is "filter-id" an integer?  Since filters are configured, using
> > >   a string as identifier is preferable.  Then the operator can give
> > >   the filters meaningful names, instead of trying to remeber if it was
> > >   filter 5 or 7 that filters for bgp-related notifications.
> >
> > In discussions with various implementers, they expressed a preference
> > for integer.
> 
> This is a WG document, so in the end this is something the WG should decide.
> Why should "filters" be different from all (?) other lists we have in various
> standard models?  Almost in all places where an arbitrary identifier is needed
> and controlled by the client, we use a string.

Alright.   I will convert to string and see if anyone cares enough to object.   Either way is fine with me.

> > The ability to augment for names is available for vendor
> > implementations.
> 
> No, since the 'id' is the key.  Of course there can be an additional 'name' leaf,
> but that doesn't help.

That is what I meant.   But let's see if anyone objects per my note above.

> > > o  10 - subscription id
> > >
> > >   Same issue as for filter id; I think the subscription-id should also
> > >   be a string.
> >
> > Subscription-id are dynamically assigned by the publisher, or
> > configured.
> 
> Yes.
> 
> > Maintaining uniqueness, and keeping the push updates minimal when
> > subscription-id is contained within it is important.
> 
> Yes to the first, but no to the second.  And if the second part is important for a
> client, it can certanily choose a short string.

A short string with uniqueness is still going to be bigger.  But no matter the answer to this, I think we have enough reasons to maintain integer.

> > The ability to augment for names for configured subscriptions only is
> > available for vendor implementations.
> >
> > > o  Terminology issue:
> > >
> > >   In general, sometimes you are using the term "event" when you mean
> > >   "notification".  Sometimes you also use "notification event", which
> > >   is unclear.
> >
> > In section 7, and the YANG model, will change "event notifications" to
> > events.  Will scan and clean up elsewhere as appropriate in the yang
> > model.
> 
> I think you should *not* use the term "event" when you mean "notification".
> Your document already defines "event" as:
> 
>    Event: An occurrence of something that may be of interest. (e.g., a
>    configuration change, a fault, a change in status, crossing a
>    threshold, or an external input to the system.)
> 
> I think you should keep this.

Not sure what you are asking then.

> > > o  10 - /filters/event-filter
> > >
> > >   For consistency: s/event-filter/filter/
> > >
> > >   also fix event-filter-ref, and maybe others.
> >
> > It needs to be event-filter, not filter.  This is to differentiate
> > from selection-filter, which gets augmented in via YANG-push.
> 
> Then I think it should be "notification-filter".

Such a name could easily confuse people into thinking the subscription mechanism defined in this document can only be applied to notifications as defined in RFC7950.  This is not the case, as custom streams of non-YANG events can also be subscribed.   In other words, even though we will no longer support an IETF defined SYSLOG stream, there is absolutely no reason why this subscription mechanism cannot be used by a vendor to do this (i.e., via a custom stream).  So this should stay event-filter.

> > > o  10 - filters
> > >
> > >   I think you should copy section 5 from RFC 5277, but change it to
> > >   use YANG, since eventually RFC 5277 will be obsoleted.
> > >
> > >
> > >       anydata subtree-filter {
> > >         description
> > >
> > >
> > >   OLD:
> > >
> > >           "Event stream evaluation criteria encoded in a syntax of a
> > >            supported type of an RFC 6241, Section 6 filter.  When
> > >            applied against an event stream and there is a non-empty or
> > >            positive result, the event is passed along.";
> > >
> > >   NEW:
> > >
> > >           "Event stream evaluation criteria encoded in the syntax of
> > >            a subtree filter as defined in RFC 6241, Section 6.
> > >
> > >            The subtree filter is applied to the contents of the
> > >            notification message, i.e., the top-level element is the
> > >            element representing the notification's name.
> >
> > What if the subtree filter is applied against something other than a
> > notification message?  YANG-Data comes to mind.
> 
> I don't understand.  All messages being sent in streams are notification
> messages, right?

No.    

While it is true that anything using the subtree filter is likely YANG encoded, that need not be true for a stream of events from IoT devices against which xpath might be applied.

> > I will make this text
> > change, but will add for example at the start of the paragraph above .
> 
> But that doesn't help!  This is normative text and it must be clear what the
> filter applies to, otherwise it will be impossible to have interoperable
> implementations.

The normative text here is that for the NETCONF stream, a subtree filter RFC 6241, Section 6.  Might be applied.  There is no reason to prohibit xpath from being applied to other streams, even though we might not normatively describe how this is done within the document.

> > >            If the subtree filter returns a non-empty node set, the
> > >            filter matches the notifcation, and the notification is
> > >            sent to the receivers.";
> > >
> > >
> > >         reference "RFC 6241, Section 6.";
> > >       }
> > >       leaf xpath-filter {
> > >         type yang:xpath1.0;
> > >         description
> > >
> > > OLD:
> > >
> > >           "Event stream evaluation criteria encoded in a syntax of xpath
> > >           1.0  When applied against an event stream and there is a
> > >           non-empty or positive result, the event is passed along.";
> > >
> > > NEW:
> > >
> > >           "Event stream evaluation criteria encoded in the syntax of
> > >            an XPath 1.0 expression.
> > >
> > >            The result of the XPath expression is converted to a
> > >            boolean value using the standard XPath 1.0 rules.  If the
> > >            boolean value is 'true', the filter matches the
> > >            notification, and the notification is sent to the
> > >            receivers.
> >
> > This misses the valid xpath case of an xpath which is true if a
> > non-empty result is returned.
> 
> No!  Please make sure you understand how the standard XPath 1.0 rules for
> converting different types to boolean works.

What I have been looking at is:
https://www.w3.org/TR/1999/REC-xpath-19991116/#section-Boolean-Functions
specifically: The boolean function converts its argument to a boolean as follows:...a node-set is true if and only if it is non-empty

What I missed in your earlier text is that what do you get if take a boolean xpath expression already framed on a node set, and apply another boolean function on that.  The result would come out fine.  So you text works.  

My text was just trying to show that both the boolean results and node set outputs are fine.   So we have the same result.   But I will use your text as it seems to resonate more normatively for you.

> > Will add that back in
> 
> Please don't do that.  Maybe my text above can be improved.  Please check
> with some other XPath expert.

Will check again with Cisco developers currently writing xpath filters for yang-push.

> > , and use the rest
> > of the text for the definition.
> 
> 
> > >            The XPath expression is evaluated in the following context:
> > >
> > >              o  The set of namespace declarations are those in scope on
> > >                 the leaf element where this type is used.
> > >
> > >              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 in the data
> > > tree.";
> > >
> > >         reference
> > >           "http://www.w3.org/TR/1999/REC-xpath-19991116
> > >            RFC 7950, Section 10.";
> > >       }
> > >
> > >
> > >     grouping event-filter-elements {
> > >       description
> > >         "This grouping defines the base for filters for notification
> > >          events.";
> > >       choice filter-spec {
> > >         description
> > >           "The content filter specification for this request.";
> > >
> > >   I think you should add some text that explains that this is a choice
> > >   so that other filters can added via augmentation.  Such a filter
> > >   must explain how it is evaluated and how its result is interpreted.
> >
> > Will do
> >
> > > o  10 - typedef egressing-from
> > >
> > >   This typedef is not used, and should be removed.
> > >   (some descriptions in this grouping are not very clear, but it
> > >   doesn't matter if it is removed.)
> >
> > Will delete
> >
> > > o  10 - grouping notification-origin-info
> > >
> > >   Here's an example of when the langauge can be tightened a bit.  The
> > >   descriptions in this grouping uses the terms "origin", "sender
> > >   source", "egress interface", "push source" for what I *think* is the
> > >   same thing.
> >
> > The one which is redundant is "push-source".  I will change
> > push-source to origin.  The others are specific types of origins, I
> > will attempt to consolidate text.
> >
> > Beyond that, I likely should change the word "notification" below to
> > "message" in this grouping.
> 
> I don't think you should do that.  That would imply that there are other types
> of messages than notifications that can be sent.

And there are.   We are not defining these normatively in the document.  But we are not excluding this possibility.
 
> > That would bring it into alignment with the terms now emerging in the
> > notification-messages draft.
> >
> > >     grouping notification-origin-info {
> > >       description
> > >         "Defines the sender source from which notifications for a
> > >          configured subscription are sent.";
> > >       choice notification-origin {
> > >         description
> > >           "Identifies the egress interface on the Publisher from which
> > >            notifications will or are being sent.";
> > >         case interface-originated {
> > >           description
> > >             "When the push source is out of an interface on the
> > >              Publisher established via static configuration.";
> > >           leaf source-interface {
> > >             type if:interface-ref;
> > >             description
> > >               "References the interface for notifications.";
> > >           }
> > >         }
> > >         case address-originated {
> > >           description
> > >             "When the push source is out of an IP address on the
> > >              Publisher established via static configuration.";
> > >           leaf source-vrf {
> > >             type string;
> > >             description
> > >               "Network instance name for the VRF.  This could also have
> > >               been a leafref to draft-ietf-rtgwg-ni-model, but that model
> > >               is not complete, and might not be implemented on a
> > > box.";
> > >
> > >   I think that you really need to use a reference to the NI model.
> > >   Alternatively remove this case for now, and add it later.
> >
> > I know of implementations which use source VRF, but don't plan on
> > implementing draft-ietf-rtgwg-ni-model.  So implementations are free
> > to make that augmentation if they choose.
> 
> I would do it the other way around - the standard model should reference
> other standard models when possible, and if an implementation uses its own
> mechanism, then they should use an augmentation for that.  Please track this.
> Hopefully someone like Lou Berger can comment as well.

I have no issues with the ietf-network-instance model.  But if I put it in, it becomes normative.   And this includes that draft's dependency on schema-mount.

There is a lot of dependencies here for implementers looking simply for a source-VRF.  But I will track.
https://github.com/netconf-wg/rfc5277bis/issues/5 

Eric

> > >           leaf source-address {
> > >             type inet:ip-address-no-zone;
> > >             description
> > >               "The source address for the notifications.  If a source VRF
> > >               exists, but this object doesn't, a default address for the
> > >               VRF can be used.";
> > >
> > >   "a default address for the VRF can be used."  - Please rephrase.  I
> > >   think you mean something like "the publisher is free to pick a
> > >   suitable source address".
> >
> > What I mean is that there is already a default egress address which
> > exists for a VRF.  Use that one.  Will rephrase.
> >
> >
> > >
> > >
> > > draft-ietf-netconf-yang-push-10
> > > -------------------------------
> > >
> > > I haven't had time to review this one yet.
> >
> > Again, thank you for the many good comments.  They are indeed
> > appreciated, and make the document better.  By Monday I should have a
> > version I will place on the NETCONF github which can be used as a
> > reference point for the issues above agreed to.  Where you still see
> > issues, we can continue to work those through.
> 
> 
> 
> /martin