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

[Martin Bjorklund <[e-mail]>]

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/


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.

  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.


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?

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

  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.


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

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.


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.

  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.


o  Section 2.4.2

  s/"start-time"/"replay-start-time"/  (3 occurrences)

  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?


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.


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.


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.


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?


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?


o  Section 2.5.2

  s/details."/details./


o  Section 2.5.6

  I know that this feature was presented in Montreal.  I think it is
  problematic, and suggest it is removed.

  For example, what happens if the transport session is lost and
  re-established; are all event records in the buffer replayed again?

  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?

  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?


  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.


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.


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.


o  Section 2.8

  s/operational datastore/operational state datastore/

 (four occurrences)


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


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.


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.

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


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


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.


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.

  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)


o  Section 4

  OLD:

    "This feature indicates support for xpath filtering.";

  NEW:

    "This feature indicates support for XPath filtering.";


o  Section 4

  With the inclusion of "replay-start-time-revision", shouldn't the
  identity "history-unavailable" be 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/


o  Section 5.4

   s/publisher"/publisher/


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)


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



/martin