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

From: "Adrian Farrel" <[email-masked]>
Date: Sun, 10 Apr 2022 22:35:30 +0100

Hi,

I have reviewed this draft as it goes through WG last call.

I think there are a few nits to resolve before this is ready for 
publication.

Cheers,
Adrian

===

I think Xufeng has changed affiliation

---

1.

s/describes YANG data model/describes a YANG data model/

---

1.

   The document describes a high-level relationship between the modules
   defined in this document, as well as other external protocol YANG
   modules.

I think it would be nice to introduce (in this section, and before this
paragraph) the modules defined in this document.

---

1.

   The model
   covers data applicable to generic or device-independent, device-
   specific, and Multiprotocol Label Switching (MPLS) technology
   specific.

Something wrong with this sentence. Maybe...

   The model
   includes data that is generic, device-independent, device-specific,
   and specific to Multiprotocol Label Switching (MPLS) technology.

---

Throughout the document.

It isn't necessary to represent "optional plural(s)". In English, the
plural includes the singular so you can write (for example) "it is
expected other YANG module that model TE signaling protocols"

---

The section headings in Section 2 seem mixed up.

I think you need...

2. Terms and Conventions

2.1. Requirements Language

2.2. Terminology

2.3.  Prefixes in Data Node Names

2.4.  Model Tree Diagrams

---

3.

You begin with "This document describes a generic TE YANG data model
that is independent of any dataplane technology."

I feel that this point is quite important and is missing from the
Abstract and the Introduction.

---

3.

s/signaling protocol/signaling protocols/
s/respect data organization/respect to data organization/

---

3.

I don't know what this means:
   "can be used to model data off a device"

Is your point that this is not just for reading or writing information
from/to a device, but is also to allow a third party to hold a 
representation of the modeled entity? If so, great, but I think you have
only discovered the whole point of data modeling. If you mean something
else, it is not clear.

---

3.

      The device-specific TE data is defined in
      module 'ietf-te-device' as shown in Figure 1,

Do you mean Figure 1? Doesn't seem like the right figure.
Should end with a period not a comma.

---

3.

Aren't bullets 2 and 4 saying the same thing?

---

4.

   The
   support of extended or vendor specific TE feature(s) is expected to
   be in either augmentations, or deviations to the model defined in
   this document.

s/be in either/either be in/

However, the text is ambiguous because it could be read to mean that 
this document defines augmentations or deviations.

---

4.1

s/The TE data model/The TE data models/  (twice)

s/in a separate YANG module(s)/in separate YANG modules/

---

4.1

   defined in another document

Missing a citation

---

In Figure 1, is the text "o: augment" intended to be specific to the
one relationship it appears next to, or is it a general key explaining
the meaning of the "o" symbol.

I think you could tidy up Figure 1 as

     TE generic +---------+        
     module     | ietf-te |o-------------+
                +---------+               \
                       o                   \
                       |\                   \
                       | \ TE device module  \
                       |  +----------------+  \
                       |  | ietf-te-device |   \
                       |  +----------------+    \
                       |       o        o        \
                       |     /           \        \
                       |   /              \        \
        RSVP-TE +--------------+           +---------------+
        module  | ietf-rsvp-te |o          | ietf-te-mpls^ |
                +--------------+ \         +---------------+
                   |              \         MPLS-TE module
                   |               \
                   |                \
                   |                 \
                   |                  \
                   o               +-------------------+
         RSVP   +-----------+      | ietf-rsvp-otn-te^ |
         module | ietf-rsvp |      +-------------------+
                +-----------+       Module for OTN
                                    extensions to RSVP-TE 

     X---oY indicates that module X augments module Y
     
     ^ indicates a module defined in another document

---

5.

   The generic TE YANG module ('ietf-te') is meant to manage and operate
   a TE network.

Ah, but does it succeed?

Possibly...

   The generic TE YANG module ('ietf-te') is meant for management and
   operation of a TE network.

---

