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
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ netmod mailing list netmod@ietf.org https://www.ietf.org/mailman/listinfo/netmod