Re: [yang-doctors] Yangdoctors last call review of draft-ietf-ccamp-layer1-types-04

["Rob Wilton (rwilton)" <[e-mail]>]

Hi Haomian,

Please see inline ...

> -----Original Message-----
> From: Zhenghaomian <[e-mail]>
> Sent: 25 December 2019 06:22
> To: Rob Wilton (rwilton) <[e-mail]>; [e-mail]
> Cc: [e-mail]; [e-mail]; draft-ietf-ccamp-layer1-
> [e-mail]
> Subject: 答复: Yangdoctors last call review of draft-ietf-ccamp-layer1-
> types-04
> 
> Hi, Robert,
> 
> Thank you very much for your detailed review and comments. The authors had
> a few iterations of discussions and give the update as follow.
> 
> Model update can be found as the pull request:
> https://github.com/haomianzheng/IETF-ACTN-YANG-Model/pull/52; you (and
> other experts) are welcome to follow up in this thread.
> An updated draft have been locally created, with the diff files attached;
> For detailed comments response/discussion, please see inline with
> '[Authors]';
> 
> Please help review if the comments are addressed, any discussion/calls are
> welcome and can be arranged by chairs, thank you.
> 
> Wish you a Merry Christmas and Happy new year.
> 
> Best wishes,
> Haomian (on behalf of all authors & contributors)
> 
> -----邮件原件-----
> 发件人: Robert Wilton via Datatracker [mailto:[e-mail]]
> 发送时间: 2019年12月14日 0:48
> 收件人: [e-mail]
> 抄送: [e-mail]; [e-mail]; draft-ietf-ccamp-layer1-
> [e-mail]
> 主题: Yangdoctors last call review of draft-ietf-ccamp-layer1-types-04
> 
> Reviewer: Robert Wilton
> Review result: Almost Ready
> 
> Comments on the document:
> 
> 1) Please check the English grammar for the earlier paragraphs.
> [Authors] fixed some editorial problems, and will do again by every
> update;
> 
> 2) If you change the prefix (as in the YANG comments below) then there are
> references in the document that will also need to be updated (e.g. section
> 3, and the IANA section).
> [Authors] done in -05.
> 
> 3) Security section, "onsidered" => "considered"
> [Authors] done in -05.
> 
> Comments on the YANG module:
> 
> 1) I would suggest changing the YANG prefix to "l1-types" to help keep it
> short (particularly for the identities).
> [Authors] done in -05. The only problem is to recognize it correctly as
> 'l1' instead of '11' or 'll', which has been raised before for 'l1csm' in
> ccamp.

[RW]
I think that generally the shorter prefix is better and will make modules that reference the types in this module more readable.


> 
> 2) I would suggest making the module "yang-version 1.1;", because the
> behaviour of YANG 1.1 is better specified.  In fact, I would recommend
> that all IETF YANG modules are defined as YANG 1.1.
> [Authors] done in -05.
> 
> 3) As per my comments in the layer-0-types module, I would suggest using a
> decimal64 for the tributary-slot-granularity, or perhaps even a uint32 if
> it is specified in Mega rather than Giga.  Also, should the units be Ghz?
> [Authors] The unit should be Gbps in layer 1 for rate; in the other
> document (layer0-type) the unit GHz is used for spectrum width.
> [Authors] There are many similar comments (identity->enum) in these
> documents. The authors decided to reject some of the proposals because of
> 1) enumeration is not possible to extend, but there can be new values in
> the future; 2) Specifically for this comment, the configuration of tsg is
> a type rather than a number, so decimal is not a good representation.
[RW] 
Enums are not fixed for all time.  New revisions of a YANG module can add new enum values:

Section 11 from RFC 7950:

   o  An "enumeration" type may have new enums added, provided the old
      enums's values do not change.  Note that inserting a new enum
      before an existing enum or reordering existing enums will result
      in new values for the existing enums, unless they have explicit
      values assigned to them.