5.

   This includes creating, modifying and retrieving TE
   Tunnels, LSPs, and interfaces and their associated attributes (e.g.
   Administrative-Groups, SRLGs, etc.).

I don't think you retried Tunnels, LSPs, etc. Just information about
them.

---

5.

   The detailed tree structure is provided in Figure 2.

Not a lot of detail in Figure 2. Did you mean a different figure?

---

Section 5.1 is all a bit jumbled and would be a lot better as...

5.1.  Module Structure

   The 'te' container is the top-level container in the 'ietf-te'
   module.  The presence of the 'te' container enables TE function 
   system wide. 
   
   There are three further containers grouped under the 'te'
   container as shown in Figure 2 and described below.

   globals:

      The 'globals' container maintains the set of global TE attributes
      that can be applied to TE Tunnels and interfaces.

   tunnels:

      The 'tunnels' container includes the list of TE Tunnels that are
      instantiated.  Refer to Section 5.1.2 for further details on the
      properties of a TE Tunnel.

   lsps:

      The 'lsps' container includes the list of TE LSPs that are
      instantiated for TE Tunnels.  Refer to Section 5.1.3 for further
      details of the properties of a TE LSP.

   The model also contains two Remote Procedure Calls (RPCs) as shown
   in Figure 2 and described below.

   tunnels-path-compute:

      An RPC to request path computation for a specific TE Tunnel.  The
      RPC allows requesting path computation using atomic and stateless
      operation.  A tunnel may also be configured in 'compute-only' mode
      to provide stateful path updates - see Section 5.1.2 for further 
      details.

   tunnels-action:

      An RPC to request a specific action (e.g. reoptimize, or tear-and-
      setup) to be taken on a specific tunnel or all tunnels.

   Figure 2, shows the relationships of these containers and RPCs within 
   the 'ietf-te' module.

   module: ietf-te
      +--rw te!
         +--rw globals
            .
            .

         +--rw tunnels
            .
            .

         +-- lsps

   rpcs:
      +---x tunnels-path-compute
      +---x tunnels-action

            Figure 2: TE Tunnel model high-level YANG tree view

---

5.1.1

OLD
   The 'globals' container covers properties that control TE features
   behavior system-wide, and its respective state (see Figure 3).  The
   TE globals configuration include:
NEW
   The 'globals' container covers properties that control a TE feature's
   behavior system-wide, and its respective state as shown in Figure 3
   and described in the text that follows.
END

---

5.1.1

s/list named/list of named/
s/The path constraint/The path constraints/

---

5.1.1

OLD
      formed up of the following path
      constraints:
NEW
      formed of the path constraints shown in Figure 4.
END

---

5.1.1

OLD
         o  setup/hold priority: A YANG leaf that holds the LSP setup
            and hold admission priority as defined in [RFC3209].
NEW
         o  setup/hold priority: YANG leafs that holds the LSP setup
            and hold admission priority as defined in [RFC3209].
END

---

5.1.1

s/exclusions instruction/exclusion instructions/  (three times)

---

5.1.1

         3.  An empty 'route-object-include-exclude' list for a reverse
             path means it always follows the forward path (i.e., the TE
             Tunnel is co-routed).  When the 'route-object-include-
             exclude' list is not empty, the reverse path is routed
             independently of the forward path.

How would you show that the reverse path is routed independently, but
that there are no constraints on the path? Do you have to include a
random link that couldn't possibly be on the path just to make this
point?

---

5.1.2

   TE Tunnel:

      A YANG container of one or more LSPs established between the
      source and destination TE Tunnel termination points.  A TE Tunnel
      LSP is a connection-oriented service provided by the network layer
      for the delivery of client data between a source and the
      destination of the TE Tunnel termination points.

Figure 5 shows nothing related to LSPs.

I had to look down at Figure 8 to find where LSPs sit in the tree.
There I found that there are some mentions under 'tunnels' but mainly
deeper under the various "path" branches such as 'primary-paths'.

