[Netconf] mbj's WGLC comments on subscribed-notifications-10

[Martin Bjorklund <[e-mail]>]

Hi,

Here are my WGLC comments on
draft-ietf-netconf-subscribed-notifications-10.

In general, I think ths document has improved a lot, it is now easier
to follow.



Major issue
-----------

o  list receiver

   In the YANG model, the "receiver" list is indexed by ip and port.

   How is the server supposed to use this simple list of ip/port?

   We get a hint from the document in 2.5.2:

     transport specific
     call-home operations will be used to establish the connection

   But it is not clear that this list of ip/port helps.  The document
   draft-ietf-netconf-netconf-client-server provides configuration for
   call home for NETCONF.  In that document, the receiver is
   identified with a "netconf-client" name and an "endpoint" name, not
   ip/port. (draft-ietf-netconf-restconf-client-server has a similar
   structure for RESTCONF).

   The best solution would have been to have leafrefs to these
   datamodels, but if we do that now, it would introduce a delay b/c
   of the dependency.

   The second best solution (I think) is to change this list to have
   just a "name" as the single key, and nothing else, and write that
   transport-specific modules can add to this in the future.

   Once the client-server docs are ready, we need to revise some
   document to add the leafrefs in this list.

   See also below about my comments about transport identities.  I
   think we should introduce a YANG module in both the
   transport-specific documents, that would define the transport
   identity, and augment the receiever list with the appropriate
   leafrefs to call home.


Other issues
------------

o  Section 1

  Somewhere in section 1 you should add:

    Tree diagrams used in this document follow the notation defined in
    [I-D.ietf-netmod-yang-tree-diagrams].

  and add the reference.


o  Section 1.4

  Should this section explain that this document is intended to be
  transport independent, whereas RFC 5277 was for NETCONF only?


o  Section 1.4

  You write:

   o  the data model in this document replaces the data model in
      [RFC5277].

  Does this mean that in an implementation that supports both 5277 and
  this new document, the set of streams in the two data models are
  guaranteed to be equal?

  I would assume that an implementation should be allowed to keep the
  5277 stuff as-is, and just add future streams to this new mechanism.


o  Section 1.4

  I suggest you make the text a bit more explicit:

  OLD:

   o  the RPC operations in this draft replaces the symmetrical
      operations of [RFC5277], section 4.

  NEW:

   o  the RPC operations in this draft replaces the operation
      "create-subscription" defined in [RFC5277], section 4.


o  Section 1.4

  I know that RFC 5277 uses the term "one-way operation" in a comment
  (but also the term "one-way message").  However, I suggest we align
  with the terminology of RFC 6241, and also make the text more
  explicit:

  OLD:

   o  the one way operation of [RFC5277] is still used.  However this
      operation will no longer be required with the availability of
      [I.D.draft-ietf-netconf-notification-messages].

  NEW:

   o  the <notification> message defined in [RFC5277] is still used.
      However this message will no longer be required with the
      availability of [I.D.draft-ietf-netconf-notification-messages].


o  Section 1.4

  (editorial; quote the name of the stream)

  OLD:

   o  the definition and contents of the NETCONF stream are identical
      between this document and [RFC5277].

  NEW:

   o  the definition and contents of the "NETCONF" stream are identical
      between this document and [RFC5277].


o  Section 1.4

  You write:

   o  a publisher MAY implement both the data model and RPCs defined in
      [RFC5277] and this new document concurrently, in order to support
      old clients.  However the use of both alternatives on a single
      transport session is prohibited.

  What exactly does the last sentence mean?  And why do you have this
  restriction?   This is just the Introduction, but I can't find any
  details about this restriction in the rest of the document.  I
  suggest you remove this last sentence.  If not, you should work out
  the details later in the doc, and add a reference to that new new
  text in section 1.4.


o  Section 2.1

  (editorial; quote the name of the stream)

  OLD:

   There is only one reserved event stream within this document:
   NETCONF.  The NETCONF event stream contains all NETCONF XML event
   record information supported by the publisher, except for where it
   has been explicitly indicated that this the event record MUST be
   excluded from the NETCONF stream.  The NETCONF stream will include
   individual YANG notifications as per [RFC7950] section 7.16.  Each of
   these YANG notifications will be treated a distinct event record.
   Beyond the NETCONF stream, implementations are free to add additional
   event streams.

  NEW:

   There is only one reserved event stream within this document:
   "NETCONF".  The "NETCONF" event stream contains all NETCONF XML event
   record information supported by the publisher, except for where it
   has been explicitly indicated that this the event record MUST be
   excluded from the "NETCONF" stream.  The "NETCONF" stream will include
   individual YANG notifications as per [RFC7950] section 7.16.  Each of
   these YANG notifications will be treated a distinct event record.
   Beyond the "NETCONF" stream, implementations are free to add additional
   event streams.


