Subject: Re: [Rift] Dear RIFT protocol and RIFT YANG co-author, plsreviewtheupdateofRIFT YANG model

From: Bruno Rijsman <[email-masked]>
Date: Fri, 10 Jul 2020 12:36:44 +0200

Hi Sandy,

Thanks for the updated model; the next round of review comments will follow shortly.

Meanwhile, here are the answers to your questions:
> And I have two questions:
> 
> There is a leaf "cost" in the neighbor grouping, is the "metric" duplicated with the "cost"?
> 
Currently, your YANG datamodel has both cost and metric attributes per neighbor (as well as bandwidth):

+--ro neighbor
      +--ro cost?      uint32
      +--ro metric?    uint32
      +--ro bandwidth? uint32

Most of the text in the RIFT spec (including the definition) uses the term "cost" to mean the cost of a link to an immediately neighboring node. The term "distance" is used to mean the total cost of a path to a remote node.

The Thrift datamodel in the RIFT spec is super inconsistent in its terminology:

 /** neighbor of a node */
 struct NodeNeighborsTIEElement {
 /** level of neighbor */
 1: required common.LevelType level;
 /** Cost to neighbor.
 @note: All parallel links to same node
 incur same cost, in case the neighbor has multiple
 parallel links at different cost, the largest distance
 (highest numerical value) MUST be advertised.
 @note: any neighbor with cost <= 0 MUST be ignored
 in computations */
 3: optional common.MetricType cost
 = common.default_distance;

Note that the Thrift attribute is called "cost" but it's type is "MetricType" and it 's default value is "default_distance" 8-)

Bottom line: cost and metric are synonyms, we only need neighbor cost OR neighbor metric in the YANG (not both). I suggest that we only keep cost and get rid of metric. We do need bandwidth as a separate attribute.

Note: in a later e-mail I will send a rather long list of attributes that could be added to the YANG data model. bandwidth-adjusted-cost will be one of those additional attributes that I will suggest (see section 4.3.6. Fabric Bandwidth Balancing in the RIFT spec).

> If different source addresses can be used for LIE and TIE sending?
> 

One thing to consider is that LIEs are always sent on IPv4 *and* IPv6.  Flooding packets (not just TIEs, but also TIDEs and TIREs) are sent on IPv4 *or* IPv6 but not both.

For a given address family, I think that it would be reasonable to require that LIEs and flooding traffic *must* be sent on the same source address. The reason is that once a neighbor has been discovered by virtue of receiving LIEs from a particular address, we want to open up a listening socket for that particular neighbor for flooding traffic. That flooding socket has the same remote address but potentially a different UDP port.

My implementation does not actually bind to a remote address, but I *could* have chosen to do so:

agg_101> show interface if_101_1 sockets
+----------+-----------+--------+------------------------------+------------+-----------------+-------------+
| Traffic  | Direction | Family | Local Address                | Local Port | Remote Address  | Remote Port |
+----------+-----------+--------+------------------------------+------------+-----------------+-------------+
| LIEs     | Receive   | IPv4   | 224.0.0.81                   | 20001      | Any             | Any         |
+----------+-----------+--------+------------------------------+------------+-----------------+-------------+
| LIEs     | Receive   | IPv6   | ff02::a1f7%en0               | 20001      | Any             | Any         |
+----------+-----------+--------+------------------------------+------------+-----------------+-------------+
| LIEs     | Send      | IPv4   | 192.168.178.192              | 63729      | 224.0.0.71      | 20002       |
+----------+-----------+--------+------------------------------+------------+-----------------+-------------+
| LIEs     | Send      | IPv6   | fe80::4b0:a72c:c8eb:dd5a%en0 | 63730      | ff02::a1f7%en0  | 20002       |
+----------+-----------+--------+------------------------------+------------+-----------------+-------------+
| Flooding | Receive   | IPv4   | 192.168.178.192              | 20004      | Any             | Any         |
+----------+-----------+--------+------------------------------+------------+-----------------+-------------+
| Flooding | Receive   | IPv6   | fe80::4b0:a72c:c8eb:dd5a%en0 | 20004      | Any             | Any         |
+----------+-----------+--------+------------------------------+------------+-----------------+-------------+
| Flooding | Send      | IPv4   | 192.168.178.192              | 54629      | 192.168.178.192 | 20003       |
+----------+-----------+--------+------------------------------+------------+-----------------+-------------+

