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

["Eric Voit (evoit)" <[e-mail]>]

> Martin Bjorklund, March 16, 2018 8:33 AM
>
> 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).

Sigh.  I need find better tooling.  I have too much manual stuff right now.   Perhaps someone has documented / is maintaining a continuous integration environment somewhere?   

This particular issue is fixed.  The YANG file if you want it for testing is at
https://github.com/netconf-wg/rfc5277bis/blob/master/ietf-subscribed-notifications%402018-03-16.yang 

Look for responses to your other comments later today.

Eric

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