Re: [netconf] Benjamin Kaduk's Discuss on draft-ietf-netconf-yang-push-23: (with DISCUSS and COMMENT)

[Benjamin Kaduk <[e-mail]>]

On Tue, May 14, 2019 at 07:53:52PM -0700, Alexander Clemm wrote:
> Hi Benjamin,
> 
> Thank you for your review!  Replies inline, <ALEX>.  Update will be
> reflected in -24, to be posted shortly.  
> 
> --- Alex
> 
> -----Original Message-----
> From: netconf <[e-mail]> On Behalf Of Benjamin Kaduk via
> Datatracker
> Sent: Wednesday, May 1, 2019 12:08 PM
> To: The IESG <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]
> Subject: [netconf] Benjamin Kaduk's Discuss on
> draft-ietf-netconf-yang-push-23: (with DISCUSS and COMMENT)
> 
> Benjamin Kaduk has entered the following ballot position for
> draft-ietf-netconf-yang-push-23: Discuss
> 
> When responding, please keep the subject line intact and reply to all email
> addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
> 
> 
> Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
> for more information about IESG DISCUSS and COMMENT positions.
> 
> 
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-netconf-yang-push/
> 
> 
> 
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
> 
> Note that I reviewed the -22 and the current version is -23.  Briefly
> skimming the diff, it seems that some changes touch on points I make in my
> review, but there is probably still discussion to have on them.
> 
> (pro-forma) I see the GenArt reviewer noted the author count (seven)
> already, but I couldn't find a response or note in the ballot or shephert
> writeup acknowledging this.  So failing that, I'll put up a discuss point
> until the responsible AD says it's fine.
> 
> [See also discussion on draft-ietf-netconf-subscribed notifications about
> the pre-RFC5378 boilerplate and whether or not it can be removed from this
> document]
> 
> <ALEX> Several of the authors have graciously agreed to step down and be
> listed as contributors. </ALEX>

Thanks to them!

> Section 3.3 states:
> 
>             In order for a subscriber to determine whether objects
>    support on-change subscriptions, objects are marked accordingly on a
>    publisher.  Accordingly, when subscribing, it is the responsibility
>    of the subscriber to ensure it is aware of which objects support on-
>    change and which do not.  For more on how objects are so marked, see
>    Section 3.10.
> 
> Chasing the reference, we see that this marking is left for future work or
> implementation-specific usage.  I'm not very comfortable with the way we are
> describing this situation, as it seems pretty fragile in the face of
> different implementations trying to interoperate.
> 
> <ALEX> The working group decided to pull this item out of this draft.  It is
> being addressed in draft-ietf-netconf-notification-capabilities
> (https://datatracker.ietf.org/doc/draft-ietf-netconf-notification-capabiliti
> es/) </ALEX>

I guess I'll see what the -24 looks like, but I'm not sure yet whether
"pull this item out of the draft" is describing a previous decision that
left it in this state or a current decision starting from this state.  (The
latter would seem to make more sense to me, but I can wait and see.)

> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> Thank you for the very thoughtful document!  I've lost track of the number
> of places where I started writing a comment only to note that my concern had
> already been addressed in the following text.  In general the writing style
> is great, though I did find some grammar and clarity nits (noted inline).
> 
> Abstract
> 
>                Providing such visibility into updates enables new
>    capabilities based on the remote mirroring and monitoring of
>    configuration and operational state.
> 
> This phrasing ("new capabilities based on") is hard for me to follow,
> particularly about whether these are protocol-level capabilities and what
> actors are granted the new capabilities.
> 
> <ALEX> The capabilities are clearly not related to protocol-level
> capabilities, but capabilities for applications that have uses for
> information obtained from YANG datastores.  If it's okay, at this point we
> would like to keep it as-is. </ALEX>

It's your prerogative to keep it as-is, though the RFC Editor may also take
a crack at the wording.

> Section 1
> 
>    Traditional approaches to providing visibility into managed entities
>    from remote have been built on polling.  [...]
> 
> nit: "remote" is an adjective and needs a noun to bind to; "from remote
> systems", "from remote vantages", or "from afar" would all be fine wordings
> here.
> 
> <ALEX> changed to  "from a remote system".  </ALEX> 
> 
>    o  Polling incurs significant latency.  This latency prohibits many
>       application types.
> 
> nit: I'd suggest wording as "types of application", since on my first
> reading I thought this was referring to some sort of codepoint.
> 
> <ALEX> changed </ALEX>
> 
>    o  Polling cycles may be missed, requests may be delayed or get lost,
>       often when the network is under stress and the need for the data
>       is the greatest.
> 
> nit: the grammar is a bit weird, here, as if a comma splice.  I think
> replacing the first comma with a colon or em dash would suffice.
> 
> <ALEX> replaced first comma with "and", to read: "Polling cycles may be
> missed and requests may be delayed or get lost, often when the network is
> under stress and the need for the data is the greatest." </ALEX>

