Re: [Netconf] mbj's WGLC review of subscribed-notifications-16

[Martin Bjorklund <[e-mail]>]

Hi,

Thank you for addressing my comments.  More inline.


"Eric Voit (evoit)" <[e-mail]> wrote:
> Hi Martin,
> 
> Much incorporated.  Latest is at:
> https://github.com/netconf-wg/rfc5277bis/blob/master/draft-ietf-netconf-subscribed-notifications-17.txt
> 
> Specific responses in-line...
> 
> > From: Martin Bjorklund, September 12, 2018 5:35 AM
> > 
> > "Eric Voit (evoit)" <[e-mail]> wrote:
> > > Thanks for the comments Martin,  thoughts in-line...
> > >
> > > > From: Martin Bjorklund, September 10, 2018 1:42 PM
> > 
> > [...]
> > 
> > > > o  Section 2.1
> > > >
> > > >   The text says:
> > > >
> > > >     It is out of the scope of this document
> > > >     to identify [...] b) how event records are defined
> > > >
> > > >   Is this really what we want?  Shouldn't this document specify that
> > > >   an event record is an instance of a YANG "notification" statement?
> > > >   Do we want to support event records that are not defined in YANG?
> > > >
> > > >   This was discussed in the thread "comments on
> > > >   draft-ietf-netconf-subscribed-notifications-12", and it seems that
> > > >   noone really have any use case for non-YANG-defined event records,
> > > >   accessed with YANG-defined operations.
> > >
> > > Yes this was discussed previously.  My reading was that others wanted
> > > to support subscription to non-YANG events.
> > > E.g., see Andy & Tianran's thoughts in
> > > https://www.ietf.org/mail-archive/web/netconf/current/msg14764.html
> > 
> > Yes, but read on in that thread, see
> > https://www.ietf.org/mail-archive/web/netconf/current/msg14766.html
> > for a clarification.
> > 
> > AFAIK, noone has presented a use case for sending non-YANG-defined
> > data
> > here.
> 
> As anything can be YANG wrapped, per the thread above.  I am fine with
> this.
> 
> > > >   I think that this document should state that an event record is an
> > > >   instance of a YANG "notification" statement.  If not, it is unclear
> > > >   both how subtree/xpath filters and xml/json/... encodings would work.
> > >
> > > Applying xpath filters against XML based event records seems viable to
> > > me.
> > 
> > Where does it say that it has to be XML?  Whatabout mixed-content?
> > What if such a notification is sent when the encoding is JSON?
> > 
> > If you want to make this specification open for other data modeling
> > languages,
> > I think you need to carefully define how that is supposed to work;
> > essentially
> > similar to how we handle transports; we have a set of requirement on
> > what a
> > transport document must define.  IMO this is super complicated and
> > overkill.
> > This draft is complex enough.
> > Don't add even more complexity.
> 
> If you are just saying that we can put everything into a YANG wrapper,
> that is fine with me.  To clarify this, I have tweaked the first
> sentence of Section 2.1 to:
> "An event stream is a named entity on a publisher which exposes a
> continuously updating set of YANG encoded event records."

Howabout:

OLD:

   An event stream is a named entity on a publisher which exposes a
   continuously updating set of event records.  Each event stream is
   available for subscription.  It is out of the scope of this document
   to identify a) how streams are defined (other than the NETCONF
   stream), b) how event records are defined/generated, and c) how event
   records are assigned to streams.

NEW:

   An event stream is a named entity on a publisher which exposes a
   continuously updating set of event records.  An event record is
   defined with the "notification" YANG statement.  Each event stream is
   available for subscription.  It is out of the scope of this document
   to identify a) how streams are defined (other than the NETCONF
   stream), b) how event records are generated, and c) how event
   records are assigned to streams.

> And I have tweaked the first sentence of the third paragraph to:
> "As YANG encoded event records are created by a system,..."

I think the original sentence is better.

Also, some tweaks are needed to the filter leafs in the YANG module:

OLD:

          The subtree filter is applied to the representation of
          individual, delineated event records as contained within the
          event stream.  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 YANG notification.

NEW:

          The subtree filter is applied to the representation of
          individual, delineated event records as contained within the
          event stream.  The top-level element in the event record is
          the name of the YANG notification.

and

OLD:

           The XPath expression is evaluated on the representation of
           individual, delineated event records as contained within
           the event stream.  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 YANG
           notification, and the root node has this top-level element
           as the only child.