o  Section 2.3

  I think it would be useful to have "dscp" covered by a separate
  feature than "dependency" and "weighting".  The former is quite
  simple to implement, but the latter seems to require a very separate
  implementation; thus I think there should be two separate features.

  Also, you write that dependency MUST work identically to 7540, but
  7540 defines an "exclusive dependency" that is not covered here.
  Maybe it is worth mentioning that this is excluded.


o  Section 2.4.1

  Be explicit:

  OLD:

   o  Successful establish or modify RPCs put the subscription into an
      active state.

   o  Failed modify RPCs will leave the subscription in its previous
      state, with no visible change to any streaming updates.

   o  A delete or kill RPC will end the subscription.


  NEW:

   o  Successful "establish-subscription" or "modify-subscription" RPCs
      put the subscription into the "active" state.

   o  Failed "modify-subscription" RPCs will leave the subscription in
      its previous state, with no visible change to any streaming
      updates.

   o  A "delete-subscription" or "kill-subscription" RPC will end the
      subscription.


o  Section 2.4.2

  (editorial; you have a name for the "via RPC"-subscribtions; use
  that name)

  OLD:

   The "establish-subscription" operation allows a subscriber to request
   the creation of a subscription via RPC.

  NEW:

   The "establish-subscription" operation allows a subscriber to request
   the creation of a dynamic subscription.


o  Section 2.4.2

  You write:

   It MUST be possible to
   support multiple establish subscription RPC requests made within the
   same transport session.

  I think you mean:

   A server MUST support multiple "establish-subscription" requests
   made within the same transport session.


o  Section 2.4.2

  You write:

   And it MUST be possible to support the
   interleaving of RPC requests made on independent subscriptions.

  I think you mean:

   Additionally, a server MUST support interleaving of RPC requests
   with outgoing notification messages.


o  Section 2.4.2

  (clarification)

  OLD:

   If the publisher can satisfy the "establish-subscription" request, it
   provides an identifier for the subscription, and immediately starts
   streaming notification messages.

  NEW:

   If the publisher can satisfy the "establish-subscription" request,
   it replies with an identifier for the subscription, and then
   immediately starts streaming notification messages.


o  Section 2.4.2.1

  This is something I have commented on before (see the thread
  "Martin's thoughts on subscribed-notifications" from October 2017).
  I think there is one small change left to do:

  OLD:

   If a "stop-time" parameter is included, it MAY also be earlier than
   the current time and MUST be later than the "replay-start-time".  The
   publisher MUST NOT accept a "replay-start-time" for a future time.

   If the "replay-start-time" is later than any information stored in
   the replay buffer, then the publisher MUST send a "replay-completed"
   notification immediately after the "subscription-started"
   notification.

  NEW:

   If a "stop-time" parameter is included, it MAY also be earlier than
   the current time and MUST be later than the "replay-start-time".

   If the "replay-start-time" is later than any information stored in
   the replay buffer, then the publisher MUST send a "replay-completed"
   notification immediately after the "subscription-started"
   notification.


o  Section 2.4.2.1

  (editorial clarification)

  OLD:

   To assess the availability of replay, subscribers can
   perform a get on "replay-log-creation-time" and "replay-log-aged-
   time".

  NEW:

   To assess the availability of replay, subscribers can
   read the leafs "replay-log-creation-time" and
   "replay-log-aged-time".


o  Section 2.4.3

  ("via RPC" again)

  OLD:

   Dynamic subscriptions established via RPC can only be modified via
   RPC using the same transport session used to establish that
   subscription.

  NEW:

   Dynamic subscriptions can only be modified
   using the same transport session used to establish that
   subscription.


o  Section 2.4.3

  (there is just *one* active state (I assume))

  OLD:

   (i.e., the modified subscription is returned to an active state.)

  NEW:

   (i.e., the modified subscription is returned to the "active"
   state.)


