Re: [Netconf] mbj's WGLC comments on netconf-event-notifications-08

Martin Bjorklund <mbj@tail-f.com> Mon, 11 June 2018 07:12 UTC

Return-Path: <mbj@tail-f.com>
X-Original-To: netconf@ietfa.amsl.com
Delivered-To: netconf@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 60990130E0F for <netconf@ietfa.amsl.com>; Mon, 11 Jun 2018 00:12:55 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.901
X-Spam-Level:
X-Spam-Status: No, score=-1.901 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, SPF_PASS=-0.001] autolearn=ham autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id VGgd0HkW_3EN for <netconf@ietfa.amsl.com>; Mon, 11 Jun 2018 00:12:52 -0700 (PDT)
Received: from mail.tail-f.com (mail.tail-f.com [46.21.102.45]) by ietfa.amsl.com (Postfix) with ESMTP id C2125130DFE for <netconf@ietf.org>; Mon, 11 Jun 2018 00:12:52 -0700 (PDT)
Received: from localhost (unknown [173.38.220.61]) by mail.tail-f.com (Postfix) with ESMTPSA id 88EEB1AE01AA; Mon, 11 Jun 2018 09:12:49 +0200 (CEST)
Date: Mon, 11 Jun 2018 09:12:48 +0200 (CEST)
Message-Id: <20180611.091248.42505202577647987.mbj@tail-f.com>
To: evoit@cisco.com
Cc: netconf@ietf.org
From: Martin Bjorklund <mbj@tail-f.com>
In-Reply-To: <0e6e711ab209437e881335756c268e07@XCH-RTP-013.cisco.com>
References: <20180316.145936.984795473579499350.mbj@tail-f.com> <20180608.163924.639364006777002795.mbj@tail-f.com> <0e6e711ab209437e881335756c268e07@XCH-RTP-013.cisco.com>
X-Mailer: Mew version 6.7 on Emacs 24.5 / Mule 6.0 (HANACHIRUSATO)
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: <https://mailarchive.ietf.org/arch/msg/netconf/kz9RJj4dfqCe5Vw6hAKnJ_mrync>
Subject: Re: [Netconf] mbj's WGLC comments on netconf-event-notifications-08
X-BeenThere: netconf@ietf.org
X-Mailman-Version: 2.1.26
Precedence: list
List-Id: Network Configuration WG mailing list <netconf.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/netconf>, <mailto:netconf-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/netconf/>
List-Post: <mailto:netconf@ietf.org>
List-Help: <mailto:netconf-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/netconf>, <mailto:netconf-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 11 Jun 2018 07:12:55 -0000

Hi,

Thanks for addressing my comments.  Some follow-ups inline.

"Eric Voit (evoit)" <evoit@cisco.com> wrote:
> > From: Martin Bjorklund, June 8, 2018 10:39 AM
> > 
> > Hi,
> > 
> > I haven't seen any reply to this WGLC review.
> 
> Hi Martin,
> 
> Below.  And with changes reflected in:
> https://github.com/netconf-wg/notif-netconf/blob/master/draft-ietf-netconf-netconf-event-notifications-10.txt


Thanks.  I have some additional comments on this version, but I don't
know if I should wait until you have published a new version or not.


> > Martin Bjorklund <mbj@tail-f.com> wrote:
> > > Hi,
> > >
> > > Here are my WGLC comments on
> > > draft-ietf-netconf-netconf-event-notifications-08
> > >
> > >
> > > o  On p. 3, theres a missing " (which messes up the colors in the
> > >    emacs mode I use)
> > >
> > >    OLD:
> > >
> > >      SHOULD NOT"
> > >
> > >    NEW:
> > >
> > >      "SHOULD NOT"
> 
> Your text is reflected in git version
> 
> > >    There's another one on p. 5:
> > >
> > >    OLD:
> > >
> > >       responses to an establish-subscription request) or "modify-
> > >       subscription-error-datastore (for error responses to a modify-
> > >
> > >    NEW:
> > >
> > >       responses to an establish-subscription request) or "modify-
> > >       subscription-error-datastore" (for error responses to a modify-
> 
> Based on comments with Kent, this has been turned into a more
> descriptive table format.
> 
> > > o  Section 3
> > >
> > >   As I have noted before, you mustn't require the :interleave
> > >   capability to be supported.  That capability is for 5277 only.  This
> > >   new mechanism *requires* that rpc's can be sent while there are
> > >   active subscriptions, so there is no need for a capability.
> > >
> > >   Remove this section.
> 
> Done.
> 
> > > o  Section 4
> > >
> > >   What is the reason for not allowing 5277 subscriptions on the same
> > >   session as these new subscriptions?
> 
> The biggest reason is that existing 5277 implementations can safely
> assume that only one subscription can be established on a NETCONF
> session.  And therefore all returned <notification> elements will
> belong to that subscription.  Even if the subscriber doesn't support
> subscribed-notifications or subsequently send an
> <establish-subscription>, without this constraint a configured
> subscription to the same receiver could inject notifications on the
> RFC-5277's NETCONF transport session.  (E.g., a <subscription-started>
> state change notification.)
> 
> > >   AFAICT this should just work fine, and no special rule is needed.
> 
> It likely could on a well behaved RFC-5277 implementation.  But
> caution is needed as RFC5277 implementations hadn't previously needed
> to worry about such co-existence.