NEW:


           The XPath expression is evaluated on the representation of
           individual, delineated event records as contained within
           the event stream.  The top-level element in the event
           record is the name of the YANG notification, and the root
           node has this top-level element as the only child.


(I think it would be useful with some filtering examples, like in
section 5 of RFC 5277, but I think you have all examples in
netconf-notif, so they would go into that draft.  (re-reading an old
thread I found that examples for filters were requested before)).



> > > Of course it will be up to the definition of any particular stream to
> > > articulate the event record contents sufficiently for filters to be
> > > created.
> > >
> > > > o  Section 2.1
> > > >
> > > >   The text says:
> > > >
> > > >     Among
> > > >     these included NETCONF XML event records are individual YANG 1.1
> > > >     notifications described in section 7.16 of [RFC7950].
> > > >
> > > >   First of all, does this mean that there can be event records on the
> > > >   "NETCONF" stream that are *not* defined with a "notification"
> > > >   statement?
> > >
> > > For the NETCONF stream, personally I only care about things YANG
> > > notification statement.  But looking again at Andy's comment from:
> > > https://www.ietf.org/mail-archive/web/netconf/current/msg14764.html
> > > he says "I would say SHOULD be defined in YANG, for the "NETCONF"
> > > stream."
> > 
> > I strongly disagree.
> > 
> > > So for -v16 I updated the text in a way that doesn't preclude this.
> > 
> > See above.  If all events records are defined in YANG we can remove
> > this.
> 
> Per above, I have included text above so that YANG encapsulation is
> required.  I think YANG Data should be an allowable structure (so I
> didn't specify just a "Notification").  If you think that YANG Data
> should be excluded from the allowed event records. I can refine the
> words.

Yes I think yang-data must be excluded (actually this was one of the
concerns, raised by Andy I think; that people start to use yang-data
for things that we already have proper YANG statements for...)

> > > >   Second, this seems to imply that *all* notifications defined in all
> > > >   YANG modules end up in the "NETCONF" stream, but I don't think this
> > > >   is the intention.  At least I hope so.  ("yp:push-update" is an
> > > >   example of a notification that will not be sent on the "NETCONF"
> > > >   stream.)
> > >
> > > The NETCONF stream has a very broad/inclusive definition in RFC-5277.
> > > This definition is: "This stream contains all NETCONF XML event
> > > notifications supported by the NETCONF server".
> > 
> > This is very underspecified.  IIRC the original intention was
> > notifications related
> > to NETCONF, hence "*NETCONF* XML event notifications".
> > 
> > > To me, thinking about how that statement and applying it to YANG, this
> > > translates as YANG 1.1 notifications should go into the NETCONF
> > > stream, unless explicitly excluded.
> > 
> > I disagree.  This is not how 5277 works.  Until we have worked out a
> > useful way
> > to assign notifications to streams, we shouldn't restrict anything in
> > this regard.
> 
> Ok.  Effectively we are naming the stream NETCONF without improving
> the definition of what membership in that stream should be.  And that
> is fine.  As people seem to understand what this is supposed to be, I
> will return to the original definition.  See text, and let me know if
> you have issues.

Maybe one small tweak from the original text:

OLD:

  The "NETCONF" event stream contains all NETCONF XML event
  record information supported by the publisher

NEW:

  The "NETCONF" event stream contains all NETCONF event
  record information supported by the publisher


(since it actually is possible to get them in JSON over RESTCONF)


> > > This provides a very simple way
> > > determining which types of notifications should be placed on the
> > > stream.  (Otherwise, how is a developer of a YANG notification
> > > supposed to know whether it should go into the NETCONF stream or not?
> > 
> > They don't.  But note that it might be an operator decision for how to
> > assign
> > notifications to streams.
> 
> Ok.  A stream membership definition is out-of-scope for all other
> streams, we can leave it this way for the "NETCONF" stream.  Someone
> else can pick up and define this in another draft the WG feels it
> necessary.

Yes.

> > > The alternative would seem to be having each notification opt-in to
> > > the NETCONF stream.)
> > >
> > > So if you agree with this reasoning, how about:
> > >
> > > The "NETCONF" event stream contains all instances of YANG 1.1
> > > notifications generated by a publisher, except where a type of
> > > notification has explicitly been excluded from the stream.
> > 
> > I think that this document should keep the definition of the NETCONF
> > stream as
> > defined in RFC 5277.
> 
> Ok
> 
> > Also, I think it should state that an event record is an instantiation
> > of a YANG
> > "notification" statement (any version of YANG).
> 
> What about a YANG Data?  (Per above)

See above.

> > [...]
> > 
> > > > o  Section 2.4.2
> > > >
> > > >   The text says:
> > > >
> > > >     The transport selected by the subscriber
> > > >     to reach the publisher MUST be able to support multiple "establish-
> > > >     subscription" requests made within the same transport session.
> > > >
> > > >   This seems to indicate that RESTCONF cannot be used with this
> > > >   specification, since RESTCONF doesn't have sessions.
> > >
> > > The desired behavior is that a subscriber can request multiple
> > > subscriptions without each requiring a separate transport session
> > > establishment.  I think we are ok here with RESTCONF because
> > > underneath RESTCONF is HTTP, TLS, TCP.  And each of those lower layers
> > > can have sessions.
> > >
> > > To clarify further, I have tweaked the text to:
> > >
> > > "The transport selected by the subscriber to reach the publisher MUST
> > > be able to support multiple "establish-subscription" requests without
> > > each requiring a separate persistent transport connection."
> > >
> > > >   This makes me wonder, why does this document specify this
> > > >   requirement?  I agree that this is the behaviour we want from
> > > >   NETCONF, but then I think that should be defined in the
> > > >   netconf-notif draft.
> > >
> > > By saying something in SN about this requirement, I believe we can
> > > preclude an implementation which can only support one subscription per
> > > subscriber.  In any case, the specific "how" details will still need
> > > to be articulated in each transport draft.
> > 
> > I think you are mixing the specification and implementation.  If a
> > transport
> > document says that an implemention MUST support multiple subscriptions
> > per
> > session (in a transport-specific way), then an implementation cannot
> > just
> > support a sinlge subscription.
> > 
> > My point is that this a transport-specific requirement, and it doesn't
> > belong in
> > this document.
> 
> I can see why you don't see it applicable to Section 2.4.2.  However
> Section 5.3 provides guidelines for transport document developers
> (this is a section Kent requested.)  And so I have moved the
> requirement there.  Is that sufficient.

Ok, I agree that the text belongs to section 5.3 rather than 2.4.2.  I
just don't agree with the text ;-)  But I can accept it.


> > > > o  Section 2.4.2
> > > >
> > > >   s/"start-time"/"replay-start-time"/  (3 occurrences)
> > >
> > > Fixed
> > >
> > > >   Also, the text says that the replay-start-time MUST be in the past.
> > > >
> > > >   Suppose the server receives a "replay-start-time" of 12:00:00.001,
> > > >   and the replay buffer is empty.
> > > >
> > > >   A) If the current time is 12:00:00.002, the request is accepted, and
> > > >      a "replay-completed" notif is sent.
> > > >
> > > >   B) If the current time is 12:00:00.000, the request is rejected with
> > > >      an error.
> > > >
> > > >   Why is this important?  Why not simply just send the
> > > >   replay-completed notif in both cases?
> > >
> > > For (B), if an event occurs at 12:00:00.0005, should it be sent?
> > > Should it be sent before or after the "replay-start-time"?  How long
> > > should we allow the clocks to drift before we reject the subscription?
> > > As we don't support future/pending subscriptions, we don't have to
> > > worry about these questions with the current definition.
> > 
> > I would simply say that if the replay-start-time is later than any
> > buffered notif,
> > then "replay-complete" is immediately sent.  It dont'
> > matter if it is one microsecond or one year later.
> 
> Tweaked the sentence:
> 
> "If the given "replay-start-time" is later than the time marked within
> any event records retained within the replay buffer, then the
> publisher MUST send a "replay-completed" notification immediately
> after a successful establish-subscription RPC response."
>  
> > The problem otherwise is how to decide if something is "in the past".
> > In the past when the client sent the request?  When the server parsed
> > the XML?
> > When the instrumentation code runs?
> > 
> > But this is not a big issue.
> 
> Hopefully with the tweak, we can leave it as modified.

