Subject: Re: [netmod] Shepherd review on draft-ietf-netmod-yang-instance-file-format-10

From: Kent Watsen <[email-masked]>
Date: Fri, 2 Apr 2021 00:19:31 +0000

[CC-ing NETMOD]

Hi Balazs,

Please see below for responses.  (One question pending)

I just got back from PTO.  Thanks for posting -13 anyways.

Kent // shepherd

> On Mar 23, 2021, at 6:01 PM, Balázs Lengyel <[email-masked]> wrote:
> 
> Hello Kent,
> I am resuming my work on draft-ietf-netmod-yang-instance-file-format. 
> I removed already agreed items from this mail and provided answers as  BALAZS3 below.
> Are you OK with my answers, corrections? What is the next step forward?
>  
> I updated the draft to -13, but did not upload it yet. 
> Regards Balazs
>  
>  
> From: Kent Watsen <[email-masked] <mailto:[email-masked]>> 
> Sent: 2020. április 9., csütörtök 22:11
> To: Balázs Lengyel <[email-masked] <mailto:[email-masked]>>
> Cc: [email-masked] <mailto:[email-masked]>
> Subject: Re: [netmod] Shepherd review on draft-ietf-netmod-yang-instance-file-format-10
>  
> Hi Balazs,
>  
> 
> 
> On Apr 8, 2020, at 8:06 AM, Balázs Lengyel <[email-masked] <mailto:[email-masked]>> wrote:
>  
> Hello Kent,
> Thanks for the review. See answers below.
> I tried to address all you comments, sorry if I missed something. 
> I updated the draft and uploaded a -11 version. Please check/advance it.
> 
> One question I could not settle: XML2RFC does not accept
>      <?rfc include='reference.I-D.netmod-yang-module-versioning'?>      
> Only
>      <?rfc include='reference.I-D.verdt-netmod-yang-module-versioning'?>      
> Why ? Please help.
>  
> Because it’s a working group document now and so uses the “ietf” prefix.  Try this:
>  
>      <?rfc include='reference.I-D.ietf-netmod-yang-module-versioning'?>  
>  
> BALAZS3: OK
> 
> Structural Issues:
> 
> - S5 contains an mix of important and unimportant information.   I think that the most important thing to state that the module defines an offline format that MAY contain security sensitive information, and thus safe handling is advised.  Maybe also say something about because the YANG module only defines a “structure”,  the Security Considerations doesn’t follow the template specified in https://tools.ietf.org/html/rfc8407#section-3.7.1 <https://tools.ietf.org/html/rfc8407#section-3.7.1>).  For instance: s/is designed as a wrapper specifying a format and a metadata header for YANG instance data defined by the content-schema/specifies an offline format/
> BALAZS: Most of text was required to be put there by earlier reviewers (Mostly Juergen and Acee Lindem) and sent to the mailing list.
> I added that we do not follow the security template for YANG models.
>  
> Please add the reference to https://tools.ietf.org/html/rfc8407#section-3.7.1 <https://tools.ietf.org/html/rfc8407#section-3.7.1> per above.
>  
> BALAZS3: OK

Thanks but, FWIW, this change must’ve been made before -13 since it doesn’t show up in the 12-to-13 diff.

>  - S8.1: agreed that RFC8525 is Normative, but the only place it it referenced is in a non-normative section…please add a ref to it from a normative section.
> BALAZS: It is referenced from the YANG module which is normative.
>  
> You just added that reference, but not correctly:
>   1) the “reference” doesn’t follow the standard format
>   2) the paragraph at the top of 3.2 doesn’t also list RFC 8525
>  
> BALAZS3: ???

Thanks, I see the reference to RFC 8525 in Section 3.2 now.  [Also must’ve been added before -13]

> 
> Editorial Issues:
> 
>  - Appendix B:
>     - s/For instance data/Instance data/
> BALAZS: Sorry, that would make the sentence incorrect.
>  
> Do you mean it to be “For instance, data” then?   If “instance data” is supposed to be read together, maybe use a hyphen or quotes?
> 
> BALAZS3: OK, changed 

Thanks, I see s/For/In case of/, which resolves the ambiguity.

