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

[Martin Bjorklund <[e-mail]>]

Hi,

[trimming resolved issues]


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

Ok, thank you.

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

No.  I'm saying that the input parameter "stream" is NOT a continuous
feed of asynchronous information - it is a reference to the *name* of
a stream (which is 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.

But why should the reader have to make this match or translation?  Why
not be specific and write that this is the name of a stream?

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

I would think that the concept of a stream is so central so that you
don't have to explain it every time it is used.

My personal opinion is that this stylistic choice makes the text
harder to read, but again, that is my opinion.  The most important
thing here is that the text is correct.


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

I disagree that Section 7 extensively covers the relationship to
5277.  Also, it is quite common that an RFC that Updates (or
obsoletes) another RFC provides a summary of those changes in a
separate section.  I suggest that you add such a section and explain:

  o  That the data model in this draft replaces the data model in RFC
     5277.
  o  That the RPC operations in this draft replaces the RPC operations
     in RFC 5277.
  o  That the <notification> message defined in RFC 5277 is still
     used.
  o  That a server MAY implement both the data model and RPCs defined
     in RFC 5277 and this new documnent at the same time, in order to
     support old clients.

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

Then I suggest

NEW:

  While the functionality defined in this document is transport-
  agnostic, protocols like NETCONF [RFC6241] or RESTCONF [RFC8040] can
  be used to configure subscriptions, and there are bindings defined
  for notification message delivery for NETCONF
  [I-D.ietf-netconf-event-notif] and HTTP2
  [I-D.ietf-netconf-restconf-notif].

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

Ok.

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

Thank you for the reference.  Reading his email, I agree that this
makes sense.  I am ok with the new behavior for start-time handling.

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

How about this:

OLD:

   A publisher implementation MUST support the ability to perform
   filtering of events within a stream.  Two filtering syntaxes
   supported are [XPATH] and subtree [RFC6241].  The subsets of these
   filtering syntaxes supported are left to each implementation.  Events
   which evaluate to "true", or return a non-null selection as a result
   of the evaluation by the event filter MUST traverse the filter in
   their entirety.  A subset of information is never stripped from
   within the event.

NEW:

   This document defines an extensible filtering mechansim.  Two
   filter syntaxes are defined in this document, based on subtree
   filtering [RFC 6241] and XPath [XPATH].  A publisher implementation
   MUST support both these filter syntaxes.


Then the specific text about these syntaxes explain hoe to evaluate
and interpret the result of these filters.

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

Please do.  Meanwhile, I think you should track this issue as well.

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

Ok.

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

Ok.

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

Ok.

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

Ok.


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

Fine.  I suggest:

OLD:

  If
  the start time points earlier than the maintained history of
  Publisher's event buffer, then the subscription MUST be rejected.

NEW:

  If the "replay-start-time" contains a value that is earlier than the
  "eventTime" of all stored notification messages in the Publisher's
  replay buffer, then the subscription MUST be rejected, and the leaf
  "replay-start-time-hint" MUST be set in the reply.


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

So how would this be done?  The box boots w/o any configuration.  Then
someone configures a subscription and configures a replay-start-time
that has a value that MUST match one of the notification messages in
the replay buffer, per the "promise theory" discussion above.  How
will the client know which start time to use in this case?

Once this is done, the server calls home, and does the replay.  Now,
let's assume the box reboots again.  This time it already has
a configured subscription, with a configured start-time, so it calls
home and does a replay from the original start time again?  This would
lead to duplicate messages.



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

Aha, the type "subscription-status" is not used for the status of the
subscription, but for the status of the receiver (within the
subscription).

You have a nice diagram for dynamic subscriptions in section 2.3, and
then you write:

   An equivalent state machine exists for configured subscriptions.

I was confused by this picture b/c I thought that the "suspended"
state was for the *subscription* - which it is in the dynamic case.

I suggest you include this state machine as a diagram as well, since
it is a bit different, due to multiple receivers.



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

Ok.

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

Well, the text is more specific than that, it actually postulates that

  When this draft is
  supported, it will be possible to support the encoding of YANG
  modeled notification messages with JSON

I still think you should remove that sentence.

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

That's not enough!  There is no capability to be advertised.  The
module "ietf-subscribed-notifications" and the features supported is
listed in the YANG library in the server.


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

Yes.  Keep it simple.  Don't include unnecessay features.  We can
always add such features later.

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

Ok.

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

Ok.

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

Ok.


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

I am confused.  It's like saying that requests are not sent over
RESTCONF, they sent over HTTP (or even TCP).

Is it really correct to talk about 'http2' as a separate protocol from
RESTCONF?  It seems to me that 'http2' isn't used on its own, but as
one transport within RESTCONF.

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

I disagree.  What are those reasons?  Please track this issue to make
it easier for the WG to follow and comment.


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

You already have a definition for "event" and another for
"notification message".  These are good, I like them.  "event" is the
occurance and "notification" is the information about the "event".

But you should make sure they are used consistently in the text.

For example "notification event" should be "notification".

And "event-filter" sounds like it filters the occurances, but in fact
it filters the messages, i.e., the "notifications".

Also, I think you use the terms "notification message",
"notification", and "event notification" for the same thing.  This
might be ok, but then you should define all three in the Terminology
section.



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

You already define "notification" as the set of information about the
event, so this is as clear as you can make it.  If people confuse it
with another term, we can't do much about it.   Except of course use
another term than "notification" for the set of information about the
event.



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

See the terminology discssion above.

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

Maybe just use:

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.
         
Possibly add:

         For example, if the notification message contains an instance
         of a notification defined in YANG, then the top-level element
         is the name of the notification.


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

Maybe Lada can comment on this one?  (if he is still reading... :)

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

See the terminology discussion above.

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

Yes, agree, but I think it is unavoidable.




/martin




> 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