Subject: [Netconf] Yangdoctors last call review of draft-ietf-netconf-rfc7895bis-04

From: Reshad Rahman <[email-masked]>
Date: Mon, 19 Feb 2018 15:30:48 -0800

Reviewer: Reshad Rahman
Review result: Ready with Nits

YANG Doctor review of draft-ietf-netconf-rfc7895bis-04 by Reshad Rahman.

1 module in this draft:
- [email-masked]

YANG validation on 2018-02-18 reports 2 errors but no warnings. This comes from
yanglint 0.14.69 and seems to be a tool issue: err : Module "ietf-yang-library"
in another revision already implemented. err : Module "ietf-yang-library"
parsing failed.

2 examples are provided in this draft (Appendices B and C).

Module [email-masked]:
- typedef revision-identifier. The definition allows invalid dates such as
2018-99-99, has there been any consideration to use a more restrictive
definition, e.g. ‘\d{4}-[0-1][0-9]-[0-3][0-9]’ (I realize this still allows
incorrect dates)?  Also the revision timestamp is just a date which follows
YYYY-MM-DD format, isn’t there an existing common typedef (as there is for
date-and-time) which can be reused? - grouping implementation-parameters should
be renamed module-implementation-parameters? - The groupings defined are used
only once. For yang-library-parameters I see why this is done since it can be
reused. But for others such as implementation-parameters, I don’t see why a
grouping is needed. Maybe this is just authors’ preference to regroup
information. - Description of leaf module in list deviation: by
“self-referential” I assume that this means that the reference can not refer to
the ietf-yang-library module? While this may seem obvious I believe it’d be
good to spell it out. - list module-set. Since this is for schemas, consider
renaming it to something along the lines of schema-module-set? - Use of
“non-import module” in a few descriptions. Should this be “non import-only
modules”?

Nits:
- Section 1, 2nd paragraph s/informaton/information/
- Section 1, 3rd paragraph, add “” quote around /module-states.
- Section 1, 5th paragraph s/Since the YANG library contents changes/Since the
YANG library contents change/? - Section 4, 2nd paragraph “…that individual
datastore schema share..”. Should that be “… that individual datastore schemas
share..”? - Appendix B and C. The examples use YANG models which are still
being updated (hence the note to RFC Editor). Why not use a YANG model which is
stable, is this because of need to have an NMDA compliant module in the
examples?

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Wed, 21 Feb 2018 17:18:07 +0100

Reshad,

thanks for your review. Comments inline...

> in another revision already implemented. err : Module "ietf-yang-library"

Yes, I did run into this as well when I tried to use yanglint to
validate the examples. Its a tool issue which is specific to this
module (hence perhaps not worth fixing).

> date-and-time) which can be reused?

It is always somewhat subjective how restrictive you make a pattern.
Note that the pattern for the date-and-time typedef in RFC 6991 also
uses '\d{4}-\d{2}-\d{2}' so I think we are consistent. Yes, there is
no common 'date' type yet - hence we define things inline.

> - grouping implementation-parameters should be renamed
> module-implementation-parameters?

This name change may make sense.

> - The groupings defined are used only once. For
> yang-library-parameters I see why this is done since it can be
> reused. But for others such as implementation-parameters, I don’t
> see why a grouping is needed. Maybe this is just authors’ preference
> to regroup information.

I am not sure which modules would be using these groupings and whether
it is necessary to provide these groupings. I know that some versions
of schema mount used some of the groupings but this may not be
necessary anymore with the new library structure. I assume Martin
knows more details about which groupings may be useful to provide for
possible reuse reasons.

> - Description of leaf module in list deviation: by
> “self-referential” I assume that this means that the reference can
> not refer to the ietf-yang-library module? While this may seem
> obvious I believe it’d be good to spell it out.

No, the intention was to say that a module should not list itself as a
deviation module, neither directly or indirectly. I guess we have to
describe this better. Perhaps something along these lines:

        description
          "A module that deviates the module associated with this
           entry. This reference MUST NOT (directly or indirectly)
           refer to the module being deviated.

           Clients, MUST make sure that they handle a situation
           where a module deviates itself (directly or indirectly)
           gracefully.";

> - list module-set. Since this is for schemas, consider renaming it
> to something along the lines of schema-module-set?

While modules sets are used for schemas, I do not see why a list name
should be selected to constrain this. Actually, changing the name does
not do anything other than changing the name. ;-)

> - Use of “non-import module” in a few descriptions. Should this be
> “non import-only modules”?

Yes, I have changed the sources.

> - Section 1, 2nd paragraph s/informaton/information/

fixed

> - Section 1, 3rd paragraph, add “” quote around /module-states.

fixed

> - Section 1, 5th paragraph s/Since the YANG library contents
> changes/Since the YANG library contents change/?

I would leave this to the RFC editor; is 'content' countable here or
not, should it be plural or singular? 

> - Section 4, 2nd paragraph “…that individual datastore schema
> share..”. Should that be “… that individual datastore schemas
> share..”?

I added a plural 's' but again I am happy to let the RFC editor decide
plural form here.

- Appendix B and C. The examples use YANG models which are still
> stable, is this because of need to have an NMDA compliant module in the

These models are NMDA versions and they are in the RFC editor queue so
I think this is (a) a reason for using them and (b) the references
will be RFCs by the time this document appears.

/js

-- 
Juergen Schoenwaelder           Jacobs University Bremen gGmbH
Phone: +49 421 200 3587         Campus Ring 1 | 28759 Bremen | Germany
Fax:   +49 421 200 3103         <https://www.jacobs-university.de/>

---

From: Martin Bjorklund <[email-masked]>
Date: Thu, 22 Feb 2018 09:37:55 +0100 (CET)

Hi,

> On Mon, Feb 19, 2018 at 03:30:48PM -0800, Reshad Rahman wrote:

[...]

> This name change may make sense.

I agree.

> knows more details about which groupings may be useful to provide for

The addition of these groupings was requested by someone in the WG b/c
they wanted to be able to build some "module-related" data structures
w/o the implementation details, IIRC.

>            where a module deviates itself (directly or indirectly)

But if the first paragraph says that servers MUST NOT do X, why should
we say that clients MUST handle not(X)?

I think we should use your new first paragraph and remove the second.

/martin

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Thu, 22 Feb 2018 10:24:08 +0100

> I think we should use your new first paragraph and remove the second.

I think it would be bad if clients go into an endless loop because a
server failed to implement the MUST correctly. Perhaps we should word
it differently:

         Robust clients way want to make sure that they handle a
         situation where a module deviates itself (directly or
         indirectly) gracefully.

Or we remove the text and add a statement to the security
considerations that clients should be robust and not go into an
endless loop in case a server fails to implement the MUST above
correctly.

Or we say nothing because no server will ever get this wrong.

/js

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Fri, 23 Feb 2018 23:12:11 +0000

Hi,

I agree with your responses (I realize the self-referential question is being discussed with Martin). 

I think it'd be good to have a common type for date so that it doesn't have to be re-defined in multiple places. But this is orthogonal to the discussion on this document.

Regards,
Reshad.

On 2018-02-21, 11:18 AM, "Juergen Schoenwaelder" <[email-masked]> wrote:

    Reshad,

    

    

    

    
            description

    

    
    
    fixed

    fixed

    

    
    
    /js