I am ok with this.

> > > o  Section 5
> > >
> > >   You write:
> > >
> > >    A NETCONF publisher MUST support XML encoding of RPCs and
> > >    Notifications.
> > >
> > >   This is already specificed in RFC 6241.  You are not changing this,
> > >   so this sentence should be removed.
> 
> Previously changed this text to indicate the mandatory support of the
> "encode-xml" feature.

IMO this shows that the decoupling of transport and encoding is not
ideal.  It seems to imply that if I implement this draft I have to
support XML also for RESTCONF?    If the answer is "no" then the
encoding should not be decoupled from the transport.


> > > o  Section 5
> > >
> > >   You write:
> > >
> > >    A NETCONF publisher supporting
> > >    [I-D.draft-ietf-netconf-subscribed-notifications] MUST support the
> > >    "NETCONF" event stream identified in that draft.
> > >
> > >   This is already specificed in that draft. You are not changing this,
> > >   so this sentence should be removed.
> 
> In subscribed-notifications, the "NETCONF" stream is defined, but it
> is not mandatory support.  Instead, just that the stream name is
> reserved.  There will be IoT clients out there which don't need the
> NETCONF stream.

Ok.

> > > o  Section 6.2
> > >
> > >   (editorial, and clarified)
> > >
> > >   OLD:
> > >
> > >    For a configured subscription, there is no guarantee a transport
> > >    session is currently in place with each associated receiver.  In
> > >    cases where a configured subscription has a receiver in the
> > >    connecting state and the protocol configured as NETCONF, but no
> > >    NETCONF transport session exists to that receiver, the publisher MUST
> > >    initiate a transport session via NETCONF call home [RFC8071], section
> > >    4.1 to that receiver.  Until NETCONF connectivity is established and
> > >    a subscription-started state change notification is successfully
> > >    sent, that receiver MUST remain in a status of either "connecting" or
> > >    "timeout".
> > >
> > >   NEW:
> > >
> > >    For a configured subscription, there is no guarantee a transport
> > >    session is currently in place with each associated receiver.  In
> > >    cases where a configured subscription has a receiver in the
> > >    "connecting" state (see section 2.5.1 of [RFCXXXX] and the protocol
> > >    is configured as NETCONF, but no
> > >    NETCONF transport session exists to that receiver, the publisher MUST
> > >    initiate a transport session via NETCONF call home [RFC8071], section
> > >    4.1 to that receiver.  Until NETCONF connectivity is established and
> > >    a "subscription-started" state change notification is successfully
> > >    sent, that receiver MUST remain in either the "connecting" or the
> > >    "timeout" state.
> 
> The git version is now...
> 
> For a configured subscription, there is no guarantee a transport
> session is currently in place with each associated receiver. In cases
> where a configured subscription has a receiver in the "connecting"
> state as described in
> [I-D.draft-ietf-netconf-subscribed-notifications], section 2.5.1, and
> the "transport" for that subscription is "NETCONF", but no NETCONF
                                           ^^^^^^^^^

This should be "nsn:netconf" (an identity defined in this draft).

> transport session exists to that receiver (or all existing NETCONF
> transport sessions are currently supporting [RFC5277] subscriptions),
> then the publisher MUST initiate a transport session via NETCONF call
> home [RFC8071], section 4.1 to that receiver.  Until NETCONF
> connectivity is established and a "subscription-started" state change
> notification is successfully sent, that receiver MUST remain in either
> the "connecting" or the "timeout" state.
> 
> 
> > >   OLD:
> > >
> > >    If the call home fails because the publisher receives receiver
> > >    credentials which are subsequently declined per [RFC8071],
> > >    Section 4.1, step S5 authentication, then that receiver MUST be
> > >    assigned a "timeout" status.
> > >
> > >   NEW:
> > >
> > >    If the call home fails because the publisher receives receiver
> > >    credentials which are subsequently declined per [RFC8071],
> > >    Section 4.1, step S5 authentication, then that receiver MUST be
> > >    placed in the "timeout" state.
> 
> Your text is reflected in git version
> 
> > >   OLD:
> > >
> > >    If the call home fails to establish for any other reason, the
> > >    publisher MUST NOT progress the receiver to the "active" state.
> > >    Additionally, the publisher SHOULD place the receiver into a
> > >    "timeout" status after a predetermined number of either failed call
> > >    home attempts or NETCONF sessions remotely terminated by the
> > >    receiver.
> > >
> > >   NEW:
> > >
> > >    If the call home fails to establish for any other reason, the
> > >    publisher MUST NOT progress the receiver to the "active" state.
> > >    Additionally, the publisher SHOULD place the receiver into the
> > >    "timeout" state after a predetermined number of either failed call
> > >    home attempts or NETCONF sessions remotely terminated by the
> > >    receiver.
> 
> Your text is reflected in git version
> 
> > >   OLD:
> > >
> > >    NETCONF Transport session connectivity SHOULD be verified via
> > >    Section 4.1, step S7.
> > >
> > >   NEW:
> > >
> > >    NETCONF Transport session connectivity SHOULD be verified as
> > >    described in [RFC8071], Section 4.1, step S7.
> 
> Update made
> 
> > > o  Section 7
> > >
> > >   You write:
> > >
> > >    Notification messages transported over NETCONF will be identical in
> > >    format and content to those encoded using one-way operations defined
> > >    within [RFC5277], section 4.
> > >
> > >   "identical in content"?  What does this section tell me?
> 
> Tweaked the words to:
> 
> Notification messages transported over the NETCONF protocol will use
> the one-way operations defined within [RFC5277], section 4.

I would prefer to be more explicit:

  Notification messages transported over the NETCONF protocol will use
  the "notification" message defined in [RFC5277], section 4.

> > > o  Section 8
> > >
> > >    o  "error-app-tag" with the value being a string that corresponds to
> > >       an identity associated with the error, as defined in
> > >       [I-D.draft-ietf-netconf-subscribed-notifications] section 2.4.6
> > >
> > >   This needs to explained better.  See also my WGLC comments on the
> > >   other drafts.
> 
> Current git version includes your requested updates, such as which
> base identity to use for each RPC, and the JSON encoding format for
> the identities.

Hmm, I think I probably have to see updated versions of all three
drafts to see that it is consistently explained.

But wasn't the idea to have a "reason" leaf in each error-info
structure that contains this identity?  If so, shouldn't we remove
this "error-app-tag" handling?


> > >   Also, I don't think the 5th bullet is complete; it doesn't mention
> > >   "establish-subscription-error-stream" for example.
> 
> There is a whole table on this is the current git version.
> 
> > >   What is this section trying to tell me that isn't already said, e.g.
> > >   in section 3.8 of the push draft.  Maybe the other drafts should be
> > >   less specific and all such text moved here.  As it is now it is not
> > >   quite clear.
> 
> Alex is removing the text from the yang-push draft.
> 
> > > o  Section 8
> > >
> > >   You write:
> > >
> > >    Note that "error-path" does not need to be included with the "rpc-
> > >    error" element, as subscription errors are generally not associated
> > >    with nodes in the datastore but with the choice of RPC input
> > >    parameters.
> > >
> > >   This is a misconception how error-path works.  Please remove this
> > >   sentence.  For info, check RFC 6241.
> 
> Removed
> 
> > > o  Appendix A.2.1
> > >
> > >   I think it is useful to show an example of something that is easily
> > >   missed; that notifications can be sent at any time:
> > >
> > >   I suggest:
> > >
> > >   OLD:
> > >
> > >             |    establish-subscription    |
> > >             |----------------------------->|
> > >             | RPC Reply: OK, id = 23       |
> > >             |<-----------------------------|
> > >             |                              |
> > >             |                              |
> > >             | notification message (for 22)|
> > >             |<-----------------------------|
> > >
> > >   NEW:
> > >
> > >             |    establish-subscription    |
> > >             |----------------------------->|
> > >             | notification message (for 22)|
> > >             |<-----------------------------|
> > >             | RPC Reply: OK, id = 23       |
> > >             |<-----------------------------|
> > >             |                              |
> > >             |                              |
> > >             | notification message (for 22)|
> > >             |<-----------------------------|
> 
> Added
> 
> > > o   Appendix A.2.1
> > >
> > >   The example in Figure 3 is not correct.
> > >
> > >   The example in Fixgure 5 is not correct wrt namespace.
> > >
> > >   Hmm, it seems many examples are wrong.  I strongly suggest that you
> > >   set up automatic testing of all your examples.  If you for some
> > >   reason don't do that, please let me (and the WG) know so that we can
> > >   validate all examples in detail manually.  Meanwhile, I will not
> > >   check all examples.
> 
> Einar has since built an automated testbed for the examples.  Results
> are included in independent directories in the git repository:
> 
> https://github.com/netconf-wg/notif-netconf
> 
> Eric



/martin