Ok.


> [..]
> > > > o  Section 2.4.6
> > > >
> > > >   The text says:
> > > >
> > > >     1.  "establish-subscription-stream-error-info": This MUST be returned
> > > >         if an RPC error reason has not been placed elsewhere within the
> > > >         transport portion of a failed "establish-subscription" RPC
> > > >         response.
> > > >
> > > >   (and similar for the other errors)
> > > >
> > > >   What exactly does "if an RPC error reason has not been placed
> > > >   elsewhere" mean?
> > >
> > > The intent is to match how NETCONF & RESTCONF historically handles
> > > errors.  As you know with NETCONF, each error identity will be
> > > inserted as the "error-app-tag".
> > 
> > It could also be in "error-tag" and/or "error-message".  My point is
> > that I don't
> > think that you mean that if an implementation populate say
> > "error-message",
> > then it should not popluate error-info with
> > establish-subscription-stream-
> > error-info.
> 
> I do mean that.  I don't think we should send:
> "establish-subscription-stream-error-info" for NETCONF when there is
> no hint included.  All the necessary error info is already in the
> existing message.

But is this really true?  In the
"establish-subscription-stream-error-info" there is a leaf called
"reason", which is an identity with a detailed error reason, for
example "history-unavailable".  This info is not present anywhere else
in the rpc-error, so it needs to be present here.


