Skip to main content

YANG Data Model for Routing in Fat Trees (RIFT)
draft-ietf-rift-yang-17

Note: This ballot was opened for revision 15 and is now closed.

Jim Guichard
Yes
Deb Cooley
No Objection
Comment (2024-08-04 for -16) Not sent
I have no comments.

Thank you to Daniel Migault for the secdir review.
Erik Kline
No Objection
Gunter Van de Velde
No Objection
Comment (2024-08-01 for -16) Sent for earlier
# Gunter Van de Velde, RTG AD, comments for draft-ietf-rift-yang-16

Changed on 2Aug2024 from No Record to No Objection

Original AD concern:

When going through the document most of the items are included and realize that a great deal of work and detail went into the proposed model.
I do have one observation that confuses me and that is the relationship with the RIFT Thrift model used. 

The RIFT specification has a Thrift data model embedded which is documented in section 7:
https://datatracker.ietf.org/doc/html/draft-ietf-rift-rift-24#section-7

It is unclear to me if the proposed YANG model aligns or has deviations of this Thrift model. Is the use case for both different?
Would it make sense to consider a table to align both models where appropriate? Have both models similar intent?

Answer from Bruno Rijsman:

* The use case and the intent for the Thrift model and the YANG model are quite different.
* The Thrift model describes the control-plane protocol, i.e. encoding of control-plane messages between routers.
* The YANG model describes the management-plane, i.e. the attributes that need to be configured / monitored by the network management system.
* They are already aligned in the sense that the YANG data model already describes what needs to be managed about the Thrift-described control plane protocol.
Mahesh Jethanandani
No Objection
Comment (2024-08-04 for -16) Sent
Thanks to Michal Vasko for his YANG doctor's review.

Thanks to Jordon Head for the shepherd writeup. The writeup was done in 2022 though. I wonder if anything has changed in the interim. I will note that the document is a protocol document, and as such could do with an implementation.

Section 1.1, paragraph 4
>    The terminology for describing YANG data models is found in [RFC6020]
>    and [RFC7950], including:
>
>    *  augment
>
>    *  container
>
>    *  choice

This is a draft that defines YANG module. It is assumed that anyone who is reviewing a YANG model is familiar with these terms. It is therefore redundant to repeat them here. I would just remove them. 

Section 2.1, paragraph 3
>    The RIFT YANG module augments the /routing/control-plane-protocols/
>    control-plane-protocol path defined in the ietf-routing module.  The
>    ietf-rift model defines a single instance of RIFT.  Multiple
>    instances are instantiated as multiple control-plane protocols
>    instances.

The last statement does not make sense. I think you mean to say "Multiple instances of the protocol are supported by the module by giving them unique names (as key)".  As a note, the routing module supports several control plane protocols, not several instances of them. This model augments the routing module to add RIFT as a control plane protocol. It then offers the ability to create a list of instances, which it does by declaring 'list rift'.

Section 2.2, paragraph 1
>    This model imports and augments ietf-routing YANG model defined in
>    [RFC8349].  Both configuration branch and state branch of [RFC8349]
>    are augmented.  The configuration branch covers node base and policy
>    configuration.  The container "rift" is the top level container in
>    this data model.  The container is expected to enable RIFT protocol
>    functionality.

RFC8349 does not have two separate configuration and state branches. Not if you were following the NMDA module, which is noted below in the document. Suggest removing the second and third sentences in the paragraph.

Section 2.3, paragraph 2
>    The RIFT YANG module augments the /routing/control-plane-protocols/
>    control-plane-protocol path defined in the ietf-routing module.  The
>    ietf-rift model defines a single instance of RIFT.  Multiple
>    instances are instantiated as multiple control-plane protocols
>    instances.

Same comment as above. Suggest rewording the last sentence to say, "Multiple instances of the protocol are supported by the module by giving each instance a unique name".

Section 2.3, paragraph 2
>    module: ietf-rift

Instead of displaying the entire tree diagram, it would help to break it into chunks and explain each section of the module as part of the overview of the model. For example, what is the 'database' section of the module (I think Eric had the same question)? Does each notification need to carry all the data defined under it, or could it do with a smaller set of data, and the rest requested using a <get> when more information is desired?