Perhaps the problem here is that you have introduced LSPs before talking
about tunnel paths (5.1.2.1> so that it is necessary to read through to
the end of 5.1.3 in order to see how it all hangs together.

---

5.1.2

The text describing the leaf nodes after Figure 5 does not appear to be
in the same order as the tree diagram. 'description' and 'admin-state'
are not described.

There is no mention of whether there is any scope of uniqueness for the
'alias'.

To be clear (and deducing from the reference), the 'color' is only to be
used when the tunnel is exposed in BGP as described in RFC 9012 and has
no other use.

Great that you have the encoding and switching type and reference RFC
3945 for their meanings. I'd note that RFC 8779 actually references  
RFC 3471 for the meaning of these two things.

I wondered by you also didn't show the G-PID (or encapsulation type) 
along with encoding and switching type. One might assume that this is
only of interest to the end points, but it is important for management
and for getting traffic into and out of the tunnel. But I see that
RFC 8779 doesn't seem to cover that either. Any thoughts?

s/inteval/interval/

I'm OK with what you have, but note that your definitions of source
and destination seem to preclude ingress and egress protection schemes.

The text at the top of the section...
   When
   the model is used to manage a TE controller, the 'tunnels' list
   contains all TE Tunnels and TE tunnel segments originating from
   device(s) that the TE controller manages.
...is confusing when considered in the context of the 'controller'
leaf.

Somewhere in the document you should be noting the implication of
setting 'bidirectional' to true for the meaning of the various
reverse-path objects.

You have:
      The TE tunnel associations
      can be overridden by associations configured directly under the TE
      Tunnel path.
Is it true that they are overridden? Or are they "supplemented"? That is,
if you say that "these two tunnels are associated" is there anything you
can do in the configuration of the paths to say that they are not?

s/(see Figure 6./(see Figure 6)./
s/as a TE links/as a TE link/

---

5.1.2.1

s/can be specified a set/can be specified as a set/
s/describe further/describes further/
s/The following set common path/The following set of common path/

      A primary
      path is identified by 'name'.

So, what is the uniqueness property required here? Just unique within
the tunnel?

Looking at 'preference', which first appears here, it is clear from the
Description of 'path-preference' how preferences are ranked, but there
is nothing to say how you distinguish between to equal values. Possibly
that situation isn't allowed, but there is nothing said about that.

---

5.1.2.1

   A secondary path contains attributes similar to a primary path.

How "similar"? What are the differences?

s/holds teh set/holds the set/
s/following set common path/following set of common path/
s/A path of TE Tunnel is/A path of a TE Tunnel is/
s/so that it can is/so that it can be/
s/instantiated in forwarding/instantiated in the forwarding plane/

---

5.1.2.1

      In some cases, a TE path may be provisioned for
      the only purpose of computing a path and reporting it without the
      need to instantiate the LSP or commit any resources.

"Provisioned" may not be the best word. It usually means "provisioned
in the network" which normally means "instantiated". How about 
"configured".

s/for the only purpose/only for the purpose/
s/a YANG container/A YANG container/  (twice)

---

5.1.3

   The 'lsps' container includes the set of TE LSP(s) that are
   instantiated.

Should that be s/that are/that have been/

---

5.3

Looks like the copyright statement in the ietf-te module is old.

---

5.3

In path-computation-error-reason you are capturing some of (all?) of the
PCE No-Path-Vector bits in the NO-PATH-VECTOR TLV Flag Field registry at
https://www.iana.org/assignments/pcep/pcep.xhtml#no-path-vector-tlv.

It is highly confusing that, although you give the same references for
the explanations, you don't use exactly the same wording. It would also
be really helpful to list them in the same order as in the registry - at
the very least, this would help me work out which reasons you have left
out so that I can ask you why you left them out.

But, that brings up another point. It seems to be messy that you have
defined a list of reasons here, but if new reasons get assigned in the
registry, each one has to be added through a new augmentation. I think
the conventional way to handle this is through an IANA managed module
that you include.

---

5.3  Sadly (and my fault for not picking this up during the review of
RFC 8776 before publication) te-types defines the base type
'association-type' which is actually a mirror of the IANA registry at
https://www.iana.org/assignments/pcep/pcep.xhtml#association-type-field

This gives us all a problem when new association types are introduced.
Indeed, you don't have "SR Policy Association" in the base type. That
is probably the first of many new associations that will get added.

Again, this should probably have been in an IANA maintained module that
could be included.

Not sure whether this is something you can fix here. It probably needs:
- Define the IANA YANG module (in this document or a separate document
  with a request for IANA to maintain it)
- Import the IANA module in ietf-te
- Make a bis of 8776 to drop association-type-field and import the new
  IANA module

The third of these might be optional, although it seems like a good 
idea.

This all seems a lot better than what you have done here which is 
define a new association type to match "Disjoint Association" (annoying
that you gave it a different name!) that was left out of 8776. What you
have done here means that someone else who wants to also use that 
association type must define it themselves or import your whole module!

---

5.3

Why do you use 5512 as a reference for protocol-origin-bgp when it has
been obsoleted by 9012? Why are neither 5512 nor 9012 mentioned in the
preamble txt at the top of 5.3?

---

5.3

Wondering why primary-reverse-path gets a Reference when primary-path,
secondary-path, and secondary-reverse-path don't. Even the reference for
primary-reverse-path is a little thin since 7551 doesn't mention 
"primary" in the whole document.

Why doesn't primary-reverse-path have a path-preference?

---

5.3

       leaf compute-only {
         type empty;
         description
           "When set, the path is computed and updated whenever
            the topology is updated. No resources are committed
            or reserved in the network.";
       }

"computed and updated whenever the topology is updated" Do you mean 
this? Surely there are some thresholds and relevance tests?

When compute-only is set, are a number of other leaf nodes meaningless
and to be ignored?

The text "When set" should probably be "When present"

---

5.3

I think that the Description for use-path-computation may not cover
what you intend. You have:

           "When 'true' indicates the path is dynamically computed
            and/or validated against the Traffic-Engineering Database
            (TED), and when 'false' indicates no validation against
            the TED is required.";

So, if the path is incomplete (i.e., computation is required) and this
field is set to false, is the path computed? Or what happens?

The text further up the document doesn't mention "validation".

---

5.3

     /* This grouping will be re-used in path-computation rpc */

s/will be/is/  ?  (several times)

---

5.3

The only mention of path-scope is when it appears in the module itself.
Here the Description tells use the meaning, but not how it gets set by
the implementation (yes, I see it is read-only).

8776 doesn't help with this (just defining the base type).

More words are needed.

---

5.3

     grouping k-requested-paths {
       description
         "The k-shortest paths requests.";
       leaf k-requested-paths {
         type uint8;
         default "1";
         description
           "The number of k-shortest-paths requested from the path
            computation server and returned sorted by its optimization
            objective. The value 0 all possible paths.";
       }
     }

Isn't "all possible paths" a potential exposure? You seem to recognise
this by limiting the requested number of paths to 255. But then you 
specifically allow "all" which could be a very large number of paths.

I think you should cap this by allowing the "path computation server"
(wow, there is some old terminology ;-) to limit how many paths it
returns in all cases, and in any case saying that it must not return 
more than 255 paths.

That said, 255 is still a lot of paths especially if you are doing 
recomputations every time the topology is updated.

---

5.3

I think I am a little disappointed that lsp-provisioning-error-info
only has a string to describe the provisioning error: no numeric code
or identity.

---

Seems you have some things like "When set to 'True'" and "If 'False'"
These should be 'true' and 'false'.

---

5.3

       leaf lsp-protection-state {
         type identityref {
           base te-types:lsp-protection-state;
         }
         default "te-types:normal";
         description
           "The state of the APS state machine controlling which
            tunnels is using the resources of the protecting LSP.";
       }

Either "tunnel is" or "tunnels are"

Why does this text talk about the APS state machine when 8776 makes
reference to 4427? Possibly the references in 8776 are wrong and should
have pointed to 7271 and 8234. If you intend those meanings of APS then
you should simply add references to your document.

(Raising an Errata Report against 8776 for this would be a bonus)

---

5.3

I think the reference for aps-signal-id is wrong.

---

5.3

Shouldn't the hold-off, wait-to-restore, and wait-to-revert timers have
defaults?

---

5.3

Why are src-tunnel-tp-id and dst-tunnel-tp-id  of type binary and not
te-tp-id?

---

5.3

           leaf protection-group-ingress-node-id {
             type te-types:te-node-id;
             description
               "When specified, indicates whether the action is
                applied on ingress node.
                By default, if neither ingress nor egress node-id
                is set, the action applies to ingress node only.";
           }
           leaf protection-group-egress-node-id {
             type te-types:te-node-id;
             description
               "When specified, indicates whether the action is
                applied on egress node.
                By default, if neither ingress nor egress node-id
                is set, the action applies to ingress node only.";
           }

The "by default" language here implies that the ingress node id is
known even when not set here. In that case, why do you need these to
be of type te-node-id? Wouldn't boolean do just as well?

BTW, this is not "by default". It is "if" since you are defining a 
meaning that has no other override.

---

5.3

Would you consider renaming the lsp-record-route-information-state etc.
to lsp-actual-route-information-state etc. and dropping mention of RSVP?
This might prove to be more useful in the general (non-RSVP-TE) case.

But, as I read a bit further beyond this grouping, I found a lot of 
stuff that was expressed in terms of RSVP-TE. Surely, either that should
be generalised to be in this model, or it should be moved out into the
RSVP-TE augmentation? Random example...

       leaf destination {
         type te-types:te-node-id;
         description
           "The tunnel endpoint address extracted from SESSION object.";
         reference
           "RFC3209";
       }

---

6.1.1

s/interface attributes/Interface attributes/

---

6.2

   Figure 11 shows the tree diagram of the device TE YANG model defined
   in modules 'ietf-te.yang'.

Possibly 'ietf-te-device.yang'

---

6.3

Copyright date

---

The two modules seem a bit confused about where the controls for 
individual LSPs go. the base module has many things for configuring and
operating individual LSPs, but the device model has the LSP timers that
are applicable only at the ingress nodes for LSPs. Surely, those timers
are also specific to the LSPs and could be grouped in the base model.

That said, the Description "Applicable to ingress LSPs only" may be
missing an "of" or maybe s/P/R/

And anyway, the Descriptions of the timers are a bit vague. For example,
does life-time report how long the LSP has been up or say after how long
it will be torn down?

---

Section 8 appears to be empty!

---

Section 13 needs to use IP addresses from the documentation range not
real routable addresses.

---

I think all three of [RFC4427], [RFC8800], and [RFC9012] are used as
explanations of items in the modules and so they should be normative
references.

-----Original Message-----
From: Teas <[email-masked]> On Behalf Of Lou Berger
Sent: 21 March 2022 13:28
To: TEAS WG <[email-masked]>
Cc: TEAS WG Chairs <[email-masked]>
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)

_______________________________________________
Teas mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/teas

---

From: t petch <[email-masked]>
Date: Mon, 11 Apr 2022 12:17:56 +0100

> This is useful and important, even from authors.

It has been a few years since I reviewed this so the
Copyright 2019
seems about right.

However, reading Adrian's comments brought back memories and indeed, my 
notes say 'three e-mail of comments' which suggests to me that they were 
never acted on:-)

