[netmod] FW: AD review of draft-ietf-netmod-yang-instance-file-format

[Balázs Lengyel <[e-mail]>]

Hello Rob,
Thanks for the review.  Here are my answers below. I will also upload the
new version asap.
Regards Balazs
-----------------------------------------------------------
Hi,

Here is my AD review of draft-ietf-netmod-yang-instance-file-format-13.

Thanks for this document, I think that it represents important useful work
for advancing the YANG ecosystem.

This document is in good shape, and I mostly have minor comments but with a
few more significant comments.

Main comments:

1.
   An instance data set MAY contain data for any number of YANG modules;
   if needed it MAY carry the complete configuration and state data for
   a server.  Default values SHOULD NOT be included.

This document recommends that default values SHOULD NOT be included, but
there are cases where they are useful, and e.g., NMDA recommends that
"in-use" values (effectively including default values) are returned.
Further, there is no way for a consumer of the file to know whether default
values are included or not.

Hence, I would recommend that the instance-data-set defines an
"includes-defaults" leaf that indicates whether default values are included
in the dataset, the leaf can default to them not being included in the
dataset.

Further, I would suggest weakening "Default values SHOULD NOT be included"
to something like:
"Default values should be excluded where they do not provide additional
useful data."
BALAZS: Section 2 states that a default attribute may be specified,
following the with-defaults capability. 
However, your proposal is better. I will include it. 
Added leaf includes-defaults.

2.
In the YANG Module:
     feature inline-content-schema {
       description
         "This feature indicates that inline content-schema
          option is supported. Support for this feature might
          be documented only via out-of-band documentation.";
     }
	 
What is the benefit of having 'inline-content-schema' as a feature?  It
seems to potentially add complexity without any benefit, given that the
device originating the instance data file would effectively choose whether
to use the inline-content-schema, hence I suggest that it might be simpler
just to remove the feature definition.
BALAZS: This was explicitly requested earlier by a reviewer (Andy ?).
The system can declare supported/not-supported in design documentation.
In a use-case when a client or a design department is sending data to a
server this is needed. E.g. in UC2, Preloading Default Configuration the
designer preparing instance data, can decide to use or not use the
inline-content-schema based on this.
   
3.
In the YANG Module:

	"case inline", description:
                    The first item is either ietf-yang-library or
                    some other YANG module that contains a list of
                    YANG modules with their name, revision-date,
                    supported-features, and deviations.
                    The usage of revision '2019-01-04' of the
                    'ietf-yang-library' module MUST be supported.
                    Using other modules, module versions MAY also
                    be supported.

This seems to make interop for consumers of instance data files hard, since
the schema can be defined by any arbitrary YANG module without updating this
module.  I would suggest that it is safer to limit this to the two currently
published versions of YANG library.
BALAZS:  I fully agree, however this was explicitly requested by some
reviewer earlier (Juergen ?) Shall I simplify this or not?

If additional modules are supported in future, then I think that it would be
safer to create a new version of this YANG module that documents what other
module formats can be used.


4.
In the YANG Module:
	list "revision"

Is revision expected to be unique, if provided? If so, should this be
explicitly stated in the YANG module description?
BALAZS: I don't think I understand your comment. There may be multiple list
entries for revision. The 'leaf date' is a key, so it is inherently unique.
The description may or may not be unique.


5.
In the YANG Module:

Is an instance-data file allowed to contain both a revision and also a
timestamp?  If so, is there any constraints on the values.  If not, then
would it make sense to put them under a choice?
BALAZS:  It is allowed to have both. There is some recommendation text about
when to use each. However I can see some corner cases, when using both in
the same file would be useful, E.g. we want a timestamp including hour,
minutes, but we also want the history of the instance data set, including
multible revision/descriptions.
I propose to add: if both are included the timestamp, SHOULD contain
the same date as the latest revision statement.


6.
References:
- RFC 6020 needs to be normative for the IANA YANG module registration.
BALAZS: OK

Minor comments:

7. 
Abstract:

   There is a need to document data defined in YANG models when a live
   server is unavailable.  Data is often needed at design or
   implementation time or needed when a live running server is
   unavailable.  This document specifies a standard file format for YANG
   instance data, which follows the syntax and semantics of existing
   YANG models, and annotates it with metadata.
   
I suggest combining the first 2 sentences to:

There is a need to document data defined in YANG models at design,
implementation time or when a live server is unavailable.
BALAZS: OK

8.
Sec 1. Introduction:
I suggest tweaking 2nd sentence to:
	Data is often needed at design, implementation time, or when a live
	running server is unavailable. 
BALAZS: OK

9.   
Sec 2. Instance Data File Format:

   o  a default attribute as defined in [RFC6243] section 6. and in
      [RFC8040] section 4.8.9.
	  
I would suggest putting default in quotes.  E.g., "a 'default' attribute, as
defined ...".
BALAZS: OK

