Early Review of draft-ietf-ccamp-l1csm-yang-07
review-ietf-ccamp-l1csm-yang-07-yangdoctors-early-wilton-2018-09-06-00

Request Review of draft-ietf-ccamp-l1csm-yang-07
Requested rev. 07 (document currently at 09)
Type Early Review
Team YANG Doctors (yangdoctors)
Deadline 2018-09-28
Requested 2018-09-05
Requested by Daniele Ceccarelli
Draft last updated 2018-09-06
Completed reviews Yangdoctors Early review of -07 by Robert Wilton (diff)
Assignment Reviewer Robert Wilton
State Completed
Review review-ietf-ccamp-l1csm-yang-07-yangdoctors-early-wilton-2018-09-06
Reviewed rev. 07 (document currently at 09)
Review result On the Right Track
Review completed: 2018-09-06

Review
review-ietf-ccamp-l1csm-yang-07-yangdoctors-early-wilton-2018-09-06

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.