So
- there are ten references in the YANG to RFC that do not appear in the 
I-D references (used to be four)

- abbreviations need expanding, I suggest in s.2 with a separate heading 
to the current s.2

- http: needs to be https:

- the TLP are out of date (that's new)

- the format of the tree diagram is clumsy - identifiers are split in 
the middle, across lines

- revision should be 'Initial ..' not 'latest'

- /constraitnt/constraint/

I will plough on - my target is by Easter, ie COB 14th April

Tom Petch

---

From: Rakesh Gandhi <[email-masked]>
Date: Mon, 11 Apr 2022 13:54:04 -0400

Hi Lou, WG,
I have reviewed this document and believe it is ready for publication.
Thanks,
Rakesh (as a co-author)

---

From: Italo Busi <[email-masked]>
Date: Wed, 13 Apr 2022 07:44:30 +0000

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 {

          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 {

          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 {

          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 {

          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

   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

> From: Lou Berger [mailto:[email-masked]]

> Sent: lunedì 21 marzo 2022 14:28

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

> for publication", are welcome!

---

From: t petch <[email-masked]>
Date: Thu, 14 Apr 2022 12:15:10 +0100

>> This is useful and important, even from authors.

I have not reviewed the document - it is too complex, largely due to the 
way in is constructed - but have seen enough to think, 'Not ready for 
publication'.

Security has the concept of number used once, soomething which makes it 
harder for anyone else to know what is going on.  The equivalent in YANG 
is 'grouping used once' which I often see YANG Doctors point out and ask 
to be removed since they increase complexity, reduce ease of use. This 
I-D does just that and goes further, nesting groupings extensively which 
makes it even harder, verging on impossible, to understand.

I start at a high level, as in Section 5, intending to 'drill down' to 
the lower levels, leaf and its type and description, to see if they make 
sense, if they do what is wanted, and the highest level is good; the 
English needs some tweaking but nothing I am unclear about.

With s.5.1.1, I get uncertain - where is the description of path 
in-segment and path out-segment that feature in the tree diagram?

With s.5.1.2 I know that something is wrong.  The YANG snippets are not 
those of this I-D; they would help if they did but they relate to a 
different I-D.  Thus the YANG has, buried in a grouping used once,
leaf description
leaf admin-state
leaf operational-state
s.5.1.2 has
description
operational-state
encoding
a related, but different, module.

Likewise, the YANG has bidirectional after dst-tunnel-tp-id in another 
grouping used once, not in this snippet.  At which point, I cry 'Hold, 
enough'; I cannot trust what I am reading.

Another problem, besides difficulty of understanding, with grouping used 
once, is the difficulty of relating the different parts of the I-D, 
description, tree diagram, YANG and so on.  The obvious solution is 
comment lines showing where globals ends and tunnel starts, in the YANG, 
in the tree diagram, in the introductory tex.  That is pointless here 
since in the YANG, they are adjacent 'uses' and at a more detailed 
level, globals, tunnels and so on are all jumbled up.

The IETF has abolished the page number so you will have 40 
undifferentiated pages of tree diagram followed by 40 undifferentiated 
pages of YANG, but with objects in a different order, courtesy of 
grouping and grouping used once.

Tom Petch

---

From: Lou Berger <[email-masked]>
Date: Thu, 14 Apr 2022 07:51:52 -0400

Tom,

Thank you for this (well at some level ;-)

Authors,

Can you work with Tom, perhaps even schedule some time to meet - 
assuming he's willing/available - and come up with an approach to 
address his comments.

Thanks,

Lou

PS I still like my page numbers so use the htmlized link on datatracker, 
e.g., https://datatracker.ietf.org/doc/html/draft-ietf-teas-yang-te  - 
the .txt version also has them...

---

From: Tarek Saad <[email-masked]>
Date: Mon, 18 Apr 2022 13:52:24 +0000

Thanks Lou, Tom and Adrian.
The authors thank Tom and Adrian for their review and comments, and are actively working on addressing them.
Authors will follow up with a meeting with reviewers if needed.

Regards,
Tarek (for co-authors)

From: Teas <[email-masked]> on behalf of Lou Berger <[email-masked]>
Date: Thursday, April 14, 2022 at 7:52 AM
To: t petch <[email-masked]>, TEAS WG <[email-masked]>
Cc: TEAS WG Chairs <[email-masked]>
Subject: Re: [Teas] WG Last Call: draft-ietf-teas-yang-te-29
Tom,

Authors,

Thanks,

Lou