Section 2.3, paragraph 1
>           +--ro spf-statistics
>           |  +--ro spf-statistics* [spf-direction-type]
>           |     +--ro spf-direction-type    enumeration
>           |     +--ro start-time?           yang:date-and-time
>           |     +--ro end-time?             yang:date-and-time
>           |     +--ro triggering-tie
>           |        +--ro tie-direction-type?     enumeration
>           |        +--ro originator?             system-id
>           |        +--ro tie-type?               enumeration
>           |        +--ro tie-number?             uint32
>           |        +--ro seq?                    uint64
>           |        +--ro size?                   uint32
>           |        +--ro origination-time?       ieee802-1as-timestamp
>           |        +--ro origination-lifetime?   uint32
>           |        +--ro remaining-lifetime?     uint32


I would suggest that all statistics be combined in a single container called 'statistics'. Currently, they are scattered all over the module. That will enable a user to specify the container to get all stats, and it enables clearing stats when they are clubbed together in one container. Which remind me. There should be action to clear stats within a model. Finally, instead of using uint32, consider using zero-based-counter32.

Section 3, paragraph 22
>         Copyright (c) 2022 IETF Trust and the persons identified
>         as authors of the code.  All rights reserved.


The Copyright year needs to be updated.

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

Found IP blocks or addresses not inside RFC5737/RFC3849 example ranges:
"224.0.0.121" and "ff02::a1f7".

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

"Authors' Addresses", paragraph 4

Section 1.1, paragraph 16
>    LIE: "Link Information Element" are exchanged on all the system's
>    links running RIFT to form _ThreeWay_ adjacencies and carry
>    information used to perform Zero Touch Provisioning (ZTP) of levels.

Why the _ before and after ThreeWay here and the other definitions? Is this different from ThreeWay Adjacency?

Section 2.1, paragraph 1
>    The model covers RIFT [I-D.ietf-rift-rift].

Seems like a redundant statement, as the next paragraph explains what the model is about.

Section 3, paragraph 98
>  } config false; description "The type of a link."; } description "The type o
>                                   ^^^^^^^^^
If "type" is a classification term, "a" is not necessary. Use "type of". (The
phrases "kind of" and "sort of" are informal if they mean "to some extent".).

Section 3, paragraph 98
> he type of a link."; } description "The type of a link."; Zhang, et al. Expir
>                                         ^^^^^^^^^
If "type" is a classification term, "a" is not necessary. Use "type of". (The
phrases "kind of" and "sort of" are informal if they mean "to some extent".).

Section 3, paragraph 105
> false; description "The direction type of a TIE."; } description "The directi
>                                   ^^^^^^^^^
If "type" is a classification term, "a" is not necessary. Use "type of". (The
phrases "kind of" and "sort of" are informal if they mean "to some extent".).

Section 3, paragraph 105
> E."; } description "The direction type of a TIE."; } // tie-direction-type g
>                                   ^^^^^^^^^
If "type" is a classification term, "a" is not necessary. Use "type of". (The
phrases "kind of" and "sort of" are informal if they mean "to some extent".).

Section 3, paragraph 106
>  2024 description "The direction type of a SPF calculation."; } description "
>                                  ^^^^^^^^^
If "type" is a classification term, "a" is not necessary. Use "type of". (The
phrases "kind of" and "sort of" are informal if they mean "to some extent".).

Section 3, paragraph 106
> n."; } description "The direction type of a SPF calculation."; } // spf-direc
>                                   ^^^^^^^^^
If "type" is a classification term, "a" is not necessary. Use "type of". (The
phrases "kind of" and "sort of" are informal if they mean "to some extent".).

Section 3, paragraph 140
> fault link capabilities. It can be overwrite by the configuration underneath
>                                 ^^^^^^^^^^^^
There may an error in the verb form "be overwrite".