Thanks!

>    o  Datastore node update: A data item containing the current value of
>       a datastore node at the time the datastore node update was
>       created, as well as the path to the datastore node.
> 
> Is this storing the "new" or the "old" value as the "current value"?
> 
> <ALEX> I don't understand the question.  I think the statement is
> clear.</ALEX>

This sounds like it's conceptually "take a copy of some data as you make an
update it, to have a historical record of what changed".  I mostly assume
that the copy operation happens before the update, so the old/previous
value is recorded in the record, but the text seems to describe them
happening at the same time, without a specified ordering between them.
(That said, IIRC, there is some text later in the document that helps
clarify the intent, but that is not a free pass for leaving things unclear
here.)

> Section 3.1
> 
>          +  Dampening period: In an on-change subscription, detected
>             [...]
>             is included.  The dampening period goes into effect every
>             time an update record completes assembly.
> 
> Just to double-check: this means that after a long hiatus, the first change
> to the monitored subtree(s) triggers an immediate push with just the single
> update, and only then does the dampening period kick in and defer delivery
> of potential subsequent updates?
> 
> <ALEX> Yes, this is correct </ALEX>
> 
> Section 3.5.2
> 
> This bit about the "create" and "delete" errors from RFC 8072 Section
> 2.5 not being errors in our usage is a little interesting, process-wise.
> In one sense, we are changing the semantics of an already published RFC, and
> would need to apply an "Updates:" relationship to indicate that, but in the
> other sense we are building a new custom thing that reuses a lot of the
> syntax/semantics of YANG patch but is fundamentally a new
> (different) thing.  The phrasing we use to talk about it can affect which
> case the reader perceives us to be in...
> 
> The discussion on the "change-type" enumeration seems to pretty clearly
> place us in the latter case, which is good.
> 
> <ALEX> Keeping this as is. </ALEX>
> 
> Section 3.6
> 
> Thank you for the note about the power of XPath expressions and the duty of
> the receiver to understand what it's asking for -- that sort of statement
> would potentially even be appropriate in the Security Considerations (but is
> fine where it is)!
> 
> <ALEX> Keeping this as is.  </ALEX>
> 
> Section 3.7
> 
>    Of note in the above example is the 'patch-id' with a value of '0'.
>    Per [RFC8072], the 'patch-id' is an arbitrary string.  With YANG
>    Push, the publisher SHOULD put into the 'patch-id' a counter starting
>    at '0' which increments with every 'push-change-update' generated for
>    a subscription.  If used as a counter, this counter MUST be reset to
>    '0' anytime a resynchronization occurs (i.e., with the sending of a
>    'push-update').  Also if used as a counter, the counter MUST be reset
>    to '0' after passing a maximum value of '4294967295' (i.e. maximum
>    value that can be represented using uint32 data type).  Such a
>    mechanism allows easy identification of lost or out-of-sequence
>    update records.
> 
> It's not really clear to me how much value there is in this counter
> mechanism if the client' can't rely on the server's behavior actually being
> to use a counter (the requirement is only "SHOULD").  Can this be a "MUST"
> (or maybe "MUST excpet when [...]") instead?
> 
> <ALEX> Would prefer to not introduce no MUST requirements at this point and
> keep this as a SHOULD.  </ALEX>

This is only a non-blocking comment, so go ahead.

> Section 3.9
> 
>                                                           Empty "push-
>    change-update" messages (in case of an on-change subscription) MUST
>    NOT be sent.  This is required to ensure that clients cannot
>    surreptitiously monitor objects that they do not have access to via
>    carefully crafted selection filters.  By the same token, changes to
>    objects that are filtered MUST NOT affect any dampening intervals.
> 
> I appreciate this attention to security-relevant detail; thank you!
> 
> <ALEX> thanks! </ALEX>
> 
> Section 3.11.1
> 
> Do we want to give any guidance for the "incomplete-update" case about
> whether a subscriber should wait and give the publisher a chance to provide
> a full "push-update" for resynchronization (per Section 4.3.2) versus
> perform a normal query for the datastore contents and effectuating its own
> resynchronization?
> 
> <ALEX> This should be up to the subscriber to decide, and what to do may
> differ for different applications.  In some instances, it can afford to
> simply wait; in other cases or when specifically interested in one
> particular closely-monitored item it may decide not to and sync immediately.
> </ALEX>

Okay.