> > > And therefore placing the YANG data
> > > "establish-subscription-stream-error-info" would be redundant.  For
> > > NETCONF, "establish-subscription-stream-error-info" are only needed
> > > when hints to be provided.
> > >
> > > Therefore assuming you are looking for an alternative way to make this
> > > point, I have tweaked the text to:
> > > "This MUST be returned if an RPC error reason has not been mapped down
> > > into transport specific error objects contained within a failed
> > > "establish-subscription" RPC response."
> > 
> > Howabout simply removing this sentence?
> 
> Done
> 
> > > > o  Section 2.5.1
> > > >
> > > >   In the second diagram (for the valid state), the states are called
> > > >   "receiver connecting", "reciever timeout" etc.  I find this a bit
> > > >   confusing; it is not the *receiver* that is connecting, it is the
> > > >   publisher.  Ok, the legend then says:
> > > >
> > > >      dashed boxes which include the word 'receiver' show the possible
> > > >      states for an individual receiver
> > > >
> > > >  But since all dashed boxes include the word "receiver", and the
> > > > diagram specifically is the state machine *per receiver*, maybe we
> > > > can simply remove the word "receiver" from the boxes?
> > >
> > > There have been some discussions on this.  I agree it is certainly
> > > valid to remove "receiver" from these four boxes.  However I have
> > > found that getting readers to really internalize that the state
> > > machine is actually per-receiver rather than per-subscription, it
> > > seems to go faster/cleaner when receiver is shown in the four boxes.
> > >
> > > Nevertheless, if you absolutely want "receiver" removed from the
> > > boxes, I can do that.
> > 
> > Maybe you can remove receiver from the individual boxes and then
> > change the
> > word "valid" from the outer box to "per-receiver state machine"?
> > 
> > Or maybe remove the outer box and add a Figure title: 'per-receiver
> > state
> > machine for the subscription's "valid" state'
> > 
> > Also not a big deal, but I found it confusing.
> 
> All are viable.  But I have found the current diagram useable. Do you
> have a major issue if we leave it as it is?

No.  As I said, I found it confusing, but if others think it is more
helpful without my suggested changes, we should leave it as is.

> > [...]
> > 
> > > > o  Section 2.5.6
> > > >
> > > >   I know that this feature was presented in Montreal.  I think it is
> > > >   problematic, and suggest it is removed.
> > >
> > > This issue was discussed in Montreal.  The feature is of course is
> > > optional.  There are a large number of downsides for someone who
> > > chooses not to implement for certain use cases.  Please see the
> > > Montreal slides and the long threads with Kent for more details.
> > 
> > I did watch the Montreal session, but I didn't see any discussion on
> > this issue (just your presentation).   Can you send a pointer to the
> > email thread with Kent?
> 
> Some minutes on the topic can be seen at:
> https://datatracker.ietf.org/doc/minutes-102-netconf/ 
> With the WG vote in Montreal, I would love not to re-open.  
> 
> The previous exchange was long.  Some items include:
> https://www.ietf.org/mail-archive/web/netconf/current/msg14575.html
> https://www.ietf.org/mail-archive/web/netconf/current/msg14902.html 

Ok, if you believe that this thread shows that the WG supports this
feature, I won't pursue this.

