Subject: [yang-doctors] Yangdoctors early review of draft-ietf-ccamp-l1csm-yang-07

From: Robert Wilton <[email-masked]>
Date: Thu, 06 Sep 2018 09:09:27 -0700

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.

---

From: Leeyoung <[email-masked]>
Date: Thu, 13 Sep 2018 16:14:06 +0000

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:[email-masked]] 
Sent: Thursday, September 6, 2018 11:09 AM
To: [email-masked]
Cc: [email-masked]; [email-masked]
Subject: Yangdoctors early review of draft-ietf-ccamp-l1csm-yang-07

Reviewer: Robert Wilton

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.

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

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

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

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

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.