YANG Doctor review of draft-ietf-i2rs-yang-network-topo-14 (by Kent Watsen) 4 modules are defined in the draft: - ietf-network@2017-06-30.yang - ietf-network-topology@2017-06-30.yang - ietf-network-state@2017-06-30.yang - ietf-network-topology-state@2017-06-30.yang 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 ietf-network@2017-06-30.yang: - 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 ietf-network-topology@2017-06-30.yang: - 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 ietf-networ-statek@2017-06-30.yang: - similar comments to ietf-network@2017-06-30.yang: Regarding ietf-network-topology-state@2017-06-30.yang: - similar comments to ietf-network-topology@2017-06-30.yang. 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 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 , 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 datastore” with just “” - replace “the intended datastore” with just “” - 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// - s/into the data model without relying/into 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