Re: [yang-doctors] [i2rs] Yangdoctors last call review of draft-ietf-i2rs-yang-network-topo-14

["Alexander Clemm" <[e-mail]>]

Hi Kent,

Thank you for your thorough review.  

I am posting a rev 15 with the comments addressed, per responses inline, <ALEX>

--- Alex

-----Original Message-----
From: i2rs [mailto:[e-mail]] On Behalf Of Kent Watsen
Sent: Tuesday, August 8, 2017 4:57 PM
To: [e-mail]
Cc: [e-mail]; [e-mail]
Subject: [i2rs] Yangdoctors last call review of draft-ietf-i2rs-yang-network-topo-14

Reviewer: Kent Watsen
Review result: Almost Ready


YANG Doctor review of draft-ietf-i2rs-yang-network-topo-14 (by Kent Watsen)


4 modules are defined in the draft:
- [e-mail]
- [e-mail]
- [e-mail]
- [e-mail]

No validation errors from either `pyang` or `yanglint`.

0 examples are defined in the draft.

The -state modules appear to have all the correct changes from their base modules.

Regarding [e-mail]:
- prefix “nd”, should be “nw”?  (in IANA Considerations also)

<ALEX> The prefixes are "historic".  We  could change them, but this will have ripple effects on derived models. 
</ALEX>

- “import ietf-inet-types” should have a ‘reference’ to RFC 6991

<ALEX> done </ALEX>

- remove “WG Chairs”, per the template in rfc6087bis-13 Appendix C.

<ALEX> done </ALEX>

- s/Editor/Author/? (this is a preference choice)

<ALEX> left as is - I think "Editor" is what is commonly used.  If you feel strongly about this, please let us know.  </ALEX>

- defines ‘network-ref’ that is unused within module (it’s used by
  ietf-network-topology)

<ALEX> left as is.  This might be useful also for other modules.  Rather than have it be redundantly defined elsewhere, IMHO this is the best place to keep it. </ALEX>

- leafref paths don’t include prefix (rfc6087bis S4.2)

<ALEX> Prefixes added </ALEX>

Regarding [e-mail]:
- prefix “lnk” should be “nt” or maybe “nwtp”? (in IANA Considerations also)

<ALEX> The prefixes are "historic".  We  could change them, but this will have ripple effects on derived models. 
</ALEX>

- “import ietf-inet-types” should have a ‘reference’ to RFC 6991
- “import ietf-network” should have a ‘reference’ to RFC XXXX
- remove “WG Chairs”, per the template in rfc6087bis-13 Appendix C.


<ALEX> done </ALEX>

- s/Editor/Author/? (this is a preference choice)

<ALEX> left as is, consistent with above </ALEX>

- defines grouping link-ref and tp-ref that are unused within module

<ALEX> left as is.  While it would be possible to strike these, we feel they might eventually be useful for other modules that want to reuse what arguably constitutes a "complex data type".  Rather than have it be redundantly defined elsewhere, IMHO this is the best place to keep it. </ALEX>

- mandatory true for source/dest-node/tp?

<ALEX>  It is true that in general this will be included, but there may be instances when the fact that there is a link will need to be represented, even if it is not properly connected yet.  Therefore, we would not make it mandatory.   
</ALEX>  

- replace “tp” with “term-pt”?  (both in “-tp” and “tp-“ uses)

<ALEX> prefer to keep.  </ALEX>

Regarding [e-mail]:
- similar comments to [e-mail]:

Regarding [e-mail]:
- similar comments to [e-mail].

<ALEX> Analogous comments apply </ALEX>


Comments on draft:

1) The document still has references to “server-provided”.  Not the YANG leaf of course, but in general text.  For instance “The model does allow to layer a network that is configured on top of one that is server-provided.”
I think that the statement is more about values being in <operational> than how it was learned.  All uses of “server-provided” should be examined for correctness.

<ALEX> Corresponding textual updates throughout </ALEX>

2) This sentence doesn’t make sense to me, how can a server-provided model (you mean data?) access any information in conventional datastores?:
   “An implementation's security policy MAY further restrict what
   information the server-provided model is allowed to access in
   standard configuration data-stores,”
Either way, this text likely should be moved to the Security Considerations section.

<ALEX> I removed this sentence altogether.  What we had originally in mind was to refer to the fact that you could have a leafref point to an object that has a different access authorization.  This is nothing specific to the model, but a corner case of NACM.   I think it is safe to remove this. 
</ALEX>

3) Should the last paragraph in Section 1 be removed now?  Is the module expected to continue to update?

<ALEX> Yes, agree - removed.  This was historic; we simply hadn't looked at this section in the draft for a while:-)
</ALEX>

4) S4.1, P3: the text here says new data nodes are augmented in, but the YANG module itself says that only presence containers are allowed.

<ALEX> I am not sure I see an issue here?  The "network-types" node is a container (not a presence container), which will serve as a target for augmentation.  The anticipated pattern is that nodes that are going to be augmented in will be presence containers that indicate that a network/topology of a certain type is present.   
</ALEX>

5) are the mandatory statement values set appropriately everywhere?
(e.g., source-node?, source-tp?)

<ALEX> Yes, we had a bunch of discussions on this in the past; mandatory is not needed.  </ALEX>