Section 3, paragraph 155
>  boolean; description "If this link allow horizontal link adjacency."; } con
>                                     ^^^^^
"link" is a singular noun. It appears that the verb form is incorrect.
Murray Kucherawy
No Objection
Comment (2024-08-07 for -16) Sent
The shepherd writeup says this document is seeking Internet Standard status, but the document itself and the datatracker metadata put it at Proposed Standard.  I presume the latter is correct.

It also says this isn't a protocol document, but why is it seeking Proposed Standard status if that's the case?  Are YANG documents normally something else?

Section 1.1 defines "ThreeWay" but everywhere else in the document it's "Three Way".
Orie Steele
No Objection
Comment (2024-08-04 for -16) Sent
# Orie Steele, ART AD, comments for draft-ietf-rift-yang-16 
CC @OR13

https://author-tools.ietf.org/api/idnits?url=https://www.ietf.org/archive/id/draft-ietf-rift-yang-16.txt&submitcheck=True

## Comments

### 2.4.  RIFT configuration

```
647	   Some features can be used to enhance protocol, such as BFD [RFC5881],
648	   flooding-reducing, community attribute.
```

The language in this section might be improved, and citations could be provided for "flooding-reducing", "community attribute".

## Nits

### Possibly normative should

```
657	   Unexpected TIE and neighbor's layer error should be notified.
```

```
1717	              This should be prohibited less than 2*purge-lifetime.";
```

I do not have enough context to understand if these are clear enough, or if they ought to be a normative SHOULDs.
Roman Danyliw
No Objection
Comment (2024-07-29 for -16) Not sent
Thank you to Linda Dunbar for the GENART review.

I have no additional feedback from the perspective of the GEN area.
Warren Kumari
No Objection
Comment (2024-08-06 for -16) Sent
Much thanks to Tim Chown for the OpsDir review (https://datatracker.ietf.org/doc/review-ietf-rift-yang-15-opsdir-lc-chown-2024-07-21/) -- I encourage the authors to address his comments.
Zaheduzzaman Sarker
No Objection
Comment (2024-08-07 for -16) Not sent
Thanks for working on this specification. I have no comments from transport protocol points of view.
Éric Vyncke
No Objection
Comment (2024-07-08 for -15) Sent
# Éric Vyncke, INT AD, comments for draft-ietf-rift-yang-15

Thank you for the work put into this document.

Please find below some non-blocking COMMENT points (but replies would be appreciated even if only for my own education), and some nits.

Special thanks to Jordan Head for the shepherd's concise write-up including the WG consensus and the justification of the intended status.

I hope that this review helps to improve the document,

Regards,

-éric



# COMMENTS (non-blocking)

## Lack of 'basic' operating values

I was expecting to see some leaves about basic counters, i.e., number of RIFT packets sent and received per interface. The only counter in the YANG model appears to be "number-of-flaps", it this counter enough for operation ?

## Abstract

Suggest adding a reference to the rift draft (with a RFC editor note requesting to update it with the rift RFC when known). Or use "XXXX" per the section 5 note.

## Section 2.1

`The model contains all the basic configuration parameters to operate the protocol` unsure whether configuration alone is enough to operate a protocol... Suggest adding "status values" to "configuration parameters" and as a protocol cannot really be operated, use "to operate a RIFT network".

## Section 2.5

The title "state" seems to relate to the "database" YANG node. If I am reading it correctly, then please update the section title and text.

## Section 3

In several descriptions there is a reference to a section but it is (somehow) unclear in which document (as it is specified in the reference); suggest doing like "feature tie-security" for all features (i.e., section in the reference).

The default MTU in `leaf mtu-size` is 1400, which is rather unusual... It is often 1500 or 1280 (for IPv6 minimum link MTU). Is there any reason for this 1400 value ? If so, some explanations or reference will be welcome.


# NITS (non-blocking / cosmetic)

## PoD or POD

Sometimes "PoD" is used and other times "POD" is used. Suggest being consistent.

## Section 1.1

Sometimes `This is an acronym for` is used, and other times the acronym is directly defined.

## Section 3

Please use lowercase only in `default "FF02::A1F7";` 

Suggest renaming  the `leaf ttl` into "leaf hop-limit"