> > > If
> > > you want to pursue this further it is probably best to start a new
> > > thread dedicated to this topic alone.
> > >
> > > >   For example, what happens if the transport session is lost and
> > > >   re-established; are all event records in the buffer replayed again?
> > >
> > > No.  The publisher will not lose context of what was sent.
> > 
> > This is not clear from the text.  In fact, the text explicitly says:
> > 
> >    The leading event record sent will be the first event record
> >    subsequent to the latest of three different times: the
> >    "replay-log-creation-time", "replay-log-aged- time", or the most
> >    recent publisher boot time.
> 
> I see your point.   I have tweaked the text to:
> 
> " If the publisher does not know the next record intended for the
> receiver, the leading event record..."

Hmm, not sure this helps.  How about:

OLD:

   As a result, when any configured subscription receivers become
   active, buffered event records will be sent immediately after the
   "subscription-started" notification.  If the publisher does not know
   the next record intended for the receiver, the leading event record
   sent will be the first event record subsequent to the latest of three
   different times: the "replay-log-creation-time", "replay-log-aged-
   time", or the most recent publisher boot time.

NEW:

   As a result, when any configured subscription receivers become
   active, buffered event records will be sent immediately after the
   "subscription-started" notification.  The leading event record
   sent will be the first event record subsequent to the latest of four
   different times: the "replay-log-creation-time", "replay-log-aged-
   time", the most recent publisher boot time, and the time of the
   last event record sent to the reciever, if any.


> I have also made a corresponding tweak to the YANG model.

In the leaf "configured-replay"?  I couldn't find this text.

> > > >   What happens if a new receiver is added to a subscription that
> > > >   already has a receiver, will all events in the buffer be replayed to
> > > >   the new receiver?
> > >
> > > Yes.  (To clarify, I added "to each configured receiver" at the end of
> > > the last sentence of the first paragraph.)
> > >
> > > >   When a new subscription is configured with configured-replay set,
> > > >   the publisher will immediately try to connect to the publisher and
> > > >   send all buffered event from the last reboot.  Is this really
> > > >   useful?
> > >
> > > Yes.  See the security cases reviewed on the earlier threads with
> > > Kent.  If someone doesn't care about log entries since boot, they
> > > simply don't have to implement this optional feature.
> > >
> > > >   If the mechansim relies on the subscriber being able to also use
> > > >   dynamic subscriptions for replay of all event records, maybe we
> > > >   could simplify things by always relying on dynamic subscriptions for
> > > >   replay for configured subscriptions.  Specifically, remove
> > > >   "configured-replay" etc, but keep the "replay-previous-event-time"
> > > >   in the "subscription-started" notification.  Then a subscriber can
> > > >   simply check this value to see if anything is missing, and if it is,
> > > >   use a dynamic subscription to request replay from the latest time he
> > > >   knows about.
> > >
> > > This is a valid approach, and can be used as an alternative to this
> > > optional feature.  However there are still many downsides to the
> > > alternative.  See the slides from Montreal, as well as the previous
> > > thread discussions for details.
> > >
> > > > o  Section 2.6
> > > >
> > > >   It just occurred to me that this text about using the <notification>
> > > >   message shouldn't be in this document, but in the netconf-notif (and
> > > >   restconf-notif) document.  It is highly transport and encoding
> > > >   specific.
> > >
> > > I think you are referring to both the sentence: "This notification
> > > message MUST be encoded in a <notification> message as defined within
> > > [RFC5277], Section 4.  And per [RFC5277]'s "eventTime" object
> > > definition, the "eventTime" is populated with the event occurrence
> > > time."  As well as to the example compliant message in Figure 10.
> > >
> > > While the text could be extracted and placed in the netconf-notif
> > > document, extracting it fully from SN would leave no baseline message
> > > format.
> > 
> > Things like this are inevitable when you have a transport-independent
> > specification.  Of course, you can require that all transports MUST
> > use the 5277
> > spec of a notification.  But then I think you rule out transports like
> > CoMI and
> > possibly other future binary protocols.
> 
> Ok, I am fine with the move.  I have moved the identified text to
> NETCONF-Notif.  (Actually the text was already there too.  So I just
> tweaked it.)
>  
> Thanks,
> Eric
> 
> > > Perhaps others will have a problem if the SN solution does not have
> > > some pre-defined structure to hold event records?
> > >
> > > I can move this if I hear no objection.  As you know, I am hoping that
> > > draft-ietf-netconf-notification-messages at some point displaces the
> > > <notification> message of [RFC5277], Section 4.  And this is easier to
> > > do with fewer references to RFC5277 within SN.
> > 
> > [...]
> > /martin
> 


/martin