Subject: [L2sm] Yangdoctors early review of draft-ietf-l2sm-l2vpn-service-model-08

From: Ladislav Lhotka <[email-masked]>
Date: Mon, 26 Feb 2018 04:35:11 -0800

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

---

From: Fioccola Giuseppe <[email-masked]>
Date: Wed, 28 Feb 2018 13:15:39 +0000

Hi Ladislav,
Thank you! We are working on a new revision that incorporates your comments.
My answers inline tagged as [GF].

Best Regards,

Giuseppe

-----Messaggio originale-----
Da: Ladislav Lhotka [mailto:[email-masked]] 
Inviato: lunedì 26 febbraio 2018 13:35
A: [email-masked]
Cc: [email-masked]; [email-masked]; [email-masked]
Oggetto: Yangdoctors early review of draft-ietf-l2sm-l2vpn-service-model-08

**** General comments

[GF]: Ok, Thanks for the suggestion. We will remove unnecessary identities, features. Note that unlike network element model, service model will describe various aspects of network infrastructure, including devices and their subsystems, and relevant protocols operating at the link and network layers across multiple device. So it is intentional to have such model design.

[GF]: It is expected some of these groupings can be reused in some other external modules, or model extension.

[GF]: Will fix it. Thanks.

[GF]: Ok

**** Specific comments

 [GF]: Ok

 [GF]: Ok

**** Instance examples

 [GF]: Will fix it.

[GF]: Will fix it.

[GF]: Will fix it.

**** Nits

[GF]: Will fix it.

[GF]: Will fix it.

       use

[GF]: Will fix it.

Questo messaggio e i suoi allegati sono indirizzati esclusivamente alle persone indicate. La diffusione, copia o qualsiasi altra azione derivante dalla conoscenza di queste informazioni sono rigorosamente vietate. Qualora abbiate ricevuto questo documento per errore siete cortesemente pregati di darne immediata comunicazione al mittente e di provvedere alla sua distruzione, Grazie. 

This e-mail and any attachments is confidential and may contain privileged information intended for the addressee(s) only. Dissemination, copying, printing or use by anybody else is unauthorised. If you are not the intended recipient, please delete this message and any attachments and advise the sender by return e-mail, Thanks. 

Rispetta l'ambiente. Non stampare questa mail se non è necessario.

---

From: Ladislav Lhotka <[email-masked]>
Date: Wed, 28 Feb 2018 15:58:03 +0100

Hi Giuseppe,

please see inline.

On Wed, 2018-02-28 at 13:15 +0000, Fioccola Giuseppe wrote:
> [GF]: Ok, Thanks for the suggestion. We will remove unnecessary identities,
> features. Note that unlike network element model, service model will describe
> various aspects of network infrastructure, including devices and their
> subsystems, and relevant protocols operating at the link and network layers
> across multiple device. So it is intentional to have such model design.

Yes, but RFC 8199 also talks about monolithic versus component-based approaches
in relation to network service models. I am not saying that your design is
wrong, just suggesting to consider the alternatives that could possibly make
this relatively big data model easier to comprehend.

> external modules, or model extension.

If only some of them are intended to be reused, then I would suggest to remove
those that aren't. Model extensions are usually implemented with an augment, and
groupings don't help in this case.

Thanks, Lada

-- 
Ladislav Lhotka
Head, CZ.NIC Labs
PGP Key ID: 0xB8F92B08A9F76C67