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

[Alexander Clemm <[e-mail]>]

Hi Benjamin, all,

we have posted -25 to address your final comment.  -25 adds a reference 
to draft-ietf-netconf-notification-capabilities. Also, in section 3.10 
(On-Change Notifiable), the 3rd paragraph has been updated as follows: 
"Implementations are therefore strongly advised to provide a solution to 
this problem. One solution might involve making discoverable to clients 
which objects are on-change notifiable, specified using another YANG 
data model.  Such a solution is specified in <xref 
target="I-D.draft-ietf-netconf-notification-capabilities"/>. Until this 
solution is standardized, implementations SHOULD provide their own 
solution."

Thanks
--- Alex

On 5/15/2019 11:59 AM, Alexander Clemm wrote:
> Hi,
> Responses inline, <ALEX2>
> Thanks
> --- Alex
>
> -----Original Message-----
> From: Benjamin Kaduk <[e-mail]>
> Sent: Wednesday, May 15, 2019 8:00 AM
> To: Alexander Clemm <[e-mail]>
> Cc: 'The IESG' <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [netconf] Benjamin Kaduk's Discuss on
> draft-ietf-netconf-yang-push-23: (with DISCUSS and COMMENT)
>
> 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-capa
>> biliti
>> 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.)
>
> <ALEX2> To give a bit of history, in a set of earlier revisions we did
> describe a mechanism for this.  There was quite a bit of discussion around
> this and various options were considered, from introducing a YANG
> extension
> (see e.g. rev 12 section 3.10) to the mechanism that is now described in
> the
> separate draft.  There were also discussions whether such mechanism should
> be generalized further - for example, what if you wanted to express not
> only
> whether an item supports on-change subscriptions, but wants to give an
> indication of the "size" of the change?  The WG ultimately decided to not
> overburden the draft and slow progress down further and address this item
> separately.  There was also at some point discussion about keeping it
> simpler and take an all-or-nothing approach (require a server to either
> support on-change for all objects or none), which however was deemed to be
> too restrictive and we did want to allow for the possibility of a
> differentiated approach.
> Bottom line, I would like to ask please accept the previous decision.
> </ALEX2>
>
>> ----------------------------------------------------------------------
>> 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.
>
> <ALEX2> Sure.  Kept as is for now. </ALEX2>
>
>> 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.)
>
> <ALEX2> I think you are misreading this. There is no update action or copy
> operation that is being applied to a datastore node.  No operation or
> action
> is mentioned.  Instead, as the definition says, this is merely a data item
> which can be put into the stream of push update notifications.
> </ALEX2>
>
>> 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.
>
> <ALEX2> Thank you </ALEX2>
>
>> 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.
>
> <ALEX2> Thanks; your comment is well taken and I understand where you are
> coming from but let's keep it then. </ALEX2>
>> 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
>>