Re: [CCAMP] Yangdoctors last call review of draft-ietf-ccamp-layer0-types-03

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

Hi Haomian,

Sorry, I had missed your email over Christmas.  Replying back to the original email to capture the other aliases.

Please see inline ...



> -----Original Message-----
> From: last-call <[e-mail]> On Behalf Of Zhenghaomian
> Sent: 25 December 2019 08:20
> To: Rob Wilton (rwilton) <[e-mail]>; [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [Last-Call] Yangdoctors last call review of draft-ietf-ccamp-
> layer0-types-03
> 
> Hi, Robert,
> 
> Again, thank you very much for your detailed review and comments. The
> authors had a few iterations of discussions and give the update for this
> document as follow.
> 
> Model update can be found as the pull request: https://github.com/ietf-
> ccamp-wg/draft-ietf-ccamp-layer0-types/pull/3 ; 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 starting with
> '[Authors]';
> 
> Please help review if the comments are addressed, any discussion/calls are
> welcome and can be arranged by chairs, thank you.
> 
> Best wishes,
> Haomian (on behalf of all authors & contributors)
> 
> -----邮件原件-----
> 发件人: Robert Wilton via Datatracker [mailto:[e-mail]]
> 发送时间: 2019年12月14日 0:50
> 收件人: [e-mail]
> 抄送: [e-mail]; [e-mail];
> [e-mail]
> 主题: Yangdoctors last call review of draft-ietf-ccamp-layer0-types-03
> 
> Reviewer: Robert Wilton
> Review result: Almost Ready
> 
> Comments on the document:
> 
> 1) I would delete the "overview" paragraph at the top of section 2, and
> just promote section 2.1 as section 2.
> [Authors] done in -04.
> 
> 2) 2.1. Layer 0 Types Module Contents:
> 
> The descriptions are good, but I would suggest formatting these as a
> table, or more tightly link the definition to it's description.
> 
> E.g.
> 
> "Operational-mode: A type that represents operational mode as defined in
> [ITU-Tg6982]."
> 
> Instead of:
> 
> "Operational-mode:
> 
> A type that represents operational mode as defined in [ITU-Tg6982]."
> [Authors] Discussion needed: we need to be careful on changing this, as
> currently the layer0-types and layer1-types are in consistent format. Need
> to change in both sides or don't change any of them.
[RW]
I agree that they should be consistent between the two drafts.  My aim was to make them as readable as possible, but this is purely a stylistic thing.


> 
> 3) I would define the module as YANG version "1.1" (because the language
> behaviour is generally better specified) and then reference only RFC 7950
> in the introduction.
> [Authors] done in -04.
> 
> 4) typo in the Security Considerations "layer0 => layer 0", and also in
> the title of section 3.
> [Authors] done in -04.
> 
> 5) I have suggested changing the module prefix to "l0-types" rather than
> layer2-types.  If you make this change then the IANA considerations would
> need to be updated, along with section 1.2.
> [Authors] done in -04.
> 
> Comments on the YANG module:
> 
> 1) I would suggest changing the YANG prefix to "l0-types" to help keep it
> short (particularly for the identities).
> [Authors] done in -04.
> 
> 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 -04.
> 
> 3) It is actually necessary to define frequency-thz at all?  I think that
> the discussion in the WG suggested that it might be better if the
> frequencies are always defined in Ghz and then converted in the client as
> necessary.
> [Authors] We have checked all the typedef, and agreed on removing
> frequency-thz and frequency-ghz together, as they are not referenced in
> later groupings. Need to further check whether the modules who import this
> type module would use such typedef. May need to add back if the answer is
> yes. Moreover, like flexi-n, another typedef flexi-m is added and
> referenced in this module.
[RW] 
I think that defining frequency-ghz is probably still helpful, it was only frequency-thz that I was suggesting removing, effectively encouraging everyone to standardize on using frequency-ghz for layer 0 properties/considerations.



> 
> 4) frequency-ghz: Is 3 fraction digits sufficient for future expansion?
> E.g.
> It would seem to support a flex grid 3.125Ghz channel spacing, but not
> 1.5625 ...
> [Authors] Discussion needed: currently the channel spacing is 6.25GHz, and
> we reserve the 3.125GHz for proof-of-future.  According to Singapore
> discussion, WG agreed on 3.125 with 3 fraction digits. We prefer to keep
> the current format given the following considerations:
> [Authors] Consideration 1: Technically it won't make much sense for Layer
> 0 to come down to 1.5625G, as layer 1 is providing the granularity at ODUk
> (<100Gbps) which is make very good use of such spectrums.
> [Authors] Consideration 2: furthermore, if 1.5625G happens in the future,
> can we keep fraction digits as 3 and specify the channel spacing as
> '1.562G'? There should not be difficulties in understanding, and we solve
> the problem forever.
> [Authors] Consideration 3: the channel spacing is not binded with the
> frequency on the grid, but a number... So maybe we can change the
> dependency on the grid as well.
[RW] 
Okay, keeping this to 3 dpi is fine.

