[CCAMP] Yangdoctors early review of draft-ietf-ccamp-l1csm-yang-07

Robert Wilton <rwilton@cisco.com> Thu, 06 September 2018 16:09 UTC

Return-Path: <rwilton@cisco.com>
X-Original-To: ccamp@ietf.org
Delivered-To: ccamp@ietfa.amsl.com
Received: from ietfa.amsl.com (localhost [IPv6:::1]) by ietfa.amsl.com (Postfix) with ESMTP id 0C69C130EB8; Thu, 6 Sep 2018 09:09:27 -0700 (PDT)
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
From: Robert Wilton <rwilton@cisco.com>
To: <yang-doctors@ietf.org>
Cc: draft-ietf-ccamp-l1csm-yang.all@ietf.org, ccamp@ietf.org
X-Test-IDTracker: no
X-IETF-IDTracker: 6.83.1
Auto-Submitted: auto-generated
Precedence: bulk
Message-ID: <153625016699.11729.8683486630406068554@ietfa.amsl.com>
Date: Thu, 06 Sep 2018 09:09:27 -0700
Archived-At: <https://mailarchive.ietf.org/arch/msg/ccamp/SP998H68Mvn14eKnz9J-JzGSUKI>
Subject: [CCAMP] Yangdoctors early review of draft-ietf-ccamp-l1csm-yang-07
X-BeenThere: ccamp@ietf.org
X-Mailman-Version: 2.1.29
List-Id: Discussion list for the CCAMP working group <ccamp.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/ccamp>, <mailto:ccamp-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/ccamp/>
List-Post: <mailto:ccamp@ietf.org>
List-Help: <mailto:ccamp-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/ccamp>, <mailto:ccamp-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 06 Sep 2018 16:09:27 -0000

Reviewer: Robert Wilton
Review result: On the Right Track

This is an "early review" rather than a WG LC review.

The YANG module is a relatively small and simple model to configure and
identify UNIs and L1 services.  The draft and model are fairly easy to follow
and both are in reasonable shape.  The YANG modules may benefit from some
fairly cosmetic cleanup, and minor restructuring.

Minor comments on the document:

1) In the abstract, and early part of the introduction, it wasn't particularly
clear whether this YANG model is intended is a "service" level YANG model, e.g.
to run on a controller, or a "device" level YANG model, intended to instantiate
YANG models on network devices.  So, I would suggest making this really clear
in the abstract, and early introduction, and perhaps reference RFC 8199

2) The diagram in 1.1 is slightly mis-aligned.

Comments on the YANG model:

1) Please check alignment, indentation, perhaps run the module through "pyang
-f yang" to help ensure alignment is OK.

2) I Suggest leaving a blank line between leaf statements (e.g. for UNI access
attributes grouping).

3) As a person preference, I try and minimize groupings because I think that
they obfuscate reading the source YANG model somewhat.  To that end, I would
suggest removing some of the groupings, and only keep the ones that are really
useful.  Groupings can always be introduced in future as a fully backwards
compatible change, so there isn't really any benefit in adding two much
structure now.

4) Please add reference statements where possible, e.g. for protocol, coding,
optical_interface

5) I suggest changing optical_interface => optical-interface

6) protocol-coding-optical_interface grouping: I Suggest hypenating the entire
name to "protocol-coding-optical_interface"

7) Should any of the UNI parameters be marked as mandatory?

8) uni-attributes grouping:
I recommend removing this grouping and just putting the single leaf (which is a
key) and "uses" statement inline in "uni-list".

9) "UNI-ID":
I would recommend renaming this key to "uni-id", but UNI should be used instead
of uni in the description.

10) I suggest changing the description for "access" to indicate that it is for
access related content.

11) I suggest renaming "uni-list" to "uni" and to put the list into a "unis"
container (so the path to the list entry becomes access/unis/uni), or another
option would be to rename "access" to unis.

12) l1cs container.
Is "l1cs" the most meaningful top level name?  Perhaps using the long form
"l1-connectivity-services" or maybe just "l1-connectivity" might be better for
someone who is not so familiar with the acronym.

13) service container
 - Suggest renaming this container to "services".
 - Suggest renaming "service-list" to "service"
 - Suggest possibly renaming "subscriber-l1vc-id" to just "service-id".
 - Suggest removing the "service-config" container and having the contents
 directly under the list entry. - Suggest adding two containers "endpoint-1"
 and "endpoint-2" each of which contain "id" and "uni".  Another choice would
 be to have a list of endpoints, requiring unique ids (which you could restrict
 to a maximum of two entries today, which would allow it to be increased in
 future if requried).  A list might be more useful if you can ever have more
 have 2 endpoints.  I don't know if that is ever possible with EPON or FlexE? -
 Should the service id, endpoint ids or endpoint uni references be mandatory at
 all?

 14) Should any of the id's have a maximum length defined, probably sticking
 some reasonable bound on them would be sensible.

 15) The description for the subscriber-l1vc-sls-service-attribute looks
 potentially wrong, or otherwise misleading. - perhaps rename "time-start" to
 "start-time" - should performance-metric be a leaf-list?  E.g. is more than
 one performance-metric allowed?  If only one then the comment needs to be
 updated.

 If you applied all of my comments above, I think that tree structure of your
 module would look something like this:

 module: ietf-l1csm
    +--rw l1-connectivity
       +--rw access
       |  +--rw unis
       |     +--rw uni* [id]
       |        +--rw id                   string
       |        +--rw protocol?            identityref
       |        +--rw coding?              identityref
       |        +--rw optical_interface?   identityref
       +--rw services
          +--rw service* [service-id]
             +--rw id                      string
             +--rw endpoint-1
             |  +--rw id                   string
             |  +--rw uni    -> /l1-connectivity/access/unis/uni/id
             +--rw endpoint-2
             |  +--rw id                   string
             |  +--rw uni    -> /l1-connectivity/access/unis/uni/id
             +--rw start-time?                 yang:date-and-time
             +--rw time-interval?              Int32
             +--rw performance-metric?         Identityref

For the types module:
17) I would suggest expanding "GigE" to "Gigabit Ethernet" in the protocol type
description, similarly for the other types.  Perhaps be more descriptive for
the difference between 10GE WAN vs LAN. 18) For fiber channel, is the
100/200/400 very meaningful, or should there be units here. 19) For the
coding-func, are the clause references to IEEE 802.3?  If so, that may be
useful to include in the reference and descriptions.  Also, do you want the
clause numbers in the names, it seems like this is something that could
potentially change over time and become less meaningful. 20) A similar comment
re clauses also applies for the optical-interface identities. 21) Identity
performance-metriclist should probably just be performance-metric.

Other doc comments:

22) The JSON example is good, but it was also be useful to have a short
paragraph describing in English what the example represents ... although I
appreciate that it is pretty trivial.

23) The security section mentions that it will indicate
sensitivity/vulnerability but then doesn't do this.