Re: [Teas] WG Last Call: draft-ietf-teas-yang-te-29

[Italo Busi <[e-mail]>]

Hi all,



I have reviewed the draft and I think it is ready for publication



I have few small comments which I think can be addressed as part of WG LC:


Technical Comments:


1)      Figure 1:

This comment is more applicable to draft-ietf-teas-yang-te-mpls-03 but its resolution has an impact on this Figure

Should the ietf-te-mpls augment both ietf-te and ietf-te-device, as in draft-ietf-teas-yang-te-mpls-03 and shown in this figure, or should it be split into ietf-te-mpls and ietf-te-mpls-device?

In the former case, it is not fully clear how the ietf-te-mpls could be used on a network controller not implementing the ietf-te-device model


2)      YANG ietf-te module (Line 7):



Based on the previous discussions on CCAMP and TEAS WG mailing lists, the prefix could be changed to “te-tnl”



OLD

  prefix te;


NEW

  prefix "te-tnl";


3)      YANG ietf-te module (Lines 112-119):



I am not sure RFC8685 is the right reference for the path-computation-error-no-inclusion-hop (I cannot find a definition for this error in RFC8685): it is proposed to remove the reference



OLD

  identity path-computation-error-no-inclusion-hop {

    base path-computation-error-reason;

    description

      "Path computation has failed because there is no

       node or link provided by one or more inclusion hops.";

    reference

      "RFC8685";

  }

NEW

  identity path-computation-error-no-inclusion-hop {

    base path-computation-error-reason;

    description

      "Path computation has failed because there is no

       node or link provided by one or more inclusion hops.";

  }



4)      YANG ietf-te module (Lines 157-164):



The path-computation-error-p2mp identity would better be defined in new p2mp TE Tunnel model rather than in this model:



  identity path-computation-error-p2mp {

    base path-computation-error-reason;

    description

      "Path computation has failed because of P2MP reachability

       problem.";

    reference

      "RFC8306";

  }



5)      YANG ietf-te module (Lines 296-310):



I am not sure the path-ref type is working as expected:



  typedef path-ref {

    type union {

      type leafref {

        path "/te:te/te:tunnels/te:tunnel/"

           + "te:primary-paths/te:primary-path/te:name";

      }

      type leafref {

        path "/te:te/te:tunnels/te:tunnel/"

           + "te:secondary-paths/te:secondary-path/te:name";

      }

    }

    description

      "This type is used by data models that need to reference

       configured primary or secondary path of a TE tunnel.";

  }



At least it should use relative paths rather than absolute paths to identify a path within the tunnel the external command has been applied to



Moreover, how is the union disambiguated? Maybe an additional attribute should be defined



6)      YANG ietf-te module (Lines 553-570)



I am not sure these XPaths are correct (see https://datatracker.ietf.org/doc/html/rfc7950#section-10.3.1.1):



OLD

        leaf tunnel-name {

          type leafref {

            path "/te:te/te:lsps/te:lsp/te:tunnel-name";

          }

          description "TE tunnel name.";

        }

        leaf node {

          type leafref {

            path "/te:te/te:lsps/te:lsp/te:node";

          }

          description "The node where the LSP state resides on.";

        }

        leaf lsp-id {

          type leafref {

            path "/te:te/te:lsps/te:lsp/te:lsp-id";

          }

          description "The TE LSP identifier.";

        }

NEW

        leaf tunnel-name {

          type leafref {

            path "/te:te/te:lsps/te:lsp/te:tunnel-name";

          }

          description "TE tunnel name.";

        }

        leaf lsp-id {

          type leafref {

            path "/te:te/te:lsps/te:lsp[tunnel-name="

               + "current()/../tunnel-name]/te:lsp-id";

          }

          description "The TE LSP identifier.";

        }

        leaf node {

          type leafref {

            path "/te:te/te:lsps/te:lsp[tunnel-name="

               + "current()/../tunnel/name lsp-id="

               + "current()/../lsp-id]/te:node";

          }

          description "The node where the LSP state resides on.";

        }



7)      YANG ietf-te module (Lines 650-661)