> 
> 5) We should aim for consistency of the identity names in the layer-1
> types module.  E.g. perhaps OTU, ODU and OPU should be capitalized.
> [Authors] done in -04.
> 
> 6) The models uses identities for bandwidth, but I wonder whether defining
> a numerical typedef might be a better choice (e.g. more efficient and
> perhaps easier for programs to work with).  Here, I have constrained the
> values that are allowed, but they could also be unconstrained:
> 
> E.g.
> typedef channel-bandwidth {
>   type decimal64 {
>     fraction-digits 2;
>     range
>       "2.66|10.70|11.04|11.09|11.27|11.31|43.01|44.57|44.58|100.00 ...
> max"
>     description
>       "Bandwidth carried by a single wavelength channel"
>   }
>   units "Gb/s";
> }
> Or another alternative would be to use Mb/s, which would then allow them
> to be specified as a union of specific values and an arbitrary bandwidth
> value.
> 
> [Authors] Discussion needed: we hope this proposal is different with the
> ones on 'identity->enumeration' but maybe push back by extensibility,
> could you please confirm? Gbps is used for data plane and should be
> accurate enough to be described in the YANG module. We are open to the
> proposal, but need to understand the difference and how it affects the
> configuration. After that we can discuss whether decimal 64 is good, Gbps
> or Mbps, etc...
> 
[RW] 
Identities are just names of things.  So, it if the bandwidth value that clients and implementations care about, then I would expect them to need to write code to translate between the identity names and numerical values.  E.g. what does a client configuring the device primarily care about here.  Is it that the bandwidth is "otu2" or that it the bandwidth is "10.70G"?


> 7) Same comment as for bandwidth applies to the channel spacing
> identities.
> I.e. I wonder whether these wouldn't be better defined using a decimal64
> type.
> 
> E.g.
> typedef dwdm-channel-spacing {
>   type decimal64 {
>     fraction-digits 2;
>     range
>       "12.5|25|50|100"
>     description
>       "Bandwidth carried by a single wavelength channel"
>   }
>   units "Ghz";
> }
> [Authors] Discussion needed: probably reject, see the consideration about
> extensibility on layer1-types. 6.25/3.125 GHz is coming...
[RW] 
The model can always be extended to support 6.25/3.125 in future.  In either solution this would require a backwards compatible change to the model.

My hunch is that having this as a numerical value is more useful for clients than having it as a named identity.  I.e. I think that the argument for using a numerical value here is stronger than for channel bandwidth above.

> 
> 8) Same comment as for bandwidth and dwdm-channel-spacing could also be
> applied to flexi-grid-channel-spacing, flexi-slot-width-granularity, cwdm-
> channel-spacing.
> [Authors] Discussion needed: probably reject, see the consideration about
> extensibility on layer1-types.
[RW] 
These should be resolved with whatever the conclusion is above.

> 
> 9) In grouping layer0-label-range-info
> I would rename this grouping to l0-layer-range-info, change "layer0" =>
> "layer 0" in the description.  Also "priority" could do with a more
> detailed description as to what it means, etc.
> [Authors] Confirmation firstly: probably a mis-spelling, propose to 'l0-
> label-range-info'.
[RW] 
Yes.


> [Authors] Discussion secondly: there are multiple identities/typedef
> started with 'layer0-' and it should be better to keep consistency in
> naming format, do we rename all of them or none of them?
[RW] 
Yes, I think that I would rename all of them "l0-"

Thanks,
Rob


> [Authors] priority updated in -04.
> 
> 10) In grouping flexi-grid-label-start-end, I think that the type should
> be "flexi-n" rather than "int16".
> [Authors] done in -04,
> 
> Typos: "girds" => "grids", "attrtibutes" => "attributes"
> [Authors] done in -04,
> 
> Spacing/indenting needs to be fixed:
>  - In "grouping wson-label-hop", just before case cwdm
>  - In "grouping flexi-grip-label-hop", should have a blank line before,
> and  "case super" block/fields indentation doesn't look right. - In some
> of the  typedef definitions, the "{" should move from the start of the
> following line  to the typedef line. In general, as a starting point,
> after all other markups  have been made then it might be a good idea to
> use pyang to format the YANG  file for you, e.g., "pyang -f yang --yang-
> line-length 69", but probably with  some more blank lines, otherwise it is
> a bit dense.
> [Authors] done in -04, probably some compilation problems and will double
> check per update.