---

From: Bruno Rijsman <[email-masked]>
Date: Fri, 10 Jul 2020 12:40:21 +0200

I was just looking at the YANG data model, and I noticed that it does *not* allow different source addresses to be specified, so your question was sort of moot.

What it *does* allow is different RX addresses to be specified for TIEs and flooding traffic.  These are receive multicast addresses, so they will appear as the destination address on the packet.  They are not source addresses.

— Bruno

> On Jul 10, 2020, at 12:36 PM, Bruno Rijsman <[email-masked]> wrote:
> 
> Hi Sandy,
>

---

From: Bruno Rijsman <[email-masked]>
Date: Fri, 10 Jul 2020 12:53:43 +0200

One more remark on this topic (hopefully the last one).

Your YANG model currently has a metric attribute on the interface, and both a cost and metric attribute on the neighbor:

  +--rw interface* [name]
  |  +--rw metric?          uint32
  |  +--ro neighbor
  |     +--ro cost?         uint32
  |     +--ro metric?       uint32

I suggest that we change it to the following

  +--rw interface* [name]
  |  +--rw cost?            uint32
  |  +--ro neighbor
  |     +--ro cost?         uint32

Note that the interface cost is the cost from us to the neighbor, where as the neighbor cost is the cost from the neighbor to us (i.e. the cost that the neighbor advertised)

— Bruno

> On Jul 10, 2020, at 12:40 PM, Bruno Rijsman <[email-masked]> wrote:
> 
>> On Jul 10, 2020, at 12:36 PM, Bruno Rijsman <[email-masked] <mailto:[email-masked]>> wrote:
>> 
>> Hi Sandy,
>>

---

From: Bruno Rijsman <[email-masked]>
Date: Fri, 10 Jul 2020 13:33:52 +0200

Hi Sandy,

Here is the next review iteration (for rift-yang-tree200706).

Once again, there is more reviewing to be done, but I am talking it step by step in bite-sized chunks.

— Bruno

module: ietf-rift
  augment /rt:routing/rt:control-plane-protocols/rt:control-plane-protocol:
    +--rw rift!
       +--rw node
          +--rw name?                        string

Node needs to be a list (indexed by name); in the case of multi-instance RIFT there can be more than one RIFT node per RIFT engine. That also implies that name is the key and hence mandatory. This brings us to:

+--rw node* [name]
   +--rw name   string

          +--ro level?                       level
          +--rw system-id                    system-id
          +--rw pod?                         uint32
          +--rw configured-level?            level
          +--rw overload?                    boolean
          +--ro protocol-minor-version       uint16
          +--ro hierarchy-indications?       enumeration
          +--rw flood-reduction?             boolean
          +--rw nonce-increasing-interval?   uint16
          +--rw maximum-nonce-delta?         uint8 {nonce-delta-adjust}?
          +--rw rx-lie-multicast-address
          |  +--rw ipv4?   inet:ipv4-address
          |  +--rw ipv6?   inet:ipv6-address
          +--rw tx-lie-multicast-address
          |  +--rw ipv4?   inet:ipv4-address
          |  +--rw ipv6?   inet:ipv6-address
          +--rw lie-tx-port?                 inet:port-number
          +--rw global-link-capabilities
          |  +--rw bfd?                     boolean
          |  +--rw v4-forwarding-capable?   boolean
          +--rw rx-flood-port?               inet:port-number
          +--rw holdtime?                    rt-types:timer-value-seconds16
          +--rw tide-generation-interval?    rt-types:timer-value-seconds16
          +--rw tie-security-key-id?         uint32
          +--rw interface* [name]
          |  +--ro local-link-id?                  linkid-type

We should be consistent. Whenever an attribute foo is both advertised to a neighbor vs receive from a neighbor, either do (a) or (b) or (c) or (d) below. The current YANG model does a mixture of all of these:
(a) Call it foo under interface and also foo (same name) under neighbor (e.g. interface cost vs neighbor cost). The context (interface vs neighbor) determines whether it is local/sent/advertised vs remote/received.
(b) Call it advertised-foo under interface and received-foo under neighbor (e.g. advertised-lie-tie-addresses vs received-lie-tie-addresses)
(c) Call it local-foo under interface and remote-foo under neighbor (e.g. local-link-id vs remote-link-id).
(d) Nest foo in advertised-in-lies for the interface, and nest foo in received-in-lies for the neighbor (e.g. interface advertised-in-lies you-are-flood-repeater vs neighbor received-in-lies you-are-flood-repeater)