Since the error-node-id and error-link-id are ro, their default values could be removed



Moreover, do the default values 0.0.0.0 and 0 have any special meaning (e.g., unknown)? If so, it would be better to define this meaning in the description statement.



OLD

        leaf error-node-id {

          type te-types:te-node-id;

          default "0.0.0.0";

          description

            "Node identifier of node where error occurred.";

        }

        leaf error-link-id {

          type te-types:te-tp-id;

          default "0";

          description

            "Link ID where the error occurred.";

        }

NEW

        leaf error-node-id {

          type te-types:te-node-id;

          description

            "Node identifier of node where error occurred.";

        }

        leaf error-link-id {

          type te-types:te-tp-id;

          description

            "Link ID where the error occurred.";

        }



8)      YANG ietf-te module (Lines 666-751):



Since the attributes within the protection-restoration-properties-state grouping  are ro, their default values can be removed



  grouping protection-restoration-properties-state {

    <...>

  }


9)      YANG ietf-te module (Lines 756-774):



Within the protection container it looks like there is some duplication between the enable and the protection-type: is it possible to have enable=true and protection-type="te-types:lsp-protection-unprotected"?



    container protection {

      description

        "Protection parameters.";

      leaf enable {

        type boolean;

        default "false";

        description

          "A flag to specify if LSP protection is enabled.";

        reference

          "RFC4427";

      }

      leaf protection-type {

        type identityref {

          base te-types:lsp-protection-type;

        }

        default "te-types:lsp-protection-unprotected";

        description

          "LSP protection type.";

      }



If not, the protection can be enabled by setting the protection-type<>"te-types:lsp-protection-unprotected" and the enable leaf could be removed from the model.



Alternatively, to keep consistency between the protection and restoration configuration, it is also possible to either:

a)       Keep the enable leaf; change/remove the default value for the protection-type leaf and deprecate the te-types:lsp-protection-unprotected (already defined in RFC8776); or

b)      Remove the enable leaf also from the restoration container and add a new “te-types:lsp-restoration-type” (e.g., te-types:lsp-restoration-restore-none) to disable restoration



10)   YANG ietf-te module (Lines 849-858):



Should the default value for the hold-off-time in the restoration container be 0 (like in the protection container)?



OLD

      leaf hold-off-time {

        type uint32;

        units "milli-seconds";

        description

          "The time between the declaration of an SF or SD condition

           and the initialization of the protection switching

           algorithm.";

        reference

          "RFC4427";

      }

NEW

      leaf hold-off-time {

        type uint32;

        units "milli-seconds";

        default "0";

        description

          "The time between the declaration of an SF or SD condition

           and the initialization of the protection switching

           algorithm.";

        reference

          "RFC4427";

      }



11)   YANG ietf-te module (Lines 979-1001):



Depending on TEAS WG decision on draft-busi-teas-te-types-update, if the draft is adopted, the encoding-and-switching-type grouping can be removed and the grouping defined in the updated ietf-te-types used instead:



  grouping encoding-and-switching-type {

    <...>

  }



12)   YANG ietf-te module (Lines 1061-1090):



What is the meaning of the default values within the hierarchical-link container?



      container hierarchical-link {

        description

          "Identifies a hierarchical link (in client layer)

           that this tunnel is associated with.";

        reference

          "RFC4206";

        leaf local-te-node-id {

          type te-types:te-node-id;

          default "0.0.0.0";

          description

            "The local TE node identifier.";

        }

        leaf local-te-link-tp-id {

          type te-types:te-tp-id;

          default "0";

          description

            "The local TE link termination point identifier.";

        }

        leaf remote-te-node-id {

          type te-types:te-node-id;

          default "0.0.0.0";

          description

            "Remote TE node identifier.";

        }

        uses te-types:te-topology-identifier {

          description

            "The topology identifier where the hierarchical link

             supported by this TE tunnel is instantiated.";

        }

      }