> On Mar 30, 2020, at 3:22 PM, Kent Watsen <[email-masked] <mailto:[email-masked]>> wrote:
> 
> As part of the Shepherd writeup, I read the entire draft and found the following issues, which I’d like to see resolved before progressing the document.  Most of these issues should have been caught be the WG and/or Editors...
> 
>  
> - In S3, P8: “the semicolons and the decimal point, if present, shall be replaced by underscores” - why are they not escaped?
> BALAZS: This is a file name. Escaping in file names does not always work (depending on the filesystem and tools used). This is more simple and understandable
> No, this is a special case CLR and we never do this.  I see this idea has been in the document since -03, so it must’ve been discussed, can you point me to the discussion? 
>  
> FWIW, my OS doesn’t even require escaping colons.  BTW, they’re “colons” (not semicolons).
> BALAZS2: Windows doesn’t allow colons in the filename. Although it’s not everyone’s favorite OS, it is pretty widespread. 
>  
> Understood, but that doesn’t explain why escapes can’t work.  Please explain.
>  
> BALAZS3: To my best knowledge, colon is never accepted in any form in windows filenames and cannot be escaped. https://superuser.com/questions/461320/get-a-colon-in-a-windows-filename <https://superuser.com/questions/461320/get-a-colon-in-a-windows-filename>
> I removed the decimal point as that works on windows and Linux.
> 
> For Ubuntu Linux and a bash shell the colon is allowed, but tab extension does not work properly.
>  
> On Bash:
>   $ touch a:b
>   $ ls a<TAB> ---> replaces “a” with "a\:b” <RETURN>
>   a:b
>  
> Seems regular to me…what’s the problem?
> BALAZS3: On another version of Ubuntu tab extension did not work for me.

Thank you for providing the link explaining why colons cannot be escaped on Windows.

Personal opinion: the underscore usage seems user-unfriendly.  I almost seems better if the colons were simply removed.  The "silver lining” is that, presumably, the form using “timestamps” will be rare, and even more-so when “semver” names become the norm.

BTW, I assume that you wish to publish this as-is and follow-up later with an update to adjust for the “semver” work - correct?

> - It is unclear how the "inline-content-schema” feature could ever be used.  I.e., there are no protocol-accessible nodes in the module…
> BALAZS: The system can declare in supported/not-supported in design documentation. 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.
>  
> When I make statements like this, please see it as an opportunity to improve the document.  In this case, please modify the inline-content-schema’s “description” statement to indicate that the feature is never supported by a server, and that it is intended to be enabled via out-of-band documentation.  BTW, was this discussed by the WG?
> BALAZS3:  OK. Added out-of-band documentation. It was discussed and this was the proposed solution.  Some people insisted that this should be optional. This is the best way to indicate that.

Thanks, I see this:  [PS: also must’ve been in an update before -13]

       description
         "This feature indicates that inline content-schema
          option is supported. Support for this feature might
          be documented only via out-of-band documentation.";

> - "leaf-list inline-module" is "min-elements 1” and "ordered-by user”, but "leaf-list module” has neither (though it may be that ordering is irrelevant for simple-inline).
> BALAZS: ordered-by  removed. It doesn't really mean anything. In this case there is no chance of the system reordering a list a CLI/Netconf/Restconf client provided.
> Min-elements is not needed for simplified-inline as the case will only be selected if there is at least one "module" leaf-list entry. It is needed for inline because otherwise the case could contain an " inline-schema" anydata section and no "inline-module" entries. That would not be usable.
>  
> That may be true, but it’s equally true for the other leaf-list.  It's inconsistent.  
> BALAZS3:  OK. This is not strictly needed, but it improves readability. Added min-elements 1 to the other leaf-list. 

Thanks.

> BTW, is "choice content-schema-spec” meant to be “mandatory true”?  - because, currently, 'content-schema” doesn’t have to be specified according to the model…
> BALAZS3: It is optional. Section 2.1.  Specifying the Content Schema specifies this. E.g. if you are sending diagnostics data every 10 seconds with the same filename, it is not needed to include the content schema every time.

Okay.

> - The last two sentences of the “description” statement on line 207 in the YANG module contradict each other.
> BALAZS: Why ? I don't see the contradiction. If you know a single datastore specify it. If not omit the leaf. If the leaf is omitted, the situation is unknown.
>  
> I think the word “undefined” is throwing me.  Maybe “unspecified” would be better?
> BALAZS3: OK, use unspecified

Thanks.

> Structural issues:
>  
> - The list under "Metadata SHOULD include:” is not indented.
> BALAZS: OK, added
>  
> I don’t see it.  The way to do it is by adding a fake “list”, with missing symbols, to put the other list inside...
>  
> BALAZS3: It is now. Please check

Thanks!