I prefer (a) but I could live with (b) or (c) or (d) as well, as long as we are consistent. For example, if we pick (a) you would remove local- from local-link-id.

          |  +--rw name                            if:interface-ref
          |  +--rw metric?                         uint32

Rename metric to cost, as discussed in separate e-mail.

          |  +--rw advertised-lie-tie-addresses
          |  |  +--ro rx-lie-source-address
          |  |  |  +--ro ipv4?   inet:ipv4-address
          |  |  |  +--ro ipv6?   inet:ipv6-address
          |  |  +--ro flooding-source-address
          |  |     +--ro ipv4?   inet:ipv4-address
          |  |     +--ro ipv6?   inet:ipv6-address
          |  +--ro direction-type?                 enumeration
          |  +--ro was-the-last-lie-accepted?      boolean

I suggest adding an optional last-lie-reject-reason (string) that explains why the LIE was rejected if was-the-last-lie-accepted is false.

          |  +--rw advertised-in-lies

YANG question: does it make sense to have a read-write container with only read-only attributes?

          |  |  +--ro you-are-flood-repeater?        boolean
          |  |  +--ro not-a-ztp-offer?               boolean
          |  |  +--ro you-are-sending-too-quickly?   boolean
          |  +--rw link-capabilities
          |  |  +--rw bfd?                     boolean
          |  |  +--rw v4-forwarding-capable?   boolean
          |  +--rw state?                          enumeration

State should be read-only and mandatory. The adjacency (and hence the interface) always has an operational (not a configured) state: one-way, two-way, three-way, or multiple-neighbors.

          |  +--ro neighbor

Neighbor should be optional. In state ONE_WAY there is no neighbor.

          |     +--ro name?                         string
          |     +--ro level?                        level
          |     +--ro system-id                     system-id
          |     +--ro pod?                          uint32
          |     +--ro protocol-version?             uint16
          |     +--ro address-families
          |     |  +--ro address-family* [address-family]
          |     |     +--ro address-family    iana-rt-types:address-family
          |     +--ro received-lie-tie-addresses
          |     |  +--ro rx-lie-source-address
          |     |  |  +--ro ipv4?   inet:ipv4-address
          |     |  |  +--ro ipv6?   inet:ipv6-address
          |     |  +--ro flooding-source-address
          |     |     +--ro ipv4?   inet:ipv4-address
          |     |     +--ro ipv6?   inet:ipv6-address
          |     +--ro remote-id?                    uint32
          |     +--ro cost?                         uint32
          |     +--ro metric?                       uint32
          |     +--ro bandwidth?                    uint32
          |     +--ro flood-reduction?              boolean
          |     +--ro received-link-capabilities
          |     |  +--ro bfd?                     boolean
          |     |  +--ro v4-forwarding-capable?   boolean
          |     +--ro received-from-lies

If we end up picking (d) in the comment near the top, rename received-from-lies => received-in-lies

          |     |  +--ro you-are-flood-repeater?        boolean
          |     |  +--ro not-a-ztp-offer?               boolean
          |     |  +--ro you-are-sending-too-quickly?   boolean
          |     +--ro tx-flood-port?                inet:port-number
          |     +--ro bfd-up?                       boolean
          |     +--ro outer-security-key-id?        uint8
          +--ro miscabled-links*             linkid-type
          +--rw (algorithm-type)?
          |  +--:(spf)
          |  +--:(all-path)

I don't know this :(foo) syntax. What does it mean?

          +--ro hal?                         level
          +--ro vol-list
          |  +--ro vol* [system-id]
          |     +--ro offered-level?   level
          |     +--ro name?            string
          |     +--ro level?           level
          |     +--ro system-id        system-id
          |     +--ro pod?             uint32

There is one sent offer per neighbor, and one received offer per neighbor (see example output of RIFT-Python "show node" below)

