Re: [yang-doctors] Yangdoctors early review of draft-ietf-i2rs-yang-dc-fabric-network-topology-02

["Zhuangyan (Yan)" <[e-mail]>]

Hi Reshad,

Thank you very much for the comments. We've updated them into new version -03.
Please see more responses inline.

Thank you.

Yan

-----邮件原件-----
发件人: Reshad Rahman [mailto:[e-mail]] 
发送时间: 2017年12月13日 6:38
收件人: [e-mail]
抄送: [e-mail]; [e-mail]
主题: Yangdoctors early review of draft-ietf-i2rs-yang-dc-fabric-network-topology-02

Reviewer: Reshad Rahman
Review result: Ready with Issues

YANG Doctor review of draft-ietf-i2rs-yang-dc-fabric-network-topology-02 (by Reshad Rahman)

3 modules defined in this draft:
- [e-mail]
- [e-mail]
- [e-mail]

No YANG validation errors or warnings (from yang and tangling).

0 examples are provided in this draft (section 3.12 of
draft-ietf-netmod-rfc6087bis-14)

Module [e-mail]:
- “import ietf-inet-types” should have a reference to RFC6991 (see section 4.7 of rfc6087bis-14) 
[Y1222] added reference for import.

- “import ietf-network-topology”, prefix should match new prefix name (if it changes) for this module as per YD review comments of draft-ietf-i2rs-yang-network-topo 
[Y1222] yes, we use the same prefix as netork-topo.

- Remove WG Chairs from contact information as per Appendix C of rfc6087bis-14
[Y1222] done.

- Description mentions draft-zhuang-i2rs-…, should say draft-ietf-i2rs-…. Also in description add “Note to RFC Editor” with text saying something along the lines of “Please replace reference to draft-ietf-i2rs-… with RFC Number when published”. - In description: Copyright s/2016/2017/
[Y1222] Done. Thank you so much…

- Is the bandwidth identity really needed? Why not a uint64 with a unit of kbps as in draft-ietf-netmod-intf-ext-yang? 
[Y1222] it is a suggested identity by Alex and we think it is good for extension. It is the BW for port, currently we have 1M,100M,GE,10GE…it is not a random figure but reflecting capacity of physical port.

- Get rid of revision history, i.e. keep only 1 revision (latest) 
[Y1222] done.

- What is the difference between identity fabric-type (VXLAN, VLAN) and enum underlay-network-type (VXLAN, TRILL, VLAM). Are both needed or do they refer to the same thing? Should be identity and not enum to support new types in the future? - service-capabilities. Should this be an identity for future extensibility? Need more in the description, add reference to other documents where appropriate. 
[Y1222] yes, they refer to the same thing… we’ve changed the module to use identity which is good for extension.

- Groupings route-group, port-functions, acl-list not used, still needed or can these be removed? - If you do keep route-group, it is IPv4 specific right now so it needs to be modified. - If you do keep acl-group, please take a look at draft-ietf-netmod-acl-model.
[Y1222] thanks, they were used in previous versions…all removed now.

- device-role and fabric-port-role. Why not use identity to allow for new roles to be added in the future. - Is it possible (now or in near-future) for multiple roles to be assumed?  If yes, how will this be handled?
[Y1222] they are all modified to use identity instead of enumeration.

- In grouping fabric-port, tunnel-option is there unconditionally. Should it be there only when type is layer2Tunnel or layer3Tunnel? If so add a when statement. Similarly should are only be allowed for layer3Tunnel? - In grouping fabric-port, looks like only L3 tunnel (GRE) is supported. What L2 Tunnels will be supported?
[Y1222] yes, both L2 and L3 are allowed. We only gave GRE as an example. For better extension, we removed the GRE case but left the choice for developers.

Module [e-mail]:
- Remove WG Chairs from contact information as per Appendix C of rfc6087bis-14
- Indentation issue on P17
[Y1222] Corrected. 
- fport-attributes is “config false”, how is a GRE tunnel configured?
[Y1222] The fabric port is mapped to the physical port which is read from the underlay network. Hence, it is not for configuration…

- Description mentions draft-zhuang-i2rs-…, should say draft-ietf-i2rs-…. Also in description add “Note to RFC Editor” with text saying something along the lines of “Please replace reference to draft-ietf-i2rs-… with RFC Number when published”. - In description: Copyright s/2016/2017/ - gateway-mode, need more text in the description and/or references to other documents.
[Y1222] Corrected and added more description in gateway-mode.

[e-mail]:
- In description make it clear that this module is not needed when NMDA is supported. - Revision history is incorrect since it has “NMDA”. - Same comments as for ietf-fabric-topology
[Y1222] Added. 

General comments on draft:
- Since the document is for DC Fabrics, should the YANG modules be renamed from ietf-fabric-xxx.yang to ietf-dc-fabric-xxx.yang? - The descriptions in all YANG Modules are very short/terse. - No IANA Considerations, please see section 3.8 of 6087bis-14. - Security Considerations. Follow template @ https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines as per 6087bis-14.
[Y1222] Renamed the module name to ietf-dc-fabric-xxx.yang. Security consideration is updated and related references added. IANA section is added.

Looks like the first part of the template is missing. - Appendix A has no text, just the YANG module. There should be some text explaining why the -state module exists. Take a look at the text in Appendix B of
draft-ietf-i2rs-yang-network-topo-14 as an example.
[Y1222] Explanations for non-NMDA module has been added.

Nits:
- 3.2.1 s/snatch/snip/?
- 3.2.3 s/terminiation/termination/
- 3.2.3 s/etc al/etc/?
- Most descriptions in the YANG Modules start with lower-case, should be upper-case. - s/Security Consideration/Security Considerations/ - s/fabric Topology/fabric topology/? Any it should either be both lower-case or both upper-case.
[Y1222] Corrected…