For the two other bullets in the list, it might read better as "metadata, as
defined ..." and "origin metadata, as specified ..."
BALAZS: OK

10.
Sec 2. Instance Data File Format:
	instance-data-set-name ['@' ( revision-date / timestamp ) ]
(i) Possibly helpful to clarify that the revision-date and timestamp take
the same format  as they are encoded in the equivalent leaves in the YANG
file. 
(ii) Would it be helpful to include an example without a revision-date?
(iii) I also note that no recommendation is made as to whether a date or
timestamp is
   included (which I think is okay).
BALAZS:  OK, Added text for (i). Added example for (ii)


11.
Sec: 2.2 Examples
(i) Should the module name start with example-acme-system-ext and
acme.example.com
BALAZS: IMHO the use of "acme" as a company name already indicates this is
an example. "E.g." and the word "example" is also included in the text in a
number of places.

(ii) Would it be helpful for some/all of the examples to also give the
filename that they would be expected to use?
BALAZS: OK. Actually, they already have filenames, but it is only visible in
the XML not in the TXT format. If you export the files with rfcstrip these
filenames will be used. I added the filenames into the text too.


12.
YANG Module:
(i) I note that the module only indicates Balazs as an author and not Benoit
(which 
   differs from the draft authors).
BALAZS: OK. added Benoit

(ii) "format-version"
Please ensure that there is an RFC editor note to update this value if the
module gets changed in anyway during reviews or RFC editor process.
BALAZS: OK. added

(iii)
"case simplified-inline", description:

                  The value SHALL start with the module name.
                  If the module contains a revision statement the
                  revision date SHALL be included in the leaf-list
                  entry. If other methods (e.g., revision-label) are
                  defined to identify individual module revisions
                  those MAY be used instead of using a revision date.

I wonder whether it would be clearer to merge the second and third sentences
into one:

                  The value SHALL start with the module name.
                  If the module contains a revision statement the
                  revision date SHALL be included in the leaf-list
                  entry, unless other methods (e.g., revision-label) are
                  defined to identify individual module revisions, when
                  those MAY be used instead of using a revision date.


If you agree to this change, then please update the equivalent text in the
inline case as well.				  
BALAZS: OK, changed

(iv)
Should the example version of YANG library used match the mandatory to
support version (in the YANG module)?
BALAZS: OK, changed. But I will still use the old modules-state branch
because its simpler

(v)
I also note that the content schema case statement order differs from how
they are described earlier in the document.  Does it make sense to align
them?  Is one of the content-schema-specs the default approach, i.e., does
it make sense to specify a default for the choice statement?
BALAZS: IMO default should not be specified. In chapter 2.1 we have:
"External Method: Do not include the "content-schema" node; the
      user needs to obtain the information through external documents."
In this case, none of the "cases" are used.
Reordered items in section 2.1. IMHO the simplified inline will be the most
used method.


13.
IANA section

	5.2.  YANG Module Name Registration

	   This document registers one YANG module in the YANG Module Names
	   registry [RFC6020].
   
Please change to:
   
   This document registers a YANG module in the YANG Module Names
   registry [RFC6020].  Following the format in [RFC6020], the following
   registrations are requested:
BALAZS: OK, changed


Grammar Warnings (generated, by tool):
Section: 2, draft text:
Later as other YANG encodings (e.g., CBOR) are defined, further instance
data formats may be specified. 
Warning:  Comparison requires than, not 'then' nor 'as'.
Suggested change:  "Later, as other ..."
BALAZS: OK

Section: 2.1.1, draft text:
 The anydata inline-schema carries instance data (conforming to the
inline-modules) that actually specifies the content defining YANG modules
including revision, supported features, deviations and any relevant
additional data (e.g., revision labels, described by [REF]) as alternative
to the revision date). 
Warning:  Unpaired symbol: '(' seems to be missing
BALAZS: OK, actually there was one to many ")"

Section: 4, draft text:
Because of this the the security considerations template for YANG models in
section 3.7.1 in [REF] is not followed. 
Warning:  Maybe you need to remove one determiner so that only the or the is
left.
Suggested change:  "the"
BALAZS: OK

Section: 4, draft text:
Depending on the nature of the instance data, instance data files MAY need
to be handled in a secure way. 
Warning:  Consider replacing this phrase with the adverb securely to avoid
wordiness.
Suggested change:  "securely"
BALAZS: OK

Section: C.1, draft text:
Server capabilities include: 
-  data defined in "ietf-yang-library": YANG modules, submodules, features,
deviations, schema-mounts, and datastores supported ([REF])
- alarms supported ([REF])
- data nodes and subtrees that support or do not support on-change
notifications ([REF])
- netconf-capabilities in ietf-netconf-monitoring


Warning:  Please add a punctuation mark at the end of paragraph.
Suggested change:  "ietf-netconf-monitoring."
BALAZS: OK
Thanks,
Rob