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

[[e-mail]]

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]; [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