Mahesh Jethanandani's Discuss on draft-ietf-teas-yang-te-41

[italobusi]

Mahesh Jethanandani has entered the following ballot position for
draft-ietf-teas-yang-te-41: Discuss

When responding, please keep the subject line intact and reply to all email addresses included in the To and CC lines. (Feel free to cut this introductory paragraph, however.)

Please refer to https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/
for more information about how to handle DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-teas-yang-te/

---

DISCUSS:

Section 3, paragraph 8

• Where TE functions or features might be optional within the
  deployed TE network, the model declares them as optional.

How does the module declare some of the features to be optional?

Section 5.3, paragraph 23

         leaf name {
           type tunnel-ref;
           description
             "Dependency tunnel name. The tunnel may not have been
              instantiated yet.";

You have a list whose key is the name. But you note here that the tunnel may
not have been instantiated yet. How would you add an entry to a list if its key
is not available?

Section 5.3, paragraph 24

   leaf enable {
     type boolean;
     description
       "Enables the TE component features.";

This can be achieved by using a presence container. See section 7.5.1 of RFC
7950. Why use a boolean?

Section 6.3, paragraph 6

 /* RFC Editor Note: Please replace the above line with the URN
  * assigned by IANA and remove this note.
  */

Why is this an action for the RFC Editor or IANA? In general, if the URN does
not have a conflict, it will be assigned, otherwise the authors will be asked
to come up with a new URN.

---

COMMENT:

"Abstract", paragraph 1

This model covers data for configuration, operational state, and remote
procedural calls, and event notifications.

Thanks to Gyan Mishra for this OPSDIR review. In general, I agree with Med on
most of the issues he has highligted in his review.

Section 1, paragraph 0

YANG [RFC6020] and [RFC7950] is a data modeling language that was
introduced to define the contents of a conceptual data store that
allows networked devices to be managed using NETCONF [RFC6241]. YANG
has proved relevant beyond its initial confines, as bindings to other
interfaces (e.g. RESTCONF [RFC8040]) and encoding other than XML
(e.g. JSON) are being defined. Furthermore, YANG data models can be
used as the basis of implementation for other interfaces, such as CLI
and programmatic APIs.

Agree with Med that this paragraph can be removed.

Section 2.2, paragraph 1

+============+===============+======================================+
| Prefix | YANG module | Reference |
+============+===============+======================================+
| yang | ietf-yang- | [RFC6991] |
| | types | |
+------------+---------------+--------------------------------------+
| inet | ietf-inet- | [RFC6991] |
| | types | |
+------------+---------------+--------------------------------------+
| rt- | ietf-routing- | [RFC8294] |
| types | types | |
+------------+---------------+--------------------------------------+
| te- | ietf-te-types | [I-D.draft-ietf-teas-rfc8776-update] |
| types | | |
+------------+---------------+--------------------------------------+
| te- | ietf-te- | [I-D.draft-ietf-teas-rfc8776-update] |
| packet- | packet-types | |
| types | | |
+------------+---------------+--------------------------------------+
| te | ietf-te | this document |
+------------+---------------+--------------------------------------+
| te-dev | ietf-te- | this document |
| | device | |
+------------+---------------+--------------------------------------+

Please update the reference of RFC6991 to point to RFC9911.

Section 5.1.1, paragraph 2

    +--rw globals
    |  +--rw named-admin-groups
    |  |  +--rw named-admin-group* [name]
    |  |        ...
    |  +--rw named-srlgs
    |  |  +--rw named-srlg* [name]
    |  |        ...
    |  +--rw named-path-constraints
    |     +--rw named-path-constraint* [name]

       Figure 3: TE globals YANG subtree high-level structure

I support Med's comment on not repeating the parent prefix in children's names,
with one exception. If the parent is a container, and the child is a list, then
I prefer that it has the same name, with the container being plural and the
list being singular. Maybe shorten the name by dropping named-??

Section 5.3, paragraph 2

• ietf-yang-types and ietf-inet-types defined in [RFC6991]

Please update the reference to RFC9911.

Section 5.3, paragraph 11

   "WG Web:   <https://tools.ietf.org/wg/teas/>

Please update the link to reference the datatracker website.

Section 5.3, paragraph 16

   "YANG data module for TE configuration, state, and RPCs.
    The model fully conforms to the Network Management
    Datastore Architecture (NMDA).

The second sentence is unnecessary.

Section 5.3, paragraph 20

     "RFCXXXX: A YANG Data Model for Traffic Engineering Tunnels
      and Interfaces.";

Please update the title of the draft.

Section 5.3, paragraph 24

 /* This grouping is re-used in path-computation YANG model
  * defined in [I-D.ietf-teas-yang-path-computation] */

Why is this statement important to make here? Please remove.

Section 5.3, paragraph 23

     description
       "Indicates whether the reverse path must be co-routed
        with the primary.";

The description is not clear on when the reverse path must be co-routed. When
set to true or false? Better to say, "When set to true, the reverse path ..."

Section 5.3, paragraph 23

 /* This grouping is re-used in path-computation YANG model
  * defined in [I-D.ietf-teas-yang-path-computation] */

Please remove.

Section 5.3, paragraph 24

           leaf disjointness-type {
             type te-types:te-path-disjointness;
             config false;

Isn't your container already config false? Why repeat it here?

Section 5.3, paragraph 23

           leaf last-computed-timestamp {
             type yang:date-and-time;
             config false;

And here.

Section 5.3, paragraph 23

       reference
         "RFC4427";

Add the title of the draft here and everywhere only the RFC number is cited.

Section 5.3, paragraph 22

       description
         "Disable restoration reversion to working path.";

Restate the description starting with, "When set to true ...".

Section 5.3, paragraph 23

     type binary;

Curious on the choice of the type being binary. Why binary and not a uint32?

Section 5.3, paragraph 23

     description
       "Indicates a bidirectional tunnel";

Restate the description starting with, "When set to true ...".

Section 5.3, paragraph 22

         description
           "Enables the hierarchical link properties supported by
            this tunnel";

Restate the description starting with, "When set to true ...".

Section 6.3, paragraph 8

     "RFCXXXX: A YANG Data Model for Traffic Engineering
      Tunnels and Interfaces";

Update the title, please.

Section 6.3, paragraph 10

   "WG Web:   <https://tools.ietf.org/wg/teas/>

Update to point to the datatracker website.

Section 6.3, paragraph 15

 description
   "This module defines a data model for TE device configurations,
    state, and RPCs. The model fully conforms to the
    Network Management Datastore Architecture (NMDA).

Remove the second sentence, as it is not necessary.

Section 6.3, paragraph 19

     "RFCXXXX: A YANG Data Model for Traffic Engineering Tunnels
      and Interfaces";

Update the title, please.

Section 8, paragraph 4

  Name:       ietf-te
  Namespace:  urn:ietf:params:xml:ns:yang:ietf-te
  Prefix:     te
  Reference:  RFCXXXX

Please add another line to this registration request that says:
Maintained by IANA: N

Section 9, paragraph 0

The YANG module specified in this document defines a schema for data
that is designed to be accessed via network management protocols such
as NETCONF [RFC6241] or RESTCONF [RFC8040]. The lowest NETCONF layer
is the secure transport layer, and the mandatory-to-implement secure
transport is Secure Shell (SSH) [RFC6242]. The lowest RESTCONF layer
is HTTPS, and the mandatory-to-implement secure transport is TLS
[RFC8446].

Please update this section to match the template defined in rfc8407bis.

No reference entries found for these items, which were mentioned in the text:
[draft-ietf-teas-rfc8776-update],
[I-D.ietf-teas-yang-path-computation],
and [RFC8231].

Possible DOWNREF from this Standards Track doc to [ITU_G.808.1]. If so, the
IESG needs to approve it.

Found IP block or address not inside RFC5737/RFC3849 example ranges: "0.0.0.0".

---

NIT

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

Section 5.1.1, paragraph 2

• I support Med's comment on not repeating parent prefix in children's names,
  with one exception. If the parent is a container, and the child is a list, then
  I prefer that it has the same name, with the container being plural and the
  list being singular. Maybe shorten the name by dropping named-??

Uncited references: [rfc3473].

Reference [RFC6991] to RFC6991, which was obsoleted by RFC9911 (this may be on
purpose).

These URLs point to tools.ietf.org, which has been taken out of service:

• https://tools.ietf.org/wg/teas/

Section 5.1.1, paragraph 2

YANG container that holds the technology agnostic TE bandwidth constraint. -
^^^^^^^^^^^^^^^^^^^
This word is normally spelled with a hyphen.

Section 5.1.3, paragraph 1

e link to reference the datatracker web site. WG List: mailto:teas@ietf.org
^^^^^^^^
Nowadays, it's more common to write this as one word.

Section 5.3, paragraph 28

-off-time { type uint32; units "milli-seconds"; description "The time between
^^^^^^^^^^^^^
This word is normally spelled as one.

Section 5.3, paragraph 28

-off-time { type uint32; units "milli-seconds"; description "The time between
^^^^^^^^^^^^^
This word is normally spelled as one.

Section 5.3, paragraph 34

type being binary. Why binary and not a uint32? description "The TE tunnel e
^
Use "an" instead of "a" if the following word starts with a vowel sound, e.g.
"an article", "an hour".

Section 5.3, paragraph 39

empty, any secondary path may be utilised when the current primary path is
^^^^^^^^
Do not mix variants of the same word ("utilise" and "utilize") within a single
text.

Section 5.3, paragraph 39

to the secondary path that may be utilised when the containing primary path
^^^^^^^^
Do not mix variants of the same word ("utilise" and "utilize") within a single
text.

Section 5.3, paragraph 44

sed to ensure global uniqueness of the the tunnel identifier by carrying an I
^^^^^^^
Possible typo: you repeated a word.

Section 5.3, paragraph 52

Update to point to the datatracker web site. WG List: mailto:teas@ietf.org
^^^^^^^^
Nowadays, it's more common to write this as one word.

Section 6.1.2, paragraph 6

ike to thank the members of the multi-vendor YANG design team who are involv
^^^^^^^^^^^^
This word is normally spelled as one.

Section 6.3, paragraph 22

l Path Constraint In this example, the a per-tunnel path constraint is expli
^^^^^
Two determiners in a row. Choose either "the" or "a".

See: https://mailarchive.ietf.org/arch/msg/teas/_TnStv6YaPY1htnVRgCdZKz9s64/