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";