In terms of extensibility, identities allow vendors or other parties to define extra values without requiring a new revision of a base model.  However, identities have a slight performance overhead (e.g. section 4.24 of BCP 216). 

If values are tightly tied to standard definitions, then enums are likely to be a better choice, with the expectation that the base model is updated if/when new enum values need to be added.



> 
> 4) For all the ODU-type definitions, I would recommend splitting the
> description from the reference.  I also don't think that the "which is
> categorized as standards track is necessary/useful.  Or if you don't want
> to keep it, then I would retain it only for types that are not standard."
> 
> E.g.
> OLD:
> 
>  identity ODU0 {
>    base odu-type;
>    description
>      "ODU0 protocol (1.24G), RFC7139/ITU-T G.709, which is
>       categorized as standards track .";  }
> 
> NEW:
>  identity ODU0 {
>    base odu-type;
>    description "ODU0 protocol (1.24G)"
>    reference "RFC7139/ITU-T G.709";
>  }
> [Authors] done in -05.
> [Authors] Discussion need: We need to check with YANG doctor, on whether
> make RFC7963/G.sup43 as informational or normative. Prefer to be
> informational, but how should we specify in the model and in the reference
> section?
> 
[RW] 
My hunch is that they should probably be normative.

> 
> 5) Some of the ODU-types don't have references.  I would be good to add
> references to everything that you can (e.g. also the client-signal
> identities) [Authors] done in -05, for ODU-types and client-signal
> identities.
> 
> 6) For the description of ETH-10Gb-WAN, it might be helpful to also
> specify the actual rate (9.95Gb/s) in the description, since it may not be
> obvious.
> [Authors] done for ETH-10Gb-WAN/ETH-10Gb-LAN.
> 
> 7) Looking at how they are used, it might be better for otn-label-range-
> type to be represented as a enum.
> [Authors] We propose not to change, as enum is not good for extension.
[RW]
Do you anticipate that vendors will need to add other otn-label-range-types?


> 
> 8) In grouping otn-label-range-info:
>  - I suggest making range type an enum rather than identity.
> [Authors] We propose not to change, as enum is not good for extension.
>  - Perhaps add a when statement to the "tsg" leaf, so that it can only be
> populated when the enum is a "trib-slot"?  Or otherwise the description is
> unclear. - I wasn't sure how familiar "tsg" is, and perhaps it would be
> better  in its expanded form. - Please add more description to Priority.
> [Authors] Discussion need: tpn-range/ts-range can both be different for
> different TSG values, refer to section 4.3 in this document to see
> detailed usage guidance.
[RW] 
Does it make sense for the user to configure the type to be "label-range-trib-port" and also define a tributary-slot-granularity?


> 
> 9) In grouping otn-label-start-end:
>  - Probably expand the "tpn" and "ts" names?
> [Authors] as they are well-known term in OTN literature. Need to
> understand the principle. As the terms are OTN-specific, it is proposed to
> change to 'otn-tpn' and 'otn-ts' respectively. We also expand in the
> description.
[RW] 
If the terms are expected to be well known to a client using this configuration model then the short acronyms are fine (I'm not that familiar with the details of OTN ...).


>  - Probably define typedefs for "tpn" and "ts" uint16's, since they are
> used in  multiple grouping definitions.
> [Authors] done in -05.
> 
> 10) In grouping otn-label-hop:
>  - Expand "tpn" name and use the typedef (as per previous comment).
>  - Expand "tsg" name
> [Authors] changed accordingly to comments 9.
>  - Expand the ts-list name, and perhaps enhance the description to state
> that
>  values must be in non-overlapping ascending ordering.   Or you could
> borrow
>  some text for section 9.2.4 of RFC 7950.
> [Authors] Following proposal:
> OLD:
>         description
>           "A list of available tributary slots ranging
>            between 1 and 4095.
>            For example 1-20,25,50-1000";
> NEW:
>         description
>           "A list of available tributary slots ranging
>            between 1 and 4095. If multiple values or
>            ranges are given, they all MUST be disjoint
>            and MUST be in ascending order.
>            For example 1-20,25,50-1000";
> 
[RW] 
Looks good.