> - The three examples should be <section> of their own (e.g., 3.2.x)
> BALAZS: OK
>  
> Better, but:
>   - the new titles don’t match the UC titles
>   - perhaps remove the “UCx,” prefix from the titles?  It looks weird in
>     the ToC and they're not needed in the title since the first sentence
>     relates the example to the UC already...
>   - BTW, missing word “in”:  s/The example illustrates UC[125] Section 1
>     /The example illustrates UC[125] in Section 1/
>  
> BALAZS3: OK

Thanks.

> - The “inline” choice node is generally confusing.  I can’t tell if it’s missing container called “inline” or if the two descendant nodes are poorly named.  In either case, it would be best to try to make it more readable.
> BALAZS: Yes it is complicated. Some members of Netmod (I think Rob W.) Asked for a full, powerful, flexible way of documenting the content schema. In some cases it is needed.
>  
> I’m not saying that it’s purpose is confusing, I’m saying that its poorly named or missing a parent container.  Try looking at your examples with “fresh” eyes.  The node names "inline-module” and “inline-schema” are odd.  It seems like “inline-module” could be “anydata-schema” and "inline-schema” could be “module-data”?
> BALAZS3: OK. Added container inline-content-schema

Thanks - better.

> Editorial issues:
>  
> - s/e.g., UC5 documenting diagnostic data/(e.g., UC5 [Section 2])/
> BALAZS: I prefer to use the short name of the use case instead of the reference. IMHO it provides information instantly without a look-up. Is that a problem?
>  
> I think I mentioned this above already, but the titles are wrong.  
>  
> Myself, I’d remove all the “Figure” postambles; I never title my figures, just more to have to look at and maintain.  In the case, this is where the UC titles are again incorrect...
> BALAZS3: OK, Figure postambles Removed.

Thanks.

> - S3.1.1 P2 doesn’t makes sense to me (esp. the verdt ref, which likely should be removed or better explained)
> BALAZS: This was explicitly requested by 2 members of the verdt team. I tried to amend the text to make it more understandable, however IMHO we should not try to explain the usage of revision label here. Also this is just an example.
>  
> OLD: 
>    (e.g., revision labels which can be used as alternative to the revision
>    date[I-D.verdt-netmod-yang-module-versioning]). 
>  
> NEW:
>     (e.g., revision labels, described by [I-D.verdt-netmod-yang-module-versioning]
>     as alternative to the revision date). 
> BALAZS3: OK

Thanks.

> BTW, immediately following, the text says "See Section 2.2.”   This doesn’t mean
> Anything to me.  Do you want to say something like “An example of the “inline” method is provided in 2.2.1”?
> BALAZS3: OK. Reference removed, replaced by a reference to an  example.

Okay.

> - s/is based on "UC1, Documenting Server Capabilities”/exemplifies UC1 [Section 2]/
> BALAZS: Exemplifying is an uncommon word I find ugly. Is the current text hard to understand or misleading? 
> I changed it to " The following example illustrates ..." I hope that's OK.
>  
> I’m unsure if it’s possible for something to be “based on” or “illustrate” a use case.  Illustrate is better though, maybe “reflects” or “epitomizes"?
> BALAZS3: OK, changed to reflects

Thanks.

> BTW, missing “in":  s/illustrates UC1 Section 1/illustrates UC1 in Section 1/
> BALAZS3: OK

Thanks.

> - s/- Use case 1, Documenting server capabilities/Exemplifying UC1/
> BALAZS: Exemplifying is an uncommon word I find ugly. Is the current text hard to understand or misleading?  
> IMHO the string stating  the name of the use case is more helpful then a reference, that needs to be looked up.
> I changed it to " The following example illustrates ..." I hope that's OK.
>  
> Same comment as above.
> BALAZS3: OK, changed to reflects

Thanks.

> - s/is based on "UC2, Preloading Default Configuration”/exemplifies UC2 [Section 2]/
> BALAZS: Exemplifying is an uncommon word I find ugly. Is the current text hard to understand or misleading?  
> I changed it to " The following example illustrates ..." I hope that's OK.
>  
> Same comment as above.
> BALAZS3: OK

Thanks.

> - s/- Use case 2, Preloading access control data/Exemplifying UC2/
> BALAZS: Exemplifying is an uncommon word I find ugly. Is the current text hard to understand or misleading?  
> IMHO the string stating  the name of the use case is more helpful then a reference, that needs to be looked up
>  
> Same comment as above.
> BALAZS3: OK

Thanks.

> >  - s/is based on UC5 Storing diagnostics data/exemplifies UC5 [Section 2]/
> BALAZS: OK. but I changed it to: exemplifies UC5, Storing diagnostics data. IMHO the string stating  the name of the use case is more helpful then a reference, that needs to be looked up.
> I changed it to " The following example illustrates "UC2, Preloading ..." I hope that's OK.
>  
> Same comment as above.
> BALAZS3: OK

