[yang-doctors] Yangdoctors early review of draft-ietf-l2sm-l2vpn-service-model-08
Ladislav Lhotka <lhotka@nic.cz> Mon, 26 February 2018 12:35 UTC
Return-Path: <lhotka@nic.cz>
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 456CE126DD9; Mon, 26 Feb 2018 04:35:11 -0800 (PST)
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
From: Ladislav Lhotka <lhotka@nic.cz>
To: yang-doctors@ietf.org
Cc: l2sm@ietf.org, ietf@ietf.org, draft-ietf-l2sm-l2vpn-service-model.all@ietf.org
X-Test-IDTracker: no
X-IETF-IDTracker: 6.72.4
Auto-Submitted: auto-generated
Precedence: bulk
Message-ID: <151964851123.31353.1471509914124380207@ietfa.amsl.com>
Date: Mon, 26 Feb 2018 04:35:11 -0800
Archived-At: <https://mailarchive.ietf.org/arch/msg/yang-doctors/SkbrM_o6nSpCWmxxWvLQ9xcKN84>
Subject: [yang-doctors] Yangdoctors early review of draft-ietf-l2sm-l2vpn-service-model-08
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: Mon, 26 Feb 2018 12:35:11 -0000
Reviewer: Ladislav Lhotka Review result: Ready with Issues **** General comments - The 'ietf-l2vpn-svc' module contained in this document is rather large: it defines 386 schema nodes, 35 features and 175 identities. It is therefore natural to ask whether the authors considered splitting the definitions into multiple modules. This would make the data model more modular and probably also make some of the features unnecessary. See also draft-ietf-netmod-rfc6087bis-18, sec. 4.17. - The module defines most containers, lists and even individual leaves in a grouping that is then used only once. This approach has its pros nad cons but it is not the common practice in YANG modelling - groupings are mostly defined only if they are used (or expected to be used) repeatedly and/or in different modules. - Some of the groupings (e.g. 'site-device') reference nodes outside the grouping, which is discouraged, see draft-ietf-netmod-rfc6087bis-18, sec. 4.13. - Names are sometimes overloaded in that the same name is used for different purposes. For example, 'cloud-access' is used as the name of both a list and a feature. In one case ('bum-frame-delivery'), the same name is used for a list, the container that encloses it, and the grouping in which the container is defined. This should be avoided. **** Specific comments - Most when expressions correctly use the XPath function 'derived-from-or-self()'. In two cases ('vpn-id' and 'cos-id' leaves) identities are compared in XPath expressions using plain string equality. These XPath expressions need to be changed to use 'derived-from-or-self()' as well. - The when expression for container 'devices' user a disjunction of two 'derived-from-or-self()' checks. This usually indicates that a new identity can be defined from which both or-ed identities are derived. **** Instance examples The example instance documents contained in the document require to be carefully checked because they are often invalid or even not well-formed. I would also suggest to use examples in JSON encoding that is easier to read: The problems that I discovered include: - extra space in opening and closing XML tags < network-access-id> and </ network-access-id> - p. 28: wrong namespace URI "urn:ietf:params:xml:ns:yang:ietf-l3vpn-svc" - p. 28: in the first 'vpn-service' entry (key 'VPNB'), two mandatory leaves are missing: 'ce-vlan-preservation' and 'ce-vlan-cos-perservation'. **** Nits - The name of 'ce-vlan-cos-perservation' should most likely be 'ce-vlan-cos-preservation' - Inconsistent indentation: features 'cfm' and 'y-1731' have less spaces of indentation than other features - When concatenating literal strings, the widely accepted convention is to put the '+' sign ot the second line, and align the string components. That is, instead of path "/l2vpn-svc/vpn-profiles/valid-provider-identifiers"+ "/cloud-identifier/id"; use path "/l2vpn-svc/vpn-profiles/valid-provider-identifiers" + "/cloud-identifier/id";
- [yang-doctors] Yangdoctors early review of draft-… Ladislav Lhotka
- [yang-doctors] R: Yangdoctors early review of dra… Fioccola Giuseppe
- Re: [yang-doctors] R: Yangdoctors early review of… Ladislav Lhotka
- [yang-doctors] R: R: Yangdoctors early review of … Fioccola Giuseppe