> Section 4
> 
>    o  For on-change subscriptions, assuming any dampening period has
>       completed, triggering occurs whenever a change in the subscribed
>       information is detected.  On-change subscriptions have more
>       complex semantics that is guided by its own set of parameters:
> 
> nit: singular/plural mismatch "semantics"/"is"
> 
> <ALEX> changed to "On-change subscriptions have more complex semantics that
> are guided by their own set of parameters" </ALEX>
> 
> Section 4.3.2
> 
> 
>    A "time-of-update" which represents the time an update record
>    snapshot was generated.  A receiver MAY assume that at this point in
>    time a publisher's objects have the values that were pushed.
> 
> nit: I think "had the values" (past tense) is more appropriate.
> 
> <ALEX> changed </ALEX>
> 
> Section 4.4.1
> 
>    a publisher that cannot serve on-change updates but periodic updates
>    might return the following NETCONF response:
> 
> nit: "but can serve periodic updates"
> 
> <ALEX> changed </ALEX>
> 
> Section 4.4.2
> 
>    The specific parameters to be returned in as part of the RPC error
>    response depend on the specific transport that is used to manage the
>    subscription.  For NETCONF, those parameters are specified in
>    [I-D.draft-ietf-netconf-netconf-event-notifications].
> 
> nit: "in" and "as part of" are redundant.
> 
> <ALEX> removed "in" </ALEX>
> 
> Section 5
> 
> It is slightly interesting to note that (apparently) the
> update-policy-modifiable grouping allows for a subscription to switch
> between periodic and triggered at runtime (by virtue of wanting a single
> grouping to cover all the cases and needing to be able to modify the
> parameters for each case).  I would mostly expect implementations to deny
> such modification requests due to the needed complexity, but I'm also not
> sure that there's a need to mention this explicitly in the document.
> 
> <ALEX>  I think it is implied that a modification request can always be
> denied if not supported by a server.  At the same time, clearly in general
> you would not change between on-change and periodic.  Keeping as is. </ALEX>
> 
>             leaf period {
>               type centiseconds;
>               mandatory true;
>               description
>                 "Duration of time which should occur between periodic
>                  push updates, in one hundredths of a second.";
> 
> It would probably be okay to skip "in one hundredths of a second" given the
> usage of the centiseconds typedef.
> 
> <ALEX> doesn't hurt to be explicit; keeping as is </ALEX>
> 
>     rc:yang-data resync-subscription-error {
>       container resync-subscription-error {
>         description
>           "If a 'resync-subscription' RPC fails, the subscription is
>            not resynced and the RPC error response MUST indicate the
>            reason for this failure.  This yang-data MAY be inserted as
>            structured data within a subscription's RPC error response
>            to indicate the failure reason.";
> 
> It's a little weird to have the normative language here constraining the RPC
> error response that must be returned for a specific RPC, since this is not
> the description of that RPC.  (It's probably also duplicating langauge
> elsewhere but I didn't double-check.)
> 
> <ALEX> Keeping as is since it is clear what is happening.  Note that there
> are limitations to the RPC semantics you can formally define in YANG, hence
> the description. </ALEX>
> 
>     rc:yang-data establish-subscription-datastore-error-info {
>       container establish-subscription-datastore-error-info {
>         description
>           "If any 'establish-subscription' RPC parameters are
>            unsupportable against the datastore, a subscription is not
>            created and the RPC error response MUST indicate the reason
>            why the subscription failed to be created.  This yang-data
>            MAY be inserted as structured data within a subscription's
>            RPC error response to indicate the failure reason.  This
>            yang-data MUST be inserted if hints are to be provided back
>            to the subscriber.";
> 
> (ditto)
> 
> Contrast to modify-subscription-datastore-error-info, which only has
> normative language about the yang-data being described and not the RPCs that
> (might) use it.
> 
> <ALEX> Would keep as is.  I think the semantics are clear.  </ALEX>

You can certainly keep it as-is and I won't complain further; I just want
to note that my complaint was not "the semantics are unclear if I read the
whole document" but rather "the semantics are described in a different
place than a naive reader might expect".  Yes, the reader is generally
going to have to look up the description of the data types involved in the
RPC as well as the RPC description itself to actually use it, but I can
imagine some cases where that doesn't work as well as it could.

> nit: push-update and push-change-update use different langauge about "does
> not constitute a general-purpose notification" and I'm not sure there's a
> reason to diverge.
> Their "incomplete-update" leaves also have divergent descriptions, but I
> think that latter divergence is more reasonable.
> 
> <ALEX> Updated the description of push-change-update to align this part of
> the description of the notifications.  </ALEX>

Thanks!

-Ben

>     augment "/sn:subscription-modified/sn:target" {
>       [...]
>       case datastore {
>         uses datastore-criteria {
>           refine "selection-filter/within-subscription" {
>             description
>               "Specifies where the selection filter, and where it came
>                from within the subscription and then populated within
>                this notification.  If the 'selection-filter-ref' is
> 
> nit: "where the selection filter" seems like an incomplete clause.
> 
> <ALEX> Updating this to  "Specifies the selection filter and where it
> originated  from.  If the 'selection-filter-ref' is ...".  This way, the
> description also aligns better with the augment to
> sn:subscription-started/sn:target. </ALEX>
> _______________________________________________
> netconf mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/netconf
>