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

Tarek Saad <tsaad.net@gmail.com> Mon, 11 July 2022 15:17 UTC

Return-Path: <tsaad.net@gmail.com>
X-Original-To: teas@ietfa.amsl.com
Delivered-To: teas@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id EDBA6C0D7C4A; Mon, 11 Jul 2022 08:17:26 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -7.094
X-Spam-Level:
X-Spam-Status: No, score=-7.094 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_HI=-5, RCVD_IN_ZEN_BLOCKED_OPENDNS=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_KAM_HTML_FONT_INVALID=0.01, T_SCC_BODY_TEXT_LINE=-0.01, URIBL_BLOCKED=0.001, URIBL_DBL_BLOCKED_OPENDNS=0.001, URIBL_ZEN_BLOCKED_OPENDNS=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
Received: from mail.ietf.org ([50.223.129.194]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ce3OfFD-ylss; Mon, 11 Jul 2022 08:17:23 -0700 (PDT)
Received: from mail-io1-xd36.google.com (mail-io1-xd36.google.com [IPv6:2607:f8b0:4864:20::d36]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 4B0CAC188715; Mon, 11 Jul 2022 08:17:11 -0700 (PDT)
Received: by mail-io1-xd36.google.com with SMTP id 190so5203793iou.6; Mon, 11 Jul 2022 08:17:11 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:to:cc:subject:thread-topic:thread-index:date:message-id :references:in-reply-to:accept-language:content-language :mime-version; bh=H7Qnu2G81GdOaRh8w3pfOxstAKiu5LD6kElLDurM/A8=; b=JjX+zXNjCgmSDCWR8s0vDOPCBRYxKwrPoUU4hzEYdDxVBg8SWLDxCB3Z//tHNzdRsh BTfGf3AU1Ic1W/U6cDQKuD5fEhFSccAcvPmDjTlVyD/+2HaQvMZv969DRjYF0PkZ/iM3 DM1QfwoAoLQ5nJY6NmHaJ1nd8ZJOj4gvp5eEwUim5fehpB1plSoPBEnFVYY9/NkkilmC Vz5r6BwR8ymL8HTzvt/HK6guUx88J56Z0Y72b3ctwtDDEfRPVNsFvIljTg3ixyBpNWPl 47ypcbfYvzXTi1beOx5DU08bDxrsD0DHNsaYB2TxI+1gEoYlh0o++n+vFpjPpCkMDcjK fMBQ==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:thread-topic:thread-index :date:message-id:references:in-reply-to:accept-language :content-language:mime-version; bh=H7Qnu2G81GdOaRh8w3pfOxstAKiu5LD6kElLDurM/A8=; b=ngfkEHW2/l9m/KNJy/yX9F4wOTN+oHW5d21xuT6ZW0tPJlAOXWOy2iFTWLlWMWLu4F /ZVIuyg4Da6XZs8+nrt+mIfHrqkFcmOP4E1w7Lk+9kjJ98ZM+YsZedwIcLfr9CuMxre0 JqOYSq9I5Lw+DyDHPWQDb5XoR7MoL9/g+MqrW3YxKM8L7W3KPAng3YJTUHzvJIU+FKV3 9XsHesX7O2fSVgtbdGUZtNj+CfbiVFGxGJW6oOaz5C7WXaYNHvn+PajDRqjJvhWLvx33 sQA05ThaeUkZp9TlTiTdVWPJfl94PUuEZbMum/JrYeMW2RdsVtpujtfHdp9tnbWUNQCP TOuw==
X-Gm-Message-State: AJIora+uykjMF3sBykBDlWLwXvELl+5U1xwuwBuURrGpxZMwYxTEt/Bc Q2hyH0m2FJgzvqJ4jZSySBwIgXD8dnY=
X-Google-Smtp-Source: AGRyM1sazYaoFzNdgb4BxjMr0n3bA3s+wbTC4+xeDnTvQULR1AF41sAl/sXIWL9wn7gTLs5IDKqMjg==
X-Received: by 2002:a05:6602:1644:b0:678:8ba4:8df6 with SMTP id y4-20020a056602164400b006788ba48df6mr9593169iow.138.1657552629900; Mon, 11 Jul 2022 08:17:09 -0700 (PDT)
Received: from DS0PR19MB6501.namprd19.prod.outlook.com ([2603:1036:5:f::5]) by smtp.gmail.com with ESMTPSA id x6-20020a056638034600b00339f193b8ddsm2983560jap.130.2022.07.11.08.17.09 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 11 Jul 2022 08:17:09 -0700 (PDT)
From: Tarek Saad <tsaad.net@gmail.com>
To: Adrian Farrel <adrian@olddog.co.uk>
CC: TEAS WG <teas@ietf.org>, "teas-chairs@ietf.org" <teas-chairs@ietf.org>, "draft-ietf-teas-yang-te@ietf.org" <draft-ietf-teas-yang-te@ietf.org>
Thread-Topic: [Teas] WG Last Call: draft-ietf-teas-yang-te-29
Thread-Index: ATBiOTc40hBzor0AyuAb8o440099M8/V014AgDKcznyAXYNiJA==
X-MS-Exchange-MessageSentRepresentingType: 1
Date: Mon, 11 Jul 2022 15:17:08 +0000
Message-ID: <DS0PR19MB6501B9942732A1C5D53FEF77FC879@DS0PR19MB6501.namprd19.prod.outlook.com>
References: <409bd958-008c-76df-1692-221ab8dfbab0@labn.net> <06c501d84d22$e975ade0$bc6109a0$@olddog.co.uk> <DS0PR19MB6501B33D8C8A0AC822544E52FCCA9@DS0PR19MB6501.namprd19.prod.outlook.com>
In-Reply-To: <DS0PR19MB6501B33D8C8A0AC822544E52FCCA9@DS0PR19MB6501.namprd19.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-CA
X-MS-Has-Attach:
X-MS-Exchange-Organization-SCL: -1
X-MS-TNEF-Correlator:
X-MS-Exchange-Organization-RecordReviewCfmType: 0
Content-Type: multipart/alternative; boundary="_000_DS0PR19MB6501B9942732A1C5D53FEF77FC879DS0PR19MB6501namp_"
MIME-Version: 1.0
Archived-At: <https://mailarchive.ietf.org/arch/msg/teas/uTRS0ZUN9_6RWNgkY2W5EljUOgg>
Subject: Re: [Teas] WG Last Call: draft-ietf-teas-yang-te-29
X-BeenThere: teas@ietf.org
X-Mailman-Version: 2.1.39
Precedence: list
List-Id: Traffic Engineering Architecture and Signaling working group discussion list <teas.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/teas>, <mailto:teas-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/teas/>
List-Post: <mailto:teas@ietf.org>
List-Help: <mailto:teas-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/teas>, <mailto:teas-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 11 Jul 2022 15:17:27 -0000

Hi Adrian,

Thank you very much for thoroughly reviewing this draft and for providing all the comments.
The authors have tried to address some of your comments (some remain outstanding and pending closure).

We have published a new revision of the draft (rev -30) that includes the changes. The diff is at: https://www.ietf.org/rfcdiff?url2=draft-ietf-teas-yang-te-30.txt

Note, we intend to send a follow-up email to close on the outstanding points soon.

Please see inline below for responses to your comments ([TS] for closed comments, and [TS/OPEN-ISSUE] for the outstanding ones).

Regards,
Tarek (for the co-authors)

From: Teas <teas-bounces@ietf.org> on behalf of Adrian Farrel <adrian@olddog.co.uk>
Date: Sunday, April 10, 2022 at 5:35 PM
To: 'Lou Berger' <lberger@labn.net>, 'TEAS WG' <teas@ietf.org>
Cc: 'TEAS WG Chairs' <teas-chairs@ietf.org>
Subject: Re: [Teas] WG Last Call: draft-ietf-teas-yang-te-29
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

---
[TS]: Fixed


1.

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

[TS]: Fixed



---

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.

---
[TS]: Fixed



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.



[TS]: Fixed


---



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"



[TS]: Fixed


---



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


[TS]: Fixed


---

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.


[TS]: Fixed.


---

3.

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



[TS]: Fixed.


---

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.


[TS]: A TE system can reside on the device or off the device (TE controller). We intend to add this clarification in next revision:

NEW:
When the model is used to manage a specific device, the model
contains the TE Tunnels originating from the specific device.  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.

---

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.

[TS]: Figure 1 is correct as it shows the relationship of the different modules. Corrected the punctuation.


---

3.

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

[TS]: not necessarily. In YANG optional/mandatory can be specified for a specific leaf. A YANG feature can be negotiated between server and client to hide/show certain nodes of the data model.

---

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.


[TS]: Fixed

---

4.1

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

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


[TS]: Fixed


---

4.1

   defined in another document

Missing a citation

[TS]: reworded to avoid citation to work outside the scope of this document.

OLD:
For example, the MPLS-TE module "ietf-te-mpls.yang" is
defined in another document and augments the TE generic model as

NEW:
The TE data models for specific instances of signaling
protocols are outside the scope of this document and are defined in other documents.

---

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


[TS]: Fixed.


---

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.


[TS]: Fixed


---

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.

[TS]: Fixed/reworded to:
NEW:
   This includes creating, modifying and retrieving information about TE
   Tunnels, LSPs, and interfaces and their associated attributes (e.g.
   Administrative-Groups, SRLGs, etc.).

---

5.

   The detailed tree structure is provided in Figure 2.

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


[TS]: Fixed


---

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


[TS]: Fixed.

---

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


[TS]: Fixed.

---

5.1.1

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


[TS]: Fixed.

---

5.1.1

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


[TS]: Fixed.

---

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


[TS]: Fixed.


---

5.1.1

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


[TS]: Fixed.


---

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?


[TS]: We have introduced a new ‘co-routed’ boolean leaf to explicitly specify co-routedness. We have added a YANG when check to assert it is only set for bidirectional LSPs.

---

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.


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.

[TS]: added text to describe “TE Path” and elaborated on relationship of Tunnel -> Path -> LSP.

NEW:
     TE Path:

    An engineered path that once instantiated can be used to forward traffic
    from the TE unnel source termination point to the TE tunnel destination
    termination point.



---

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.


XX: Fixed.

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


[TS]: the alias is not a key in the tunnel list and need not be unique. The alias was introduced because there is a need to change the tunnel name after it has been instantiated (refer to https://github.com/tsaad-dev/te/issues/50#issuecomment-631940823)

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.


[TS]: yes, this color is used for services mapping to TE tunnel.


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?

[TS/OPEN-ISSUE]: do we need the G-PID?

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.


[TS]: the controller container holds information about the controller owner. This may be useful to show on other controller(s) in the network that are ‘non-owners’.


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

[TS/OPEN-ISSUE]: elaborate on implication of setting bidirectional in the document.


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?


[TS/OPEN-ISSUE]: the team has not converged on whether ‘override’ or ‘supplement’ or both options may be needed.



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



[TS]: Fixed.

---

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?

[TS]: unique within each path list.



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.

[TS]: multiple with same preference are allowed. The server is free to pick any of the equal paths and activate it.

---

5.1.2.1

   A secondary path contains attributes similar to a primary path.

How "similar"? What are the differences?

[TS/OPEN-ISSUE]: elaborate on 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/


[TS]: Fixed.

---

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".

[TS]: Accepted.


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


[TS]: fixed.

---

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/


[TS]: OK.

---

5.3

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

[TS]: Fixed.


---

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.

[TS/OPEN-ISSUE]: working with team on a way forward regarding this point.


---

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.

[TS/OPEN-ISSUE]: working with team on a way forward regarding this point.


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?

[TS]: Fixed.


---

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?

[TS]: primary-reverse-path is single path, so no preference arbitration is needed.


---

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?

[TS]: compute-only tunnels are treated similar to regular tunnels. So, yes a compute-service may optimize acting on relevant topology updates as the case for any other tunnel.

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

[TS]: yes, the leafs relevant to LSP instantiation are optional and will not be populated.


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

[TS]: Fixed.


---

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?

[TS]: If set to ‘false’, the user-supplied path is instantiated as-is without further validation or expansion by the computation engine. The onus is on the operator to provide a full valid path. If path expansion (even partial) is required, then use-path-computation ‘true’ is expected.


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

[TS]: rephrased as:

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


---

5.3

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

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


[TS]: Fixed.


---

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.



leaf path-scope {
  type identityref {
    base te-types:path-scope-type;
  }
  default "te-types:path-scope-end-to-end";
  config false;
  description
    "Indicates whether the path is a segment or portion of
      of the full path., or is the an end-to-end path for
      the TE Tunnel.";
}

[TS]: Updated the description above.


---

5.3

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

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.


[TS/OPEN-ISSUE]: working with team to address this.

---

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.


[TS/OPEN-ISSUE]: working with team to address this. We may be adding a base error identity that other path-setup technology modules can extend.

---

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


[TS]: Fixed.

---

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"


[TS]: Fixed.

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)


[TS]: Added reference to RFC7271 and RFC8234.
[TS/OPEN-ISSUE]: to track other suggestions.
---

5.3

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


[TS/OPEN-ISSUE]: working with team to address this.

---

5.3

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

[TS]: the approach we followed was to specify a default when one is specified in the RFC/standard. Otherwise, we leave it as an implementation choice.

---

5.3

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


[TS]: for tunnel termination point, we tried to be consistent with rfc8795.

---

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?


[TS]: this was reworked and suggestion was incorporated. Updated below:

leaf protection-group-ingress-node {
  type boolean;
  default "true";
  description
    "When 'true', indicates that the action is
     applied on ingress node.
     By default, the action applies to the ingress node
     only.";
}
leaf protection-group-egress-node {
  type boolean;
  default "false";
  description
    "When set to 'true', indicates that the action is
     applied on egress node.
     By default, the action applies to the ingress node
     only.";
}

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.

container lsp-record-route-information {
¦ description
¦   "RSVP recorded route object information.";
¦ list lsp-record-route-information {


[TS/OPEN-ISSUE]: working with team to address this.


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";
       }


[TS]: reference to RSVP specific language SESSION/SENDER_TEMPLATE was removed.

---

6.1.1

s/interface attributes/Interface attributes/


[TS]: Fixed.


---

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'

[TS]: Fixed.

---

6.3

Copyright date

[TS]: Fixed.

---

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.
XX: move to base.

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?


[TS]: updated description of timers.

---

Section 8 appears to be empty!


[TS]: Removed.

---

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


[TS/OPEN-ISSUE]: working with team to address this.

---

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.

[TS]: Fixed.




-----Original Message-----
From: Teas <teas-bounces@ietf.org> On Behalf Of Lou Berger
Sent: 21 March 2022 13:28
To: TEAS WG <teas@ietf.org>
Cc: TEAS WG Chairs <teas-chairs@ietf.org>
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
Teas@ietf.org
https://www.ietf.org/mailman/listinfo/teas

_______________________________________________
Teas mailing list
Teas@ietf.org
https://www.ietf.org/mailman/listinfo/teas