6) network-type is a presence container, not an identity? No where in the draft is there an example showing it being used.

<ALEX> The examples are in the L3 topology draft.  Let me add a text snippet referring to that </ALEX>

7) Section 4.4.8., why not use identities?

<ALEX> This was our design choice.  We did want to be able to represent "type hierarchies", this we could do with containers (that can become targets for augmentation).   

8) S4.4.9 is a duplicate of some text in S4.1

<ALEX> Looking at the text, some things described in 4.4.9 are alluded to in 4.1 (the general overview), but not in a way that I would consider "overspecified" or redundant - would prefer to keep as is - the way it is it flows well IMHO.  
</ALEX>

9) This statement is true, but I think misleading in context. “YANG requires data nodes to be designated as either configuration or operational data, but not both”. It seems that the solution depends entirely on config true nodes, and NMDA to present the operational value of those config true nodes.  Right?

<ALEX> This text of course predates the relatively recent NMDA stuff, but NMDA does not make it false.  I don't find the text misleading, but I did make some edits to refer specifically to NMDA, to clarify that the nodes are config true, and that NMDA is used for the distinction.  
</ALEX>

10) When using <CODE BEGINS>, you should have a note to the RFC Editor to change the date to the date of publication, both in the filename as well as in the module’s ’revision’ statement.
<ALEX> Thank you, done </ALEX>

11) IANA Considerations has comments “(RFC form)” - are these supposed to be notes to RFC Editor to replace with final RFC designation?

<ALEX> Yes, this is what is meant here.  Replaced this with a more explicit statement "NOTE TO RFC EDITOR:"  </ALEX>

12) Your Security Considerations section should follow the template here:
https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines

<ALEX> I made updates, but do not list every single object with its security implications  which I find excessive.  
</ALEX>

13) create examples for the use-cases in Appendix A?  Note, every draft should have examples of its YANG modules...

<ALEX> Really prefer not to unless we have to.  We want to be done with this.  The model has been successfully used and extended; IMHO an example is not needed to add for clarification.  <ALEX>

Nits:

- why reference RFC6991 in the Introduction?
<ALEX> removed </ALEX>
- replace “allows to define” with “enables the definition of”
<ALEX> ok </ALEX>
- in a few places, you refer to “network.yang”, but the
  file is called “ietf-network.yang”.
<ALEX> fixed </ALEX>
- in a few places, you refer to “network-topology.yang”,
  but the file is called “ietf-network-topology.yang”.
<ALEX> fixed </ALEX>
- replace “- X1 and X2 - mapping onto a single L3 network element”
  with “(X1 and X3) mapping onto a single L3 network element (Y2)”
<ALEX> ok </ALEX>
- replace “data-store” with “datastore”
<ALEX> ok </ALEX>
- replace “model” with “data model” where it’s not already.
<ALEX> not sure there were any ambiguities and it's been done in other cases, but ok </ALEX>
- replace “the <intended> datastore” with just “<intended>”
- replace “the intended datastore” with just “<intended>”
<ALEX> Updated most instances, i.e. those where it made sense, but left some in place as I believe it flows better.  </ALEX>
- update Section 2 to also include a reference to RFC 8174 (see RFC 8174)
<ALEX> ok </ALEX>
- pull the “datastore” term from the revised-datastores draft?
<ALEX> I think the definition here is good </ALEX>
- NETCONF and YANG don’t need to be terms/defined, since references
  to their RFCs are provided when they’re used.
<ALEX> ok, removed </ALEX>
- s/the network.yang module/the “ietf-network” YANG module/
<ALEX> ok </ALEX>
- your tree-diagram notation in S4.1 isn’t complete.  And you duplicate
  it in S4.2.  Create a top-level section called “tree diagram notation”
  that both sections reference?
<ALEX> For the notation description, I incorporated a reference to the YANG tree diagrams draft </ALEX>
- s/allows to represent/allows representation of/
- s/another container/a presence container called/
- s/allows to define more/allows definition of more/
- s/allows also to represent/allows representation of/
<ALEX> updated </ALEX>
- s/configuration and intended datastores/conventional datastores/
<ALEX> I think the current reads better, prefer to keep </ALEX>
- s/and show up only/and thus only appear/
- /ietf-network:networks/network/network-id - being the list’s key,
  I was expecting this to the first element defined.
- for the various leafrefs in both models, I see a lot of longish
  relative paths; suggest you change these to absolute paths if possible
<ALEX>Prefer to keep; they work fine</ALEX>
- /nd:networks/nd:network:/link/link-id is the list key but not the
  first leaf listed
- incomplete sentence: “augmentations can in turn against augmenting
  modules”
<ALEX>  fixed </ALEX>
- s/need to specified/need to be specified/
- s/if the link-to-links mapping known/if the link-to-links mapping
  are known/
- s/each link known/each link are known/
- s/the operational datastore/<operational>/
- s/into the data model without relying/into <operational> without relying/
<ALEX> updated </ALEX>
- s/in the following two companion modules/the following two companion modules/
<ALEX> keeping this one, original reads better than the substitution </ALEX>
- s/that represent a state model/to represent the operational state/
<ALEX> ok </ALEX>

Thanks,
Kent



_______________________________________________
i2rs mailing list
[e-mail]
https://www.ietf.org/mailman/listinfo/i2rs