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

Thanks for the comments Martin,  thoughts in-line...

> From: Martin Bjorklund, September 10, 2018 1:42 PM
> 
> Hi,
> 
> I have reviewed this document, and I think it is almost ready.  Here are my
> comments:
> 
> 
> o  Section 1.4
> 
>   s/RFC-5277/RFC 5277/

Done

> 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 

>   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.  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."   So for -v16 I updated the text in a way that doesn't preclude this.  

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

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

>   Further, the text says:
> 
>     Each of these
>     YANG 1.1 notifications will be treated as a distinct event record.
> 
>   This seems to confuse the schema definition with specific instances;
>   it is not the schema definition that is treated as a disctinct event
>   record, but the notification instances.
> 
>   So, I suggest that these two sentences are removed.

Removed.

> o  Section 2.4.1
> 
>   OLD:
> 
>    o  A delete or kill RPC will end the subscription, as will the
>       reaching of a "stop-time".
> 
>   NEW:
> 
>    o  A "delete-subscription" or "kill-subscription" RPC will end the
>       subscription, as will the reaching of a "stop-time".

updated

> o  Section 2.4.1
> 
>   OLD:
> 
>       Successful
>       modification returns the subscription to an active state.
> 
>   NEW:
> 
>       Successful
>       modification returns the subscription to the active state.

updated

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

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

> o  Section 2.4.2.1
> 
>   OLD:
> 
>    If the time the replay starts 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.
> 
>   NEW:
> 
>    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.

Change made

> o  Section 2.4.2.1
> 
>   There is no text that explains what happens in the "succesful" case;
>   i.e., when there are buffered event records with time greater than
>   or equal to the requested "replay-start-time".
> 
>   Maybe the first paragraph is supposed to cover this, but it just
>   says "meets the timeframe criteria", which is a bit vague.

This was supposed to be covered by the first paragraph.  To help clarify, I have updated the text to:

"In other words, as the subscription initializes itself, it sends any event records within the target event stream which meet the filter criteria, which have an event time which is after the "replay-start-time", and which have an event time before the "stop-time" should this "stop-time" exist."

> o  Section 2.4.5
> 
>   OLD:
> 
>     A publisher MUST terminate any dynamic
>     subscription identified by RPC request.
> 
> 
>   NEW:
> 
>     A publisher MUST terminate the dynamic
>     subscription identified by the "identifier" parameter in the RPC
>     request, if such a subscription exists.

updated

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

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

> o  Section 2.5.2
> 
>   s/details."/details./

Fixed

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

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

> o  Section 2.7
> 
>   This section talks about "subscription state" notifications.  But in
>   many places in the document, you use the term "state change"
>   notification.  I suggest pick one term and use it consistently, to
>   avoid confusion.

Updated to subscription state change notification

> o  Section 2.8
> 
>   s/operational datastore/operational state datastore/
> 
>  (four occurrences)

updated

> o  Section 4
> 
>           action reset {
>             if-feature "configured";
>             description
>               "Allows the reset of this configured subscription receiver
> 
>   This action will be available also for dynamic subscription, if the
>   server supports the "configured" feature.  Maybe add:
> 
>     when "../../../configured-subscription-state";

This would be nice to have.  But action doesn't support "when" as an allowed sub-statement.

> o  Section 4
> 
>   Should the rpc kill-subscription be marked with
>   nacm:default-deny-all?  Otherwise, by default, anyone can terminate
>   any other's subscriptions.

This is a good idea.  Added.

> o  Section 4
> 
>   I suggest that the leaf /filters/stream-filter/identifier is renamed
>   to "name", for consistency, and that it is of type "string", and
>   that the type "filter-id" is removed, for consistency and
>   simplicity.

Done

>   Also, I suggest that the leaf /subscriptions/subscription/target/
>   stream-filter/by-reference/stream-filter-ref is renamed to
>   "stream-filter", or maybe "stream-filter-name".

Done.   (It is now 'stream-filter-name')

> o  Section 4
> 
>   I suggest that the subscription's key leaf "identifier" is
>   renamed to simply "id".  This would match the name of the type which
>   is "subscription-id".
> 
>   (it is also consistent with other (not all) IETF models, where a
>   "name" is used for an arbitrary string key, and "id" for a more
>   strict key, often some integer type)
> 
>   NOTE: yang push should also be changed to use the same naming
>   convention

Change made.   Alex knows to make this to YANG Push as well.

> o  Section 4
> 
>   I suggest the counters "count-sent" and "count-exlcuded" are renamed
>   to something more descriptive, perhaps "out-event-records" or
>   "sent-event-records", and "excluded-event-records" or
>   "dropped-event-records", respectively.

Changed to "sent-event-records" and "excluded-event-records".

> o  Section 4
> 
>   I suggest you consistently format the YANG module wrt. spaces etc.
>   Also, some lines are longer than 69 characters and should be
>   adjusted.

All lines are no longer that 69 characters now.

>   You can use pyang --ietf --max-line-length 69 to help with the
>   latter, and pyang -f yang --keep-comments to help with the former.
>   (note that you may need to manually fix the output of pyang -f,
>   e.g. if some lines are too long)
> 
> 
>   (The RFC editor checks this, so it is better to do it now)

Done. 

> o  Section 4
> 
>   OLD:
> 
>     "This feature indicates support for xpath filtering.";
> 
>   NEW:
> 
>     "This feature indicates support for XPath filtering.";

Updated.

> o  Section 4
> 
>   With the inclusion of "replay-start-time-revision", shouldn't the
>   identity "history-unavailable" be removed?

Removed.

> o  Section 4
> 
>   identity no-such-subscription {
>     base modify-subscription-error;
>     base delete-subscription-error;
>     base subscription-terminated-reason;
>     description
>      "Referenced subscription doesn't exist. This may be as a result of
>       a non-existent subscription ID, an ID which belongs to another
>       subscriber, or an ID for configured subscription.";
>   }
> 
>   s/ID/id/

Updated

> o  Section 5.4
> 
>    s/publisher"/publisher/

Updated

> o  Appendix A
> 
>   Since this appendix defines an example module, it needs to be named
>   accordingly.  Specifically, the module should be called
>   "example-foo-subscribed-notifications", and the namespace should be
>   "urn:example:foo-subscribed-notifications".
> 
>   (with this change, you can also remove the note to the RFC editor)

Updated

> o  Appendix A
> 
>   The name of Figure 21 should be changed:
> 
>   OLD:
> 
>            Figure 21: Example Transport Augmentation for NETCONF
> 
>   NEW:
>            Figure 21: Example Transport Augmentation for the
>            fictitious protocol foo

Done.   And thanks!

Eric

> /martin
> 
> _______________________________________________
> Netconf mailing list
> Netconf@ietf.org
> https://www.ietf.org/mailman/listinfo/netconf