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

[Leeyoung <[e-mail]>]

Hi Robert,

Thank you for your thorough review and many good suggestions. I believe all your comments have been incorporated in the revision that was just published.  Please check the following pointer that compares versions 07 and 08. 

https://www.ietf.org/rfcdiff?url2=draft-ietf-ccamp-l1csm-yang-08

In case I missed any of your comments, please let me know. 

Best regards,
Young (on behalf of co-authors)

-----Original Message-----
From: Robert Wilton [mailto:[e-mail]] 
Sent: Thursday, September 6, 2018 11:09 AM
To: [e-mail]
Cc: [e-mail]; [e-mail]
Subject: Yangdoctors early review of draft-ietf-ccamp-l1csm-yang-07

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.