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

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

Thanks Robert,   inline....


From: Robert Wilton, September 11, 2018 5:34 AM


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".

<Eric> Works for me.




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 ..."


<eric> Agree.  I.e., I think we can make the changes needed for this text to become non-normative.



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).

<Eric> SHOULD is not appropriate here, as usually there will not be hints encapsulated.




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.

<Eric> Agree


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.

<Eric> We had that originally.  Other commenters requested the change be made so that the wrong error messages couldn’t possibly be delivered.


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.

<Eric> These are all possible.  But I am hoping what we currently have is sufficient.  (Right now we have limited the design to two tiers of identities.  It would be good not to add more.)

Eric



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