[yang-doctors] Yangdoctors early review of draft-ietf-i2rs-yang-dc-fabric-network-topology-02
Reshad Rahman <rrahman@cisco.com> Tue, 12 December 2017 22:37 UTC
Return-Path: <rrahman@cisco.com>
X-Original-To: yang-doctors@ietf.org
Delivered-To: yang-doctors@ietfa.amsl.com
Received: from ietfa.amsl.com (localhost [IPv6:::1]) by ietfa.amsl.com (Postfix) with ESMTP id 4734512957C; Tue, 12 Dec 2017 14:37:55 -0800 (PST)
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
From: Reshad Rahman <rrahman@cisco.com>
To: yang-doctors@ietf.org
Cc: i2rs@ietf.org, draft-ietf-i2rs-yang-dc-fabric-network-topology.all@ietf.org
X-Test-IDTracker: no
X-IETF-IDTracker: 6.67.1
Auto-Submitted: auto-generated
Precedence: bulk
Message-ID: <151311827525.30097.15477684350738516157@ietfa.amsl.com>
Date: Tue, 12 Dec 2017 14:37:55 -0800
Archived-At: <https://mailarchive.ietf.org/arch/msg/yang-doctors/OUJYH4Mt8eGR9mQz_QWsQR_AuhI>
Subject: [yang-doctors] Yangdoctors early review of draft-ietf-i2rs-yang-dc-fabric-network-topology-02
X-BeenThere: yang-doctors@ietf.org
X-Mailman-Version: 2.1.22
List-Id: Email list of the yang-doctors directorate <yang-doctors.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/yang-doctors/>
List-Post: <mailto:yang-doctors@ietf.org>
List-Help: <mailto:yang-doctors-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 12 Dec 2017 22:37:55 -0000
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: - ietf-fabric-topology-state@2017-11-29.yang - ietf-fabric-topology@2017-11-29.yang - ietf-fabric-types@2017-11-29.yang 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 ietf-fabric-types@2017-11-29.yang: - “import ietf-inet-types” should have a reference to RFC6991 (see section 4.7 of rfc6087bis-14) - “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 - Remove WG Chairs from contact information as per Appendix C of rfc6087bis-14 - 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/ - Is the bandwidth identity really needed? Why not a uint64 with a unit of kbps as in draft-ietf-netmod-intf-ext-yang? - Get rid of revision history, i.e. keep only 1 revision (latest) - 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. - 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 - 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? - 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? Module ietf-fabric-topology@2017-11-29.yang: - Remove WG Chairs from contact information as per Appendix C of rfc6087bis-14 - Indentation issue on P17 - fport-attributes is “config false”, how is a GRE tunnel configured? - 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. ietf-fabric-topology-state@2017-11-29.yang: - 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 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. 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. 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.
- [yang-doctors] Yangdoctors early review of draft-… Reshad Rahman
- Re: [yang-doctors] Yangdoctors early review of dr… Zhuangyan (Yan)