At this point in the YANG model we are already in the neighbor container. This implies that 
(a) We only need to keep track of one single sent offer and one received offer (not a list of offers indexed by system-id).
(b) We already know the system-id of the neighbor (it is an attribute of the neighbor container)
(c) For both sent and received offers, we need to keep track of the level and not-a-ztp-offer
(d) FOr the received offer, we additionally need to keep track of whether it is the best offer (in the Python implementation I keep track of some additional information for easier debugging, e.g. whether or not it was removed from consideration and if so why, once again see output below).

All of this leads to the following possible YANG model:

          +--rw interface* [name]
          |  +--ro local-link-id?                    linkid-type
          |  +--rw name                              if:interface-ref
          |  +--ro neighbor
          |     +--ro name?                           string
          |     +--ro sent-offer
          |     |  +--ro level                        level
          |     |  +--ro not-a-ztp-offer              boolean
          |     +--ro received-offer
          |     |  +--ro level                        level
          |     |  +--ro not-a-ztp-offer              boolean
          |     |  +--ro best                         boolean
          |     |  +--ro removed-from-consideration   boolean
          |     |  +--ro removal-reason?              string

agg_101> show node
Node:
+---------------------------------------+------------------+
| Name                                  | agg_101          |
[...]
+---------------------------------------+------------------+

Received Offers:
+-------------+-----------+-------+-----------------+-----------+-------+------------+---------+----------------+
| Interface   | System ID | Level | Not A ZTP Offer | State     | Best  | Best 3-Way | Removed | Removed Reason |
+-------------+-----------+-------+-----------------+-----------+-------+------------+---------+----------------+
| if_101_1    | 1         | 24    | False           | THREE_WAY | True  | True       | False   |                |
+-------------+-----------+-------+-----------------+-----------+-------+------------+---------+----------------+
| if_101_1001 | 1001      | 0     | False           | THREE_WAY | False | False      | True    | Level is leaf  |
+-------------+-----------+-------+-----------------+-----------+-------+------------+---------+----------------+
| if_101_1002 | 1002      | 0     | False           | THREE_WAY | False | False      | True    | Level is leaf  |
+-------------+-----------+-------+-----------------+-----------+-------+------------+---------+----------------+
| if_101_2    | 2         | 24    | False           | THREE_WAY | False | False      | False   |                |
+-------------+-----------+-------+-----------------+-----------+-------+------------+---------+----------------+

Sent Offers:
+-------------+-----------+-------+-----------------+-----------+
| Interface   | System ID | Level | Not A ZTP Offer | State     |
+-------------+-----------+-------+-----------------+-----------+
| if_101_1    | 101       | 23    | True            | THREE_WAY |
+-------------+-----------+-------+-----------------+-----------+
| if_101_1001 | 101       | 23    | False           | THREE_WAY |
+-------------+-----------+-------+-----------------+-----------+
| if_101_1002 | 101       | 23    | False           | THREE_WAY |
+-------------+-----------+-------+-----------------+-----------+
| if_101_2    | 101       | 23    | True            | THREE_WAY |
+-------------+-----------+-------+-----------------+-----------+

