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

Zhenghaomian <zhenghaomian@huawei.com> Fri, 17 January 2020 12:48 UTC

Return-Path: <zhenghaomian@huawei.com>
X-Original-To: ccamp@ietfa.amsl.com
Delivered-To: ccamp@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 3E82B120046; Fri, 17 Jan 2020 04:48:07 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -4.2
X-Spam-Level:
X-Spam-Status: No, score=-4.2 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RCVD_IN_DNSWL_MED=-2.3, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id SX0QhWgcQtqC; Fri, 17 Jan 2020 04:48:02 -0800 (PST)
Received: from huawei.com (lhrrgout.huawei.com [185.176.76.210]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 507D812003F; Fri, 17 Jan 2020 04:48:02 -0800 (PST)
Received: from lhreml701-cah.china.huawei.com (unknown [172.18.7.106]) by Forcepoint Email with ESMTP id 03C1032C5788C6CEB743; Fri, 17 Jan 2020 12:47:59 +0000 (GMT)
Received: from lhreml713-chm.china.huawei.com (10.201.108.64) by lhreml701-cah.china.huawei.com (10.201.108.42) with Microsoft SMTP Server (TLS) id 14.3.408.0; Fri, 17 Jan 2020 12:47:58 +0000
Received: from lhreml713-chm.china.huawei.com (10.201.108.64) by lhreml713-chm.china.huawei.com (10.201.108.64) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Fri, 17 Jan 2020 12:47:58 +0000
Received: from DGGEML405-HUB.china.huawei.com (10.3.17.49) by lhreml713-chm.china.huawei.com (10.201.108.64) with Microsoft SMTP Server (version=TLS1_0, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA) id 15.1.1713.5 via Frontend Transport; Fri, 17 Jan 2020 12:47:58 +0000
Received: from DGGEML511-MBX.china.huawei.com ([169.254.1.145]) by dggeml405-hub.china.huawei.com ([10.3.17.49]) with mapi id 14.03.0439.000; Fri, 17 Jan 2020 20:47:53 +0800
From: Zhenghaomian <zhenghaomian@huawei.com>
To: "Rob Wilton (rwilton)" <rwilton@cisco.com>, "yang-doctors@ietf.org" <yang-doctors@ietf.org>
CC: "last-call@ietf.org" <last-call@ietf.org>, "ccamp@ietf.org" <ccamp@ietf.org>, "draft-ietf-ccamp-layer1-types.all@ietf.org" <draft-ietf-ccamp-layer1-types.all@ietf.org>
Thread-Topic: Yangdoctors last call review of draft-ietf-ccamp-layer1-types-04
Thread-Index: AdXNNFLvUcFE+Wj7RPWEeQF8aquoEA==
Date: Fri, 17 Jan 2020 12:47:52 +0000
Message-ID: <E0C26CAA2504C84093A49B2CAC3261A43B95BF55@dggeml511-mbx.china.huawei.com>
Accept-Language: zh-CN, en-US
Content-Language: zh-CN
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-originating-ip: [10.24.178.249]
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: base64
MIME-Version: 1.0
X-CFilter-Loop: Reflected
Archived-At: <https://mailarchive.ietf.org/arch/msg/ccamp/Ccpk2vtPecxZhFLTWQMidxldOc0>
Subject: Re: [CCAMP] Yangdoctors last call review of draft-ietf-ccamp-layer1-types-04
X-BeenThere: ccamp@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
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: Fri, 17 Jan 2020 12:48:07 -0000

Hi, Robert, 

Thank you for the feedback, please see inline with [Authors2] as well. 

We will send updated pull request after we address the open issues on default values, probably in a few days. 

Best wishes,
Haomian


-----邮件原件-----
发件人: Rob Wilton (rwilton) [mailto:rwilton@cisco.com] 
发送时间: 2020年1月16日 19:26
收件人: Zhenghaomian <zhenghaomian@huawei.com>om>; yang-doctors@ietf.org
抄送: last-call@ietf.org; ccamp@ietf.org; draft-ietf-ccamp-layer1-types.all@ietf.org
主题: RE: Yangdoctors last call review of draft-ietf-ccamp-layer1-types-04

Hi Haomian,

Please see inline ...

> -----Original Message-----
> From: Zhenghaomian <zhenghaomian@huawei.com>
> Sent: 25 December 2019 06:22
> To: Rob Wilton (rwilton) <rwilton@cisco.com>om>; yang-doctors@ietf.org
> Cc: last-call@ietf.org; ccamp@ietf.org; draft-ietf-ccamp-layer1- 
> types.all@ietf.org
> 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:noreply@ietf.org]
> 发送时间: 2019年12月14日 0:48
> 收件人: yang-doctors@ietf.org
> 抄送: last-call@ietf.org; ccamp@ietf.org; draft-ietf-ccamp-layer1- 
> types.all@ietf.org
> 主题: 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.
[Authors2] noted, will use 'l1-types' as the prefix.

> 
> 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.

[Authors2] Thanks for clarifying our concerns on enumeration, it's solved now for standard version, but not easy for vendor to define their own characteristics. For our second concern, on the 'type configuration' with 'value configuration', we still have multiple places to negotiate. This also applies to the comment #7, #8 in this draft, and a few other places for layer0-types.

> 
> 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.
[Authors2] Okay, let's make it normative now.

> 
> 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?
[Authors2] probably not for the otn-label-range-type, the concern for enumeration would be if there is anything vendor-specific in future, not very likely.
> 
> 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?
[Authors2] Yes, the tsg is always needed because in some cases different tsg can support different tpn-ranges, refer to section 4.3 in this draft.

> 
> 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 ...).
[Authors2] Okay, we will use 'otn-tpn' and 'otn-ts'.

>  - 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.
[Authors2] Okay.

> 
> 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.
[Authors2] We will remove the default value here, and see the next 'authors2' comments for more discussion.

> [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.
[Authors2] Thanks for the advice, we will decide after we look into the ietf-otn-topology. When the otn topology model augments the te-topology, this grouping is also intended to be used under another case choice, so we also need to figure out how to set the default value.
See the new open issue on the github for tracking the problem: https://github.com/haomianzheng/IETF-ACTN-YANG-Model/issues/54

> 
> 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.
[Authors2] okay, will add this in -05.
> 
> 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