Re: [Netconf] RGW YANG push -18 LC comments [part 2]

[Robert Wilton <[e-mail]>]

Hi Eric,

Please see inline (RW:).  Most issues are closed, just a couple that 
require further resolution.


On 10/09/2018 18:19, Eric Voit (evoit) wrote:
>
> Hi Robert,
>
> Thanks for the comments.  In-line with <Eric>....
>
> *From:*Robert Wilton, September 10, 2018 6:19 AM
>
> Hi Alex, authors,
>
> Here are my remaining comments from my WG LC review of the YANG push 
> draft.  Again, I think that these are all minor.
>
> 1. Sec 4.3.2, paragraph 2.  "resulted in a particular push" => 
> "resulted in a particular update record"?
>
> <Eric> Agree, we should make this tweak.
>
>
>
> 2. Sec 4.4.1, after Figure 7, "MAY respond" => "may respond".  This is 
> just an example from that normative behaviour.
>
> <Eric> Agree, we should make this tweak.
>
>
>
> 3. Sec 4.4.1, paragraph starting "If a request is rejected ...".  Is 
> it really right for this to be "SHOULD" rather than "MAY"?  Early text 
> (e.g. 3.2. paragraph 2 uses "may be inserted".  Also in section 4.4.2, 
> paragraph after Figure 11.
>
> <Eric> I prefer SHOULD.  Section 3.2 text had “may” instead of “MAY”.
>
My reading of the text in 3.2, paragraph 2, is that the error 
information is some optional, whereas section 4.4.1 strongly encourages 
it.  I think that it would be better if these two were consistent, i.e. 
if you want to keep the "SHOULD" in 4.4.1, then I think that it would be 
better if the "may" in sec 3.2 was a "should".

>
>
> 4. Sec 4.4.1, "Optionally, an "error-severity" of "error" (this MAY 
> but does not have to be included).", I don't think that the last 
> clause is required (given that it is predicated by Optionally, so 
> suggest deleting the clause in parenthesis.  Otherwise it is also 
> inconsistent with the following "error-info" description. If this 
> change, it should also be changed for "error-severity" in Sec 4.4.2.
>
> <Eric> Agree, we should make this tweak.
>
>
>
> 5. Sec 4.4.1, NETCONF specific text about errors.  Should that text 
> really be in this draft?  Would it not be nicer to keep all of the 
> NETCONF specifics to a NETCONF protocol draft?  Perhaps this is not 
> practical, but the WG is currently experiencing some pain where 
> NETCONF protocol specific normative text is in RFC 7950.  Hence, it 
> would be nice to avoid introducing that issue here if possible.  This 
> same comment also applies to Sec 4.4.2.
>
> <Eric> As this is a non-normative example, we should not hit the same 
> issue.  I do like examples for readers to digest.
>
RW:
I was meaning this text:

In the case of NETCONF
    [I-D.draft-ietf-netconf-netconf-event-notifications], when a
    subscription request is rejected, the NETCONF RPC reply MUST include
    an "rpc-error" element with the following elements:

    o  "error-type" of "application".

    o  "error-tag" of "operation-failed".

    o  Optionally, an "error-severity" of "error" (this MAY but does not
       have to be included).

    o  Optionally, "error-info" containing XML-encoded data with hints
       for parameter settings that might result in future RPC success per
       yang-data definition "establish-subscription-error-datastore".


This text includes RFC 2119 language, so a reader would surely interpret 
that text as being normative.

If your want to be non-normative then I think that you need to replace 
the "reply MUST include" statement with "reply would be expected to 
include ", and prefix the text with, "For example, in the case of 
NETCONF ..."



>
>
> 6. Sec 4.4.2, description paragraph.  Suggest the following change to 
> match the actual example: "modify the "period" of a subscription." => 
> "modify the "period" and "datastore XPath filter" of a subscription."
>
> <Eric> Agree, we should make this tweak.
>
>
>
> 7. Sec 4.4.2, first paragraph after figure 11:  Possible rewording:
>
> Old:
>
>    The publisher MUST respond explicitly positively or negatively to the
>    request.  If the subscription modification is rejected, the
>    subscription is maintained as it was before the modification request.
>    In addition, the publisher MUST send an RPC error response.  This RPC
>    error response SHOULD contain hints encapsulated within the yang-data
>    structure "modify-subscription-error-datastore".  A subscription MAY
>    be modified multiple times.
>
>
> NEW:
>
>    The publisher MUST respond to the subscription modification request.
>    If the request is rejected, the existing subscription is left 
> unchanged, and
>    the publisher MUST send an RPC error response, which MAY|SHOULD contain
>    hints encapsulated within the yang-data structure
>    "modify-subscription-error-datastore".  A subscription MAY be modified
>    multiple times.
>
> <Eric> Proposed text looks fine, with a small tweak:
>
> The publisher MUST respond to the subscription modification request. 
>  If the request is rejected, the existing subscription is left 
> unchanged, and the publisher MUST send an RPC error response.  This 
> response might have hints encapsulated within the yang-data structure 
> "modify-subscription-error-datastore".  A subscription MAY be modified 
> multiple times.
>
RW:
I think that "SHOULD" it better than "might" for consistency, as per the 
same discussion point above (which is why my original text was 
MAY|SHOULD, i.e. only a single one should be used depending on how issue 
3 above got resolved).

>
> 8. Sec 4.4.4, no figure number under the example.
>
> <Eric> Agree.  Alex, can you tweak the XML?
>
>
>
> 9. Sec 4.4.5, paragraph 1:
> I think that it would be better if this just referred to YANG library, 
> hence suggest changing the first two paragraphs:
>
> OLD:
>
>    To make subscription requests, the subscriber needs to know the YANG
>    module library available on the publisher.  The YANG 1 module library
>    information is sent by a NETCONF server in the NETCONF "hello"
>    message.  For YANG 1.1 modules and all modules used with the RESTCONF
>    [RFC8040] protocol, this information is provided by the YANG Library
>    module (ietf-yang-library.yang from [RFC7895].  This YANG library
>    information is important for the receiver to reproduce the set of
>    object definitions used within the publisher.
>    The YANG library includes a module list with the name, revision,
>    enabled features, and applied deviations for each YANG module
>    implemented by the publisher.  The receiver is expected to know the
>    YANG library information before starting a subscription.
>
>
> NEW:
>
>    To make subscription requests, the subscriber needs to know the YANG
>    datastore schemas used by the publisher, which are available via 
> the YANG
>    Library module, ietf-yang-library.yang from [RFC7895].  The receiver
>    is expected to know the YANG library information before starting a
>    subscription.
>
> <Eric> Agree, we should make this tweak.
>
>
> YANG module:
>
> 10. Top level description statement, it looks like the copyright has 
> accidentally been indented by 4 chars.
>
> <Eric> Agree, we should make this tweak.
>
>
>
> Identities:
> 11. I suggest renaming "no-such-subscription-resynch" to just 
> "no-such-subscription".  I would have thought that this error could 
> also be returned to cancel an unknown subscription.
>
> <eric> The reason we need "no-such-subscription-resynch" is that 
> "no-such-subscription" is already used in SN.  (I.e., is not possible 
> to augment the existing identity with a new base.)
>
RW:

Hum, OK.

It is somewhat of a shame that the original error can't just be used here.

I wonder if the using multiple bases for the different subscription 
errors isn't trying to be too clever.  I.e. I wonder whether it would be 
better to just have a single base identity for subscription errors.

An alternative solution would be to define the "resync-subscription" rpc 
in the base subscribed notifications draft, and then make use of that 
RPC here.  I think that it is slightly plausible that a 
resync-subscription could still be useful in the case of a periodic 
subscription.  E.g. perhaps some client size buffers get overloaded so 
the client has to disregard the request.

But this is a minor wart, having a separate name isn't the end of the 
world.  Making "no-such-subscription-resync" also have 
"no-such-subscription" as a base might also be sensible.

>
>
> 12. "synchronization-size" doesn't sound like an error, perhaps 
> "synchronization-too-big" or "resync-too-big" might be better?
>
> <eric> I have no problem with changing it to 
> "synchronization-too-big", that aligns to the other identity 
> "update-too-big"
>
>
>
> Type definitions:
>
>
> 13.  Change all instances (create, delete, insert, move, replace)  of 
> "data node" to "datastore node".
>
> 14. grouping selection-filter-types, anydata datastore-subtree-filter, 
> perhaps add a reference to "RFC 6241: Network Configuration Protocol, 
> Section 6.", mirroring draft-ietf-netconf-nmda-netconf.
>
> 15. grouping update-policy-modifiable, container on-change description 
> statement, needs to be indented by 2 spaces.
>
> 16. grouping hints, description, indent the second description line by 
> 1 space.
>
> 17. rpc resynch-subscription, description statement, last two lines 
> require alignment.
>
> <eric> I think all five suggestions above are good tweaks.
>

Thanks,
Rob


> Eric
>
>
>
> Thanks,
> Rob
>