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

[Kent Watsen <[e-mail]>]

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)
- “import ietf-inet-types” should have a ‘reference’ to RFC 6991
- remove “WG Chairs”, per the template in rfc6087bis-13 Appendix C.
- s/Editor/Author/? (this is a preference choice)
- defines ‘network-ref’ that is unused within module (it’s used by
  ietf-network-topology)
- leafref paths don’t include prefix (rfc6087bis S4.2)

Regarding [e-mail]:
- prefix “lnk” should be “nt” or maybe “nwtp”? (in IANA Considerations also)
- “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.
- s/Editor/Author/? (this is a preference choice)
- defines grouping link-ref and tp-ref that are unused within module
- mandatory true for source/dest-node/tp?
- replace “tp” with “term-pt”?  (both in “-tp” and “tp-“ uses)

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

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


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.

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.

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

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.

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

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

7) Section 4.4.8., why not use identities?

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

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?

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.

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

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

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


Nits:

- why reference RFC6991 in the Introduction?
- replace “allows to define” with “enables the definition of”
- in a few places, you refer to “network.yang”, but the 
  file is called “ietf-network.yang”.
- in a few places, you refer to “network-topology.yang”, 
  but the file is called “ietf-network-topology.yang”.
- replace “- X1 and X2 - mapping onto a single L3 network element”
  with “(X1 and X3) mapping onto a single L3 network element (Y2)”
- replace “data-store” with “datastore”
- replace “model” with “data model” where it’s not already.
- replace “the <intended> datastore” with just “<intended>”
- replace “the intended datastore” with just “<intended>”
- update Section 2 to also include a reference to RFC 8174 (see RFC 8174)
- pull the “datastore” term from the revised-datastores draft?
- NETCONF and YANG don’t need to be terms/defined, since references
  to their RFCs are provided when they’re used.
- s/the network.yang module/the “ietf-network” YANG module/
- 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?
- 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/
- s/configuration and intended datastores/conventional datastores/
- 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
- /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”
- 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/
- s/in the following two companion modules/the following two companion modules/
- s/that represent a state model/to represent the operational state/

Thanks,
Kent