o  Section 2.4.5

  (fix typo)

  OLD:

   by searching for the IP address of a receiver within
   "subscriptions\subscription\receivers\receiver\address".

  NEW:

   by searching for the IP address of a receiver within
   "/subscriptions/subscription/receivers/receiver/address".


o  Section 2.4.6

  OLD:

   1.  yang-data establish-subscription-error-stream: This MUST be

   NEW:

   1.  "establish-subscription-error-stream": This MUST be


   And same for the other two.

   Also, I suggest you change the name to
   "establish-subscription-error".


o  Section 2.5.1

  (explicitily name the states)

  OLD:

   subscription may also transition to a concluded state if a configured

  NEW:

   subscription may also transition to the "concluded" state if a configured


  OLD:

   subscription, and is only relevant when the configured subscription
   itself is determined to be valid.

  NEW:

   subscription, and is only relevant when the configured subscription
   itself is in the "valid" state.

  OLD:

   When a subscription first becomes valid, the operational state of
   each receiver is initialized to connecting.  Individual receivers are
   moved to an active state when a "subscription-started" state change
   notification is successfully passed to that receiver.  Configured
   receivers remain active if transport connectivity is not lost, and
   event records are not being dropped due to a publisher buffer
   overflow.  A configured subscription's receiver MUST be moved to
   connecting if transport connectivity is lost, or if the receiver is
   reset via configuration operations.

  NEW:

   When a subscription first becomes valid, the "state" leaf of
   each receiver is initialized to "connecting".  Individual receivers are
   moved to the "active" state when a "subscription-started" state change
   notification is successfully passed to that receiver.  Configured
   receivers remain active if transport connectivity is not lost, and
   event records are not being dropped due to a publisher buffer
   overflow.  A configured subscription's receiver MUST be moved to the
   "connecting" state if transport connectivity is lost, or if the receiver is
   reset via configuration operations.

  OLD:

   A configured subscription's receiver MUST be moved to a suspended
   state if there is transport connectivity between the publisher and
   receiver, but notification messages are not being generated for that
   receiver.  A configured subscription receiver MUST be returned to an
   active state from the suspended state when notification messages are
   again being generated and a receiver has successfully been sent a
   "subscription-resumed" or a "subscription-modified".

  NEW:

   A configured subscription's receiver MUST be moved to the "suspended"
   state if there is transport connectivity between the publisher and
   receiver, but notification messages are not being generated for that
   receiver.  A configured subscription receiver MUST be returned to the
   "active" state from the "suspended" state when notification messages are
   again being generated and a receiver has successfully been sent a
   "subscription-resumed" or a "subscription-modified" notification.



o  Section 2.5.1

  You write:

   A configured subscription's receiver MUST be moved to
   connecting if transport connectivity is lost, or if the receiver is
   reset via configuration operations.

  What does "reset via configuration operations" mean?  I see an
  action called "reset", is that involved here?


o  Section 2.5.1

  You write:

   Suspended receivers will also be
   informed of the modification.  However this notification will await
   the end of the suspension for that receiver.

  What does the last sentence mean here?  Maybe instead write

   Suspended receivers will also be
   informed of the modification.  See Section 2.5.3 for details.


o  Section 2.5.1

  OLD:

   [I-D.ietf-netconf-yang-push] provides an example of such an
   extension.

  NEW:

   [I-D.ietf-netconf-yang-push] provides a definition of such an
   extension.

  (it is not just an example...)


o  Section 2.5.3

  If a suspended receiver exists, and the subscription is modified
  several times, does the server have to buffer all
  "subscription-modified" notifs?  I think this behavior should be
  clarified.


o  Section 2.5.5

  Add text that explains that the "reset" action is used for this.


o  Section 2.6

  ("via RPC" again)

  OLD:

   For dynamic subscriptions set up via RPC
   operations, notification messages are sent over the session used to
   establish the subscription.

  NEW:

   For dynamic subscriptions, notification messages are sent over the
   session used to establish the subscription.


o  Section 2.6

  I can't parse this:

   A subscription's events MUST NOT be sent to a receiver
   until after a corresponding RPC response or state-change notification
   has been passed receiver indicating that events should be expected.

  Maybe you mean:

   A subscription's events MUST NOT be sent to a receiver unless the
   receiver is in the "active" state.


