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

[Adrian Farrel <[e-mail]>]

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 <[e-mail]> On Behalf Of Lou Berger
Sent: 21 March 2022 13: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)




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