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

["Rob Wilton (rwilton)" <[e-mail]>]

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


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

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?


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?


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


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.


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. 


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

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


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


11.
Sec: 2.2 Examples
(i) Should the module name start with example-acme-system-ext and acme.example.com

(ii) Would it be helpful for some/all of the examples to also give the filename that
they would be expected to use?


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

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

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


(iv)
Should the example version of YANG library used match the mandatory to support version (in
the YANG module)?

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


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:



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

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

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"

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"

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

Thanks,
Rob