13)   YANG ietf-te module (Lines 1615-1704):



Since the attributes within the lsp-properties-state grouping  are ro, their default values can be removed:



  grouping lsp-properties-state {

    <...>

  }



14)   YANG ietf-te module (Lines 1711-1712):


  container te {

    presence "Enable TE feature.";

I am not sure the requirement to enable TE on a system-level is technology and device independent. At least, for Optical domain controllers TE shall always be enabled.

Maybe an enable leaf with no default would better fit the purpose: not yet sure if this leaf should be defined in the ietf-te (as device independent) or in the ietf-te-device (as device-specific).

Editorial Comments:


15)   Abstract



The text in the abstract and introduction assumes that the draft defines the model also for MPLS-TE tunnels which used to be part of old versions of this draft but later has been moved to another draft



OLD

   (LSPs), and interfaces.  The model is divided into YANG modules that

   classify data into generic, device-specific, technology agnostic, and

   technology-specific elements.

NEW

   (LSPs), and interfaces.  The model cover technology agnostic data

   and it  is divided into YANG modules that  classify data into

   device independent and device-specific elements.



16)   Introduction



See previous comment



OLD

   tunnels, Label Switched Paths (LSPs), and interfaces.  The model

   covers data applicable to generic or device-independent, device-

   specific, and Multiprotocol Label Switching (MPLS) technology

   specific.

NEW

   tunnels, Label Switched Paths (LSPs), and interfaces.  The model

   cover technology agnostic data and it  is divided into YANG modules

   that  classify data into device independent and device-specific

   elements.



17)   Figure 1



Also the ietf-rsvp-te and ietf-rsvp modules are shown for illustration (not in this document)



18)   YANG ietf-te module (Line 55)



The copyright year shall be updated



19)   YANG ietf-te module (Line 74)



Usually the description of the first version of YANG models is “Initial revision.”

Nits:


20)   Section 5.1.2



OLD

   hierarchy:



      A YANG container that holds hierarchy related properties of the TE

      Tunnel (see Figure 6.  A TE LSP can be set up in MPLS or

NEW

   hierarchy:



      A YANG container that holds hierarchy related properties of the TE

      Tunnel (see Figure 6).  A TE LSP can be set up in MPLS or



21)   Section 5.1.2



OLD

   compute-only:



      A path of TE Tunnel is, by default, provisioned so that it can is

      instantiated in forwarding to carry traffic as soon as a valid

NEW

   compute-only:



      A path of TE Tunnel is, by default, provisioned so that it can be

      instantiated in the forwarding plane to carry traffic as soon as a valid



22)   Section 5.1.2



OLD

   computed-paths-properties: > A YANG container that holds properties

   for the list of computed paths.

NEW

   computed-paths-properties:



      A YANG container that holds properties

      for the list of computed paths.





Italo



> -----Original Message-----

> From: Lou Berger [mailto:[e-mail]]

> Sent: lunedì 21 marzo 2022 14:28

> To: TEAS WG <[e-mail]>

> Cc: TEAS WG Chairs <[e-mail]>

> Subject: [Teas] WG Last Call: draft-ietf-teas-yang-te-29

>

>

> All,

>

> This starts working group last call on

> https://datatracker.ietf.org/doc/draft-ietf-teas-yang-te/

>

> Given the size of the document and this week's meeting, this will be an

> extended LC. The working group last call ends on April 13th.

> Please send your comments to the working group mailing list.

>

> Positive comments, e.g., "I've reviewed this document and believe it is ready

> for publication", are welcome!

> This is useful and important, even from authors.

>

> Thank you,

> Lou (Co-Chair & doc Shepherd)

>

>

>

>