Re: [CCAMP] Rtgdir last call review of draft-ietf-ccamp-flexigrid-yang-13

[Daniele Ceccarelli <[e-mail]>]

Hi Dan, 

Thanks much appreciated.
I thought I submitted to the IESG the version addressing the shepherd comments as well as the rtg-dir ones but probably it was addressing on the first ones.

Since the draft will go through other rounds of review (security, management etc.) I would suggest not to issue a new version now to avoid a misalignment between the actual version and the submitted one. 
We can address the rtg-dir comments together with the ones coming from that reviews, just keep them ready.

Thanks
Daniele  

> -----Original Message-----
> From: Daniel King <[e-mail]> On Behalf Of [e-mail]
> Sent: Thursday, 25 August 2022 15:01
> To: 'Dhruv Dhody' <[e-mail]>; [e-mail]; 'tom petch'
> <[e-mail]>
> Cc: [e-mail]; [e-mail]; last-
> [e-mail]
> Subject: RE: Rtgdir last call review of draft-ietf-ccamp-flexigrid-yang-13
> 
> Thanks, Dhruv, Tom and Daniele.
> 
> We will work through these comments and fix the 'when' statements.
> 
> BR, Dan.
> 
> -----Original Message-----
> From: Dhruv Dhody via Datatracker <[e-mail]>
> Sent: 18 August 2022 17:22
> To: [e-mail]
> Cc: [e-mail]; [e-mail]; last-
> [e-mail]
> Subject: Rtgdir last call review of draft-ietf-ccamp-flexigrid-yang-13
> 
> Reviewer: Dhruv Dhody
> Review result: Has Issues
> 
> # RTGDIR review of draft-ietf-ccamp-flexigrid-yang-13
> 
> ## Major
> - None
> 
> ## Minor
> - Section 1, Introduction states ".."media-channel" defined in [RFC9093]."; I
> did not find "media-channel" in 9093. - Section 2, Terminology: suggest
> combining the text related to RFC7950 together, something like - ````
>    The terminology for describing YANG data models is found in
>    [RFC7950]. The following terms are defined in [RFC7950] and
>    are not redefined here:
> 
>    *  client
>    *  server
>    *  augment
>    *  data model
>    *  data node
> ````
> - Also consider adding TTP, LTP, etc in the Terminology
> - Section 4
>     - the purpose of this section is not clear to me. Are you trying to justify
>     why a topology model is needed? - Further, I suggest avoiding using the
>     phrase - "We define..", "we do..". - Maybe you could add some example
>     values for yang parameters for flexi-grid to make it more useful
> - Section 6, to increase the readability of the YANG tree, I suggest breaking
> them up and grouping similar augmentations together. - Security
> consideration section needs work
>     - it only mentions "rw", there are "ro" elements as well but not listed in
>     the security section. - List the impact on the system as well - In the list
>     of "rw", you list the paths where augmentation is done and not the new
>     leafs added via augmentation. I think the focus should be on the incorrect
>     flex-grid parameters setting at each of these levels.
> 
> ## YANG Model
> - The YANG model compiles with no error and is formatted correctly.
> - Happy to report that all the required places in ietf-te-topology model that
> requires augmentation are done. My eyes did hurt a bit after this exercise :) -
> The description in the YANG model is not the same as in the rest of the
> document, specifically: "This module provides a YANG data model for the
> routing and wavelength assignment (RWA) Traffic Engineering (TE) topology
> in flexi-grid optical networks.". There is no mention of RWA anywhere else. -
> The use of the absolute address in the when would be incorrect as you want
> to make sure that the current network is with the flexi-grid-topology!
> Everywhere else it is a relative path. ````
>   augment "/nw:networks/nw:network/nw:node/tet:te"
>         + "/tet:te-node-attributes" {
>     when "/nw:networks/nw:network/nw:network-types"
>        + "/tet:te-topology/flexgt:flexi-grid-topology" {
>       description
>       "Augmentation parameters apply only for networks with
>        flexi-grid topology type.";
>     }
>     description "Augment TE node attributes.";
>     container flexi-grid-node {
>       presence "The TE node is a flexi-grid node.";
>       description
>         "Introduce new TE node type for flexi-grid node.";
>     }
>   }
> ````
> - I notice a few augmentations do not have a when clause, I could not figure
> out why these ones should not... ````
>   augment "/nw:networks/tet:te/tet:templates/"
>         + "tet:link-template/tet:te-link-attributes/"
>         + "tet:label-restrictions/tet:label-restriction" {
>     description
>       "Augment TE label range information for the TE link template.";
>     uses l0-types:flexi-grid-label-range-info;
>   }
>   :
>   :
>     augment "/nw:networks/tet:te/tet:templates/"
>         + "tet:link-template/tet:te-link-attributes/"
>         + "tet:underlay/tet:primary-path/tet:path-element/tet:type/"
>         + "tet:label/tet:label-hop/tet:te-label/tet:technology" {
>     description
>       "Augment TE label hop for the underlay primary path
>        of the TE link template.";
>     case flexi-grid {
>       uses l0-types:flexi-grid-label-hop;
>     }
>   }
>     augment "/nw:networks/tet:te/tet:templates/"
>         + "tet:link-template/tet:te-link-attributes/"
>         + "tet:underlay/tet:backup-path/tet:path-element/tet:type/"
>         + "tet:label/tet:label-hop/tet:te-label/tet:technology" {
>     description
>       "Augment TE label hop for the underlay backup path
>        of the TE link template.";
>     case flexi-grid {
>       uses l0-types:flexi-grid-label-hop;
>     }
>   }
> 
>   augment "/nw:networks/tet:te/tet:templates/"
>         + "tet:link-template/tet:te-link-attributes/"
>         + "tet:label-restrictions/tet:label-restriction/"
>         + "tet:label-start/tet:te-label/tet:technology" {
>     description
>       "Augment TE label range start for the TE link template.";
>     case flexi-grid {
>       uses l0-types:flexi-grid-label-start-end;
>     }
>   }
> 
>   augment "/nw:networks/tet:te/tet:templates/"
>         + "tet:link-template/tet:te-link-attributes/"
>         + "tet:label-restrictions/tet:label-restriction/"
>         + "tet:label-end/tet:te-label/tet:technology" {
>     description
>       "Augment TE label range end for the TE link template.";
>     case flexi-grid {
>       uses l0-types:flexi-grid-label-start-end;
>     }
>   }
> 
>   augment "/nw:networks/tet:te/tet:templates/"
>         + "tet:link-template/tet:te-link-attributes/"
>         + "tet:label-restrictions/tet:label-restriction/"
>         + "tet:label-step/tet:technology" {
>     description
>       "Augment TE label range step for the TE link template.";
>     case flexi-grid {
>       uses l0-types:flexi-grid-label-step;
>     }
>   }
> }
> ````
> ## Nits
> - The use of the word "database" in the abstract stood out to me. Apart from
> abstract you dont use that word anywhere else. - I suggest removing the
> NMDA statement from the abstract, you have it covered in the Introduction.
> - s/it augments [RFC8795]/it augments ietf-te-topology model [RFC8795]/ -
> For all RFC-editor notes, suggest adding a prefix "[Note to RFC-EDITOR:]". - I
> suggest changing the title of section 5 - "YANG Data Model for Flexi-Grid
> Topology" to "Overview of Flexi-Grid Topology Model". The actual model is in
> section 7 and this would avoid any confusion. You can collapse section 5.1
> also. - Expand on first use: WDM - Section 5.1; s/augments from a
> more/augments a more/
> 
> This review is also available at -
> https://notes.ietf.org/draft-ietf-ccamp-flexigrid-yang?view
>