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
_______________________________________________
netmod mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/netmod
