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

From: Robert Wilton <[email-masked]>
Date: Mon, 10 Sep 2018 11:18:56 +0100

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

2. Sec 4.4.1, after Figure 7, "MAY respond" => "may respond". This is 
just an example from that normative behaviour.

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.

4. Sec 4.4.1, "Optionally, an "error-severity" of "error" (this MAY but 
does nothave 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.

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.

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

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.

8. Sec 4.4.4, no figure number under the example.

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.

YANG module:

10. Top level description statement, it looks like the copyright has 
accidentally been indented by 4 chars.

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.

12. "synchronization-size" doesn't sound like an error, perhaps 
"synchronization-too-big" or "resync-too-big" might be better?

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.

Thanks,
Rob

---

From: "Eric Voit (evoit)" <[email-masked]>
Date: Mon, 10 Sep 2018 17:19:24 +0000

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

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.

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.

Old:

NEW:

   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.

<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:

NEW:

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

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.

Eric

Thanks,
Rob

---

From: Robert Wilton <[email-masked]>
Date: Tue, 11 Sep 2018 10:34:28 +0100

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:
> *From:*Robert Wilton, September 10, 2018 6:19 AM
>
> Hi Alex, authors,
>
> 1. Sec 4.3.2, paragraph 2.  "resulted in a particular push" => 
> "resulted in a particular update record"?
>
> 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 
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.
>
> 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 ..."

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

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

> <eric> I have no problem with changing it to 
> "synchronization-too-big", that aligns to the other identity 
> "update-too-big"
>
>
>
> Type definitions:
>
>

Thanks,
Rob

---

From: "Eric Voit (evoit)" <[email-masked]>
Date: Tue, 11 Sep 2018 15:25:13 +0000

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.

Hi Alex, authors,

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

In the case of NETCONF

      have to be included).

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.

Old:

NEW:

   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.

OLD:

NEW:

   subscription.

YANG module:

Identities:

Hum, OK.

<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

Type definitions:

Thanks,
Rob

Eric

Thanks,
Rob