o  Section 2.6

  OLD:

   This notification
   message MUST be encoded as one-way notification element of [RFC5277],
   Section 4.

  NEW:

   This notification
   message MUST be encoded in a <notification> message as defined in [RFC5277],
   Section 4.

  OLD:

   This [RFC5277] section 4 one-way operation has the drawback of not
   including useful header information such as a subscription
   identifier.

  NEW:

   The <notification> message defined in [RFC5277] section 4 has the
   drawback of not including useful header information such as a
   subscription identifier.


o  Section 2.7

  Perhaps write that the subscription state notifications are not
  stored in replay buffers.


o  Section 2.7.1

  The diagram doesn't quite fit.  Using the latest pyang like this:

    $ pyang -f tree [e-mail] \
      --tree-line-length 69 --tree-path /subscription-started

  gives you a tree that fits.

  In general, I think you should re-generate all tree diagrams in the
  document, for example by using the latest pyang from github, so that
  they match the I-D.ietf-netmod-yang-tree-diagrams.

  The RFC editor tries to check that the tree output is correct, so
  fixing this before sending the doc to the RFC editor helps.


o  Section 2.7.2

  (clarififcation)

  OLD:

      A "subscription-modified" state
      change notifications MUST be sent if the contents of a filter
      identified by a "stream-filter-ref" has changed.

  NEW:

      A "subscription-modified" state change notification MUST be sent
      if the contents of the filter identified by the subscription's
      "stream-filter-ref" leaf has changed.


o  Section 2.7.4

  You write:

   The
   two conditions where is this possible are "insufficient-resources"
   and "unsupportable-volume".

  What exactly is the difference between these two?  If I can't handle
  the volume I probably have insufficient resources?


o  Section 2.8

  ("by RPC" again)

  OLD:

   Subscriptions that were established by RPC are removed from the list
   once they expire (reaching stop-time) or when they are terminated.
   Subscriptions that were established by configuration need to be
   deleted from the configuration by a configuration editing operation
   even if the stop time has been passed.

  NEW:

   Dynamic subscriptions are removed from the list
   once they expire (reaching stop-time) or when they are terminated.
   Configured subscriptions need to be
   deleted from the configuration by a configuration editing operation
   even if the stop time has been passed.

o  Section 4

  The module's description should contain the usual IETF boilerplate
  text.


o  subscription-terminated-reason

      "Problem condition communicated to a receiver as part of absolute
      'subscription-terminated' notification. ";

  What does the word "absolute" tell me?  Can it be removed?

  (same for subscription-suspended-reason)


o  identity encodings

  Shouldn't this be "encoding"?  It is just one singluar encoding,
  right?


o  identity encode-xml

   Add "as described in RFC 7950."

   and add a reference statement


o  identity encode-json

   Add "as described in RFC 7951."

   and add a reference statement


o  For the transport identities, add RFC editor notes to replace the
   draft strings.

   For the http1.1 and http2 transport, you have listed these docs as
   "informative refererences".  I understand this; otherwise it would
   have been a downref, but it is really correct?   An alternative
   would be to specify the transport-specific identities in the
   transport-specific documents.


o  typedef stream-ref

    description
      "This type is used to reference a system-provided datastream.";

    What is a "datastream"?   This word occur twice in the document,
    w/o further explanation.


o    typedef stream-filter-ref {

  OLD:

      "This type is used to reference a configured stream filter.";

  NEW:

      "This type is used to reference a stream filter.";


o  list stream-filter

   What happens if no filter-spec has been defined?
   Should the choice filter-spec be mandatory?


o  leaf protocol / encoding

   Why is "protocol" only for the feature "configured", but "encoding"
   is not?


o  For all rpcs:

   In order to make it clear to implementors what to do in the case of
   errors, add a paragraph to the description:

     If an error occurs, the server replies with an 'rpc-error' where
     the 'error-info' field MAY contain an instantation of the
     'establish-subscription-error' structure.


o  The YANG module is inconsistently idented.  To get some help, you
   can use the latest version of pyang on github and run:

     $ pyang -f yang --keep-comments \
         [e-mail]  > x
     $ diff [e-mail] x


o  Terminology issue.

  I have reported this one before.  You use the terms "transport",
  "protocol", and "transport protocol" for the same thing.  I suggest
  you pick one.

  For example, the leaf "protocol" has type "transport".

  I think that the term "protocol" is supposed to mean a management
  protocol for YANG data, such as NETCONF or RESTCONF.

  NETCONF can use two different transports, SSH and TLS.



/martin