(Reviewed until here in this review iteration).

          +--rw instance-label?              uint32
          +--ro database
          |  +--ro ties* [direction-type originator tie-type tie-number]
          |     +--ro direction-type          enumeration
          |     +--ro originator              system-id
          |     +--ro tie-type                enumeration
          |     +--ro tie-number              uint32
          |     +--ro seq?                    uint64
          |     +--ro origination-time?       uint32
          |     +--ro origination-lifetime?   uint32
          |     +--ro node
          |     |  +--ro name?              string
          |     |  +--ro level?             level
          |     |  +--ro system-id          system-id
          |     |  +--ro pod?               uint32
          |     |  +--ro flood-reduction?   boolean
          |     |  +--ro overload?          boolean
          |     |  +--ro startup-time?      uint64
          |     |  +--ro neighbors* [system-id]
          |     |  |  +--ro name?                         string
          |     |  |  +--ro level?                        level
          |     |  |  +--ro system-id                     system-id
          |     |  |  +--ro pod?                          uint32
          |     |  |  +--ro address-families
          |     |  |  |  +--ro address-family* [address-family]
          |     |  |  |     +--ro address-family    iana-rt-types:address-family
          |     |  |  +--ro received-lie-tie-addresses
          |     |  |  |  +--ro rx-lie-source-address
          |     |  |  |  |  +--ro ipv4?   inet:ipv4-address
          |     |  |  |  |  +--ro ipv6?   inet:ipv6-address
          |     |  |  |  +--ro flooding-source-address
          |     |  |  |     +--ro ipv4?   inet:ipv4-address
          |     |  |  |     +--ro ipv6?   inet:ipv6-address
          |     |  |  +--ro remote-id?                    uint32
          |     |  |  +--ro cost?                         uint32
          |     |  |  +--ro metric?                       uint32
          |     |  |  +--ro bandwidth?                    uint32
          |     |  |  +--ro flood-reduction?              boolean
          |     |  |  +--ro received-link-capabilities
          |     |  |  |  +--ro bfd?                     boolean
          |     |  |  |  +--ro v4-forwarding-capable?   boolean
          |     |  |  +--ro received-from-lies
          |     |  |  |  +--ro you-are-flood-repeater?        boolean
          |     |  |  |  +--ro not-a-ztp-offer?               boolean
          |     |  |  |  +--ro you-are-sending-too-quickly?   boolean
          |     |  |  +--ro tx-flood-port?                inet:port-number
          |     |  |  +--ro bfd-up?                       boolean
          |     |  |  +--ro outer-security-key-id?        uint8
          |     |  +--ro miscabled-links*   linkid-type
          |     +--ro prefix
          |        +--ro prefix?              inet:ip-prefix
          |        +--ro (type)?
          |        |  +--:(prefix)
          |        |  +--:(positive-disaggregation)
          |        |  +--:(negative-disaggregation)
          |        |  +--:(external)
          |        |  +--:(positive-external-disaggregation)
          |        |  +--:(pgp)
          |        +--ro metric?              uint32
          |        +--ro tags*                uint64
          |        +--ro monotonic-clock
          |        |  +--ro prefix-sequence-type
          |        |     +--ro timestamp         ieee802-1as-timestamp-type
          |        |     +--ro transaction-id?   uint8
          |        +--ro loopback?            boolean
          |        +--ro directly-attached?   boolean
          |        +--ro from-link?           linkid-type
          +--ro kv-store
             +--ro kvs* [kvs-index]
                +--ro kvs-index    uint32
                +--ro kvs-tie
                   +--ro direction-type?         enumeration
                   +--ro originator?             system-id
                   +--ro tie-type?               enumeration
                   +--ro tie-number?             uint32
                   +--ro seq?                    uint64
                   +--ro origination-time?       uint32
                   +--ro origination-lifetime?   uint32
                   +--ro key-value
                      +--ro key?     uint16
                      +--ro value?   uint32

  notifications:
    +---n error-set
       +--ro tie-level-error
       |  +--ro direction-type?         enumeration
       |  +--ro originator?             system-id
       |  +--ro tie-type?               enumeration
       |  +--ro tie-number?             uint32
       |  +--ro seq?                    uint64
       |  +--ro origination-time?       uint32
       |  +--ro origination-lifetime?   uint32
       +--ro neighbor-error
          +--ro neighbor* [system-id]
             +--ro name?                         string
             +--ro level?                        level
             +--ro system-id                     system-id
             +--ro pod?                          uint32
             +--ro protocol-version?             uint16
             +--ro address-families
             |  +--ro address-family* [address-family]
             |     +--ro address-family    iana-rt-types:address-family
             +--ro received-lie-tie-addresses
             |  +--ro rx-lie-source-address
             |  |  +--ro ipv4?   inet:ipv4-address
             |  |  +--ro ipv6?   inet:ipv6-address
             |  +--ro flooding-source-address
             |     +--ro ipv4?   inet:ipv4-address
             |     +--ro ipv6?   inet:ipv6-address
             +--ro remote-id?                    uint32
             +--ro cost?                         uint32
             +--ro metric?                       uint32
             +--ro bandwidth?                    uint32
             +--ro flood-reduction?              boolean
             +--ro received-link-capabilities
             |  +--ro bfd?                     boolean
             |  +--ro v4-forwarding-capable?   boolean
             +--ro received-from-lies
             |  +--ro you-are-flood-repeater?        boolean
             |  +--ro not-a-ztp-offer?               boolean
             |  +--ro you-are-sending-too-quickly?   boolean
             +--ro tx-flood-port?                inet:port-number
             +--ro bfd-up?                       boolean
             +--ro outer-security-key-id?        uint8