> 
> 11) In grouping "otn-label-step" the "default" statements don't really
> have any effect.  I think that either you need to add a default statement
> to the choice, or you should delete the default statements under the tpn
> and ts leaves.
> [Authors] Discussion needed: the motivation is to set a default value of
> tpn/ts in each branch to 1 in the otn-label-step grouping, with the choice
> structure. Deleting the default statement would lose our restriction. We
> don't understand the comments on the option 'add a default statement to
> the choice'. Can you give more guidance? Moreover, if we put a default
> value under choice, we are not sure where this default value goes.
[RW] 
A default value is the one used if a client hasn't provided a value.  With a choice statement, only one of the case statements can apply, and the a case statement only takes effect if a value under that case statement has been provided.  Hence, in your current model a client must either explicitly provide a "tributary-port" value or a "tributary-slot" to choose which case statement applies, in which case the default statement has no useful effect because explicit values have to be provided anyway.


> [Authors] Discussion needed: we find the choice on 'tpn' or 'ts' is not
> determined in this grouping, instead it is specified in 'otn-label-range-
> info'. Help requested for modeling this representation.
[RW] 
I need to understand more about how these groupings are used.

- Probably need to split otn-label-step into two groupings, one for tributary-port, the other for tributary-slot.

- The usage of these grouping should probably be restricted via a when statement back to the type in the otn-label-range-info (but you can't do this abstractly in the grouping, you need to know the absolute or relative path).  So, either you need a higher level combined grouping, or some of the logic needs to be where the grouping is used.


> 
> 12) identity coding-func.
> - All of these identities should be before all of the groupings.
> [Authors] done in -05, the indentation looks good so far.
> - They also need to have their indentation fixed (e.g., "pyang -f yang --
> yang-line-length 69") - Are the references to MEF 63 the right choice, or
> should these references be to IEEE Std 802.3?
> [Authors] Discussion needed: Need to check the reference rule: the MEF63
> is the target we want to align, but IEEE 802.3 is the original one to
> track. MEF63 is referencing to IEEE 802.3 with corresponding clause, and
> it is propose not to repeat.
[RW] 
Okay.

It might be worth having a small paragraph in the main body of the draft to explain this, and perhaps add 802.3 as an informative reference.

> 
> 13) identity optical-interface-func
> - Are the references to MEF 63 the right choice, or should these
> references be to IEEE Std 802.3?
> [Authors] Discussion needed: Need to check the reference rule: the MEF63
> is the target we want to align, but IEEE 802.3 is the original one to
> track. MEF63 is referencing to IEEE 802.3 with corresponding clause, and
> it is propose not to repeat.
> - "base identity" => "Base identity"
> [Authors] done in -05.
> - Do you definitely want the "clause-XX" as part of each identity name?
> Is it possible that these clauses could ever change or be renumbered?
> [Authors] We adjusted as, "ETH-1000X-PCS-36" -> "ETH-1000X", "SX-PMD-
> clause-38"-> "SX-PMD-1000" (need to specify the rate to avoid
> duplication), refer to the updated model.
[RW] 
Okay.

> 
> 
> 14) identity service-performance-metric:
> - Rather than "list of service-specific ..." perhaps "Base identity for
> service specific ..." - Perhaps change "One-way-..." to "One-Way-...", it
> looks strange to have a single character not capitialized. - Probably
> remove the hypens from the descriptions?
> [Authors] done in -05, as most of the parameters in YANG is lower case, so
> we changed everything to lower case.
> 
> 15) identity network-performance-metric
> - Should any identities be defined here?
> - Rather than "list of network-specific ..." perhaps "Base identity for
> network specific ..."
> [Authors] This identity is removed as no identities defined here,
> according to MEF63.
[RW] 
Thanks,
Rob