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

[Martin Bjorklund <[e-mail]>]

Hi,

"Eric Voit (evoit)" <[e-mail]> wrote:
> Hi Martin,
> 
> Thanks for the suggestions.   In-line...
> 
> > From: Martin Bjorklund, November 15, 2017 2:33 PM
> > 
> > Hi,
> > 
> > I have reviewed draft-ietf-netconf-subscribed-notifications-07,
> > specifically to ensure that my earlier comments are addressed.  Here are my
> > new comments:
> > 
> > 
> > 
> > o  General
> > 
> >   The document uses the term "RPC" to refer to the operations that
> >   affect dynamic subscriptions.  I think that this term is not quite
> >   correct.  For example the text says:
> > 
> >       a configured subscription cannot be modified or deleted using
> >       RPC.
> > 
> >   But this is not correct, I can delete a configured subscription with
> >   the <edit-config> rpc.
> 
> Will append the clarification:
> "using RPCs from the document."
> 
> > o  Section 1.2
> > 
> >   What is "RPC subscription state signaling messages"?
> 
> Will change "signaling messages" to  "Notifications".   In this case the notifications act as signaling messages.
> 
> > o  Section 2.2
> > 
> >      The subsets of these filtering syntaxes supported are
> >      left to each implementation.
> > 
> >    Juergen has already pointed out that this is not good design - how
> >    will the client know which subset of subtree filtering a certain
> >    device supports?   I think that if the server doesn't support
> >    subtree filtering as defined, then it should not advertise the
> >    "subtree" feature.  It can always advertise another feature.
> 
> Agree,  that was always the intent.  The words just need to match to that.  How about:
> " If [RFC6241] filtering is supported, the full set of Section 6 must be enabled.  If XPATH filtering is supported, the subset of XPATH functionality is left to the implementation "

The same argument applies to XPath.  How would this be interoperable
otherwise?  I suggest you simply remove the sentence

      The subsets of these filtering syntaxes supported are
      left to each implementation.

> > o  Section 2.3
> > 
> >   The state diagram shows an arrow "modify" from "suspended" to
> >   "active".  Is this really correct?  Will a suspended subscription be
> >   resumed if it is modified?  
> 
> Yes. 
> The subscription could immediately be suspended, but this reduces the available knobs.

How does it reduce the available knobs?  I was thinking that if a
modify-subscription rpc was sent to a suspended subscription, it
wouldn't by itself change the suspension status; if the server thinks
it can be resumed it will resume.

With the current solution, the server must send three notifications if
a suspended subscription is modified, and it is still suspended:

  subscription-modified
  subscription-resumed
  subscription-suspended


> > If so, this is not correct:
> >     o  Suspend and resume state changes are driven by internal process
> >        and prioritization.  There are no external controls over suspend
> >        and resume.
> 
> How about: "there are no direct controls over suspend and resume other than modifying a subscription."  
> 
> Right before Figure 2, I will also add the sentence to clarify:
> Modification of a configured subscription will place any suspended receivers into the connecting receiver state.
> 
> > o  Section 3
> > 
> >   The data model is not yet converted to NMDA single tree (and there
> >   is no issue that tracks this).
> 
> We have an umbrella issue for this:
> YP#11: Shift to NMDA compliant structure
> 
> We authors are willing to shift to NMDA constructs (allowing side-by-side comparison) once all other issues are closed and a firm WGLC start date is set for yang push. 

The sooner you get it in the better, since it will speed up
everything.

> > o  Section 4.1.1
> > 
> >      Not all streams will support replay.  Those that do MUST include
> >      they do via the "replay-support" object.
> > 
> >   The second sentence still looks odd.  I propose:
> > 
> >     If a stream supports replay, the "replay-support" leaf is present
> >     in the "/streams/stream" list entry for the stream.
> 
> Will make this change
> 
> > o  Section 4.4
> > 
> >      An operator may find subscription
> >      identifiers which may be used with "kill-subscription" by searching
> >      for the ip address of a receiver within the yang tree.
> > 
> >   s/within the yang tree/in the "subscription" list"/
> > 
> >   or something along those lines.
> 
> Will make this change
> 
> > o  Section 5
> > 
> >      o  One or more receiver IP addresses (and corresponding ports)
> >         intended as the destination for notification messages for each
> >         subscription.  In addition, the transport for each destination MAY
> >         be defined.
> > 
> >    Since the transport is mandatory in the data model, the last
> >    sentence should be removed.
> 
> Will make this change
> 
> > o  Section 5.1
> > 
> >   The text in -07 is an improvement over -05.  But there are still
> >   some things that needs to be specified.
> > 
> >   The text needs to discuss what's supposed to happen if a transport
> >   session goes down. Will the server re-establish the session?  
> 
> As there are transport specific behaviors.  For NETCONF, see Section 5.2 of draft-ietf-netconf-netconf-event-notifications.

Ok, makes sense.  I suggest you add a sentence along the lines of:

  How transport failures are dealt with is out of scope for this
  document.  It is expected that transport-specific documents describe
  how failures are handled.

> >  Will it buffer events during this time?  
> 
> Events are buffered, if available.  If events are lost (rather than just delayed) due to the loss of transport connectivity, a new subscription-started must be sent.  This indicates the discontinuity to the receiver.