Thanks.

> - s/- UC5 Storing diagnostics data/Exemplifying UC5/
> BALAZS: Exemplifying is an uncommon word I find ugly. Is the current text hard to understand or misleading?  
> IMHO the string stating  the name of the use case is more helpful then a reference, that needs to be looked up.>
> I changed it to " The following example illustrates ..." I hope that's OK.
>  
> Same comment as above.
> BALAZS3: OK

Thanks.

> - “description" statement on line 100: s/content schema/schema (i.e., YANG modules)/?
> BALAZS: The term "content-schema" is defined in the terminology section.  It defines  
>  
> Fine, but please add "(i.e., YANG modules)” so people will have better clue 
> BALAZS3: OK

Thanks.

> - “type string” statement on lines 109 and 131 are missing a “pattern" statement.
> BALAZS: OK, Defined it as a typedef.
>  
> Good!  But I’m unsure about the pattern statement (esp. "pattern '.|..|[^xX].*|.[^mM].*|..[^lL].*’;”)…did you copy/paste it from somewhere?
> BALAZS3: Copied from rfc6991. It is meant to ensure that an identifier does not start with the string xml.
>  typedef yang-identifier {
>       type string {
>         length "1..max";
>         pattern '[a-zA-Z_][a-zA-Z0-9\-_.]*';
>         pattern '.|..|[^xX].*|.[^mM].*|..[^lL].*';
>       }
> 

Okay.

> - P2 in the “description" statements on lines 220 and 249: s/For instance data sets/Instance data sets/
> BALAZS: The sentence will not make sense unless I change the comma at the end of sentence to a colon.
>  
> Hmmm, that didn’t come out very well.  This is the same issue as before, whereby “For instance data...” looks like it should be read “For instance, data…”.  Maybe you can find a better way to express this?
> 
> BALAZS3: OK, changed to
> In case of 'instance data sets ...

Thanks!

K.

---

From: Balázs Lengyel <[email-masked]>
Date: Wed, 7 Apr 2021 13:56:31 +0000

Hello Kent,

Thanks for the thorough review and sorry for my absence and hence the earlier slow response.

See answer below as BALAZS4. Removed already agreed parts.

What is the next step?

Regards Balazs

From: Kent Watsen <[email-masked]> 
Sent: 2021. április 2., péntek 2:20
To: Balázs Lengyel <[email-masked]>
Cc: [email-masked]
Subject: Re: [netmod] Shepherd review on draft-ietf-netmod-yang-instance-file-format-10

Kent // shepherd

On Mar 23, 2021, at 6:01 PM, Balázs Lengyel <[email-masked] <mailto:[email-masked]> > wrote:

Hello Kent,

Regards Balazs

From: Kent Watsen <[email-masked] <mailto:[email-masked]> > 
Sent: 2020. április 9., csütörtök 22:11
To: Balázs Lengyel <[email-masked] <mailto:[email-masked]> >
Cc: [email-masked] <mailto:[email-masked]> 
Subject: Re: [netmod] Shepherd review on draft-ietf-netmod-yang-instance-file-format-10

Hi Balazs,

K.

BALAZS4:  Yes, correct. 

Semver will take time and may undergo changes, so it would be too early to adapt this draft/RFC to it. However, two items already deal with Semver:

1.	The instance data files themselves will not have a Semver revision as the concept of compatibility is not defined for instance data.
2.	The YANG module ietf-yang-instance-data already includes:

                                                     “... If other methods

                    (e.g., revision-label) are defined to identify

                    individual module revisions those MAY be used

                    instead of using a revision date.”

---

From: Kent Watsen <[email-masked]>
Date: Thu, 15 Apr 2021 00:51:40 +0000

Hi Balazs,

Please find my comments below.

K.

> On Apr 7, 2021, at 9:56 AM, Balázs Lengyel <[email-masked]> wrote:
> 
> Hello Kent,

Join the club ;)

> See answer below as BALAZS4. Removed already agreed parts.

Time for me to attempt the shepherd writeup again, and then publish it for IESG review.

K.

> On Mar 23, 2021, at 6:01 PM, Balázs Lengyel <[email-masked] <mailto:[email-masked]>> wrote:
>  
> Hello Kent,

Ack.

> The instance data files themselves will not have a Semver revision as the concept of compatibility is not defined for instance data.
> The YANG module ietf-yang-instance-data already includes:
>                                                      “... If other methods