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

[Martin Bjorklund <[e-mail]>]

Hi,

While reviewing the other drafts, I found another bug in this
document.  The "establish-subscription" rpc is supposed to return the
subscription id in the reply, but the rpc doesn't have any output
defined in the YANG model (interestingly, it is present in the tree
diagram).


/martin


Martin Bjorklund <[e-mail]> wrote:
> 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
> 
> _______________________________________________
> Netconf mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/netconf
>