I assume this should also be covered by the transport specific
documents.

> Note I will add these two sentences directly above to section 5.1 to clear up any confusion. 
> 
> On a side note: I will tweak the text of " 8.4. subscription-suspended" to 
> " This indicates that a publisher has suspended the sending of event records to a receiver, and indicates the possible loss of events."
> And delete the sentence from both 8.4 and 8.5 saying...
> "Suspensions are notified to the subscriber..."
> .
> >  What happens if the call home
> >   setup due to authentication issues, will it still try to
> >   re-establish?
> 
> For NETCONF, see Section 5.2 of draft-ietf-netconf-netconf-event-notifications.
>  
> >   The new text says:
> > 
> >      Note that is possible to configure replay on a configured
> >      subscription.  This is to allows a configured subscription to
> >      exist on a system so that event records generated during boot can
> >      be buffered and pushed as soon as the transport session is
> >      established.
> > 
> >   I assume this means that the reciever must be prepared to receive
> >   the same events multiple times?  (if the server reboots before the
> >   replay buffer has been overwritten)
> 
> No, boot time is considered.   The proper operation is documented in the YANG model..
> 
>   notification subscription-started {
>     uses subscription-policy {
>       refine "target/stream/replay-start-time"
>            "Indicates the time that a replay using for the streaming of
>            buffered event records.  This will be populated with the most
>            recent of the following: replay-log-creation-time,
>            replay-log-aged-time, replay-start-time, or the most recent
>            publisher boot time.";

Ok.

> > o  Section 9.2
> > 
> >   s/yang/YANG/
> 
> Will fix
> 
> >   Also, the text says:
> > 
> >     In addition support for optional features: encode-xml,
> >     encode-json, configured, and replay MUST also be indicated if
> >     supported.
> > 
> >   I think you should remove this sentence; it is redundant.  If you
> >   decide to keep it, write:
> > 
> > 
> >     In addition support for optional features: "encode-xml",
> >     "encode-json", "configured", "replay", "xpath", and "subtree" MUST
> >     also be indicated if supported.
> 
> Will do this one.  
> 
> >   BTW, why did you rename "configured-subscriptions" to "configured"?
> >   "configured" looks a bit short.
> 
> Due to the YANG tree.   The longer feature name causes make a significant number of the lines exceed the character limit for a row.   And things become very difficult to read.

I think we should pick descriptive names that make sense, and not
names that make the tree diagram or whatever look nice.  I suggest you
keep the original name.

> > o  10 - extension
> > 
> >      extension subscription-state-notif {
> > 
> >   I think you should spell out "notification"; i.e., use
> >   subscription-state-notification.
> > 
> >   (You replied "will do" to this comment in my previous review, so I
> >   suspect you simply forgot this one)
> 
> Yes.  This time I really will do :-)
> 
> > o  10 - filter-spec
> > 
> >   I think both the subtree and XPath filters are not correctly
> >   defined.  I provided text about the XPath filter, but you didn't
> >   include it, so here it is again, updated with your text about
> >   events.
> 
> 
> Thanks for keeping on about this.  The definitions you provide are an upgrade, and I will include.
>  
> >       anydata subtree-filter {
> >         description
> > 
> > 
> >   OLD:
> > 
> >           "Event stream evaluation criteria encoded in a syntax of a
> >            supported type of an RFC 6241, Section 6 filter.  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.  If the
> >            stream filter matches an event record from the stream, the
> >            event record should be included in a notification message
> >            to the receiver(s).";
> >       }
> > 
> >   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 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.
> > 
> >            If the subtree filter returns a non-empty node set, the
> >            filter matches the event record, and the it is included in
> >            the notification message sent to the receivers.";
> > 
> >         reference "RFC 6241, Section 6.";
> >       }
> > 
> >   and:
> > 
> >       leaf xpath-filter {
> >         type yang:xpath1.0;
> >         description
> > 
> >   OLD:
> > 
> >           "Event stream evaluation criteria encoded in a syntax of xpath
> >            1.0 and applied against an event stream. The result of
> >            applying XPath expression is converted to a boolean value
> >            using the standard XPath 1.0 rules.  If the boolean value is
> >            'true', the stream filter matches an event record within the
> >            stream, and the notification message should be sent to the
> >            receiver(s).";
> > 
> >   NEW:
> > 
> >           "Event stream evaluation criteria encoded in the syntax of
> >            an XPath 1.0 expression.
> > 
> >            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.
> > 
> >            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 the event
> >            record, and the it is included in the notification message
> >            sent to the receivers.
> > 
> >            The expression is evaluated in the following XPath context:
> > 
> >              o  The set of namespace declarations are those in scope on
> >                 the 'xpath-filter' leaf element where th
> > 
> >              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.
> > 
> >         reference
> >           "http://www.w3.org/TR/1999/REC-xpath-19991116
> >            RFC 7950, Section 10.";
> >       }
> > 
> > 
> > 
> > o  10 - grouping notification-origin-info
> > 
> >   In your reply tpo my previous review, you wrote that you would
> >   remove the term "push source".  It is still used.
> 
> Yes, will update.




/martin