Subject: [Rift] Comments on most recent version of the RIFT YANG data model.

From: Bruno Rijsman <[email-masked]>
Date: Thu, 4 Feb 2021 08:48:33 +0100

Here are my comments on the most recent version of the RIFT YANG data model.

I think it is in pretty good shape now.

I have not actually implemented the YANG data model and I am no YANG expert, so more eyeballs would be helpful.

— Bruno

module: ietf-rift
  augment /rt:routing/rt:control-plane-protocols/rt:control-plane-protocol:
    +--rw rift!
       +--rw node* [name]
          +--rw name                         string
          +--ro level?                       level
          +--rw system-id                    system-id
          +--rw pod?                         uint32
          +--rw configured-level?            level
          +--rw overload?                    boolean

>>> Suggest adding: ro protocol-major-version uint8

          +--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 link-id?                       linkid-type
          |  +--rw name                           if:interface-ref
          |  +--rw cost?                          uint32
          |  +--rw advertised-source-addresses
          |  |  +--rw ipv4?   inet:ipv4-address
          |  |  +--rw ipv6?   inet:ipv6-address
          |  +--ro direction-type?                enumeration
          |  +--ro was-the-last-lie-accepted?     boolean
          |  +--ro last-lie-reject-reason?        string
          |  +--ro advertised-in-lies
          |  |  +--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
          |  +--ro state                          enumeration

Suggest adding per interface:
- Number of flaps
- Time duration in current state

          +--ro miscabled-links*             linkid-type
          +--rw (algorithm-type)?
          |  +--:(spf)
          |  +--:(all-path)
          +--ro hal?                         level
          +--rw instance-label?              uint32

Suggest making this dependent on a {segment-routing} or maybe {label-switching} feature

          +--ro neighbor
          |  +--ro nbrs* [system-id]
          |     +--ro name?                         string
          |     +--ro level?                        level
          |     +--ro system-id                     system-id
          |     +--ro pod?                          uint32
          |     +--ro protocol-version?             uint16

protocol-minor-version

          |     +--ro address-families
          |     |  +--ro address-family* [address-family]
          |     |     +--ro address-family    iana-rt-types:address-family
          |     +--ro received-source-addresses
          |     |  +--ro ipv4?   inet:ipv4-address
          |     |  +--ro ipv6?   inet:ipv6-address
          |     +--ro link-id-pair* [remote-id]
          |     |  +--ro local-id?    uint32
          |     |  +--ro remote-id    uint32
          |     |  +--ro if-index?    uint32
          |     |  +--ro if-name?     if:interface-ref
          |     +--ro cost?                         uint32
          |     +--ro bandwidth?                    uint32
          |     +--ro flood-reduction?              boolean
          |     +--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
          |     +--ro received-link-capabilities
          |     |  +--ro bfd?                     boolean
          |     |  +--ro v4-forwarding-capable?   boolean
          |     +--ro 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 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

Maybe I am misunderstanding something, but the list of Node TIE attributes below appears to be
including several attributes that are not stored in the Node TIE, for example the
"sent-offer" and "received-offer" attributes.

          |     |  +--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-source-addresses
          |     |  |  |  +--ro ipv4?   inet:ipv4-address
          |     |  |  |  +--ro ipv6?   inet:ipv6-address
          |     |  |  +--ro link-id-pair* [remote-id]
          |     |  |  |  +--ro local-id?    uint32
          |     |  |  |  +--ro remote-id    uint32
          |     |  |  |  +--ro if-index?    uint32
          |     |  |  |  +--ro if-name?     if:interface-ref
          |     |  |  +--ro cost?                         uint32
          |     |  |  +--ro bandwidth?                    uint32
          |     |  |  +--ro flood-reduction?              boolean
          |     |  |  +--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
          |     |  |  +--ro received-link-capabilities
          |     |  |  |  +--ro bfd?                     boolean
          |     |  |  |  +--ro v4-forwarding-capable?   boolean
          |     |  |  +--ro 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
          |     +--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

What made you model the kv-store separately? It seems more logical to me to model it as part 
of the TIE database (similar to how you model node TIEs and the various flavors of prefix TIEs)

          +--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-source-addresses
             |  +--ro ipv4?   inet:ipv4-address
             |  +--ro ipv6?   inet:ipv6-address
             +--ro link-id-pair* [remote-id]
             |  +--ro local-id?    uint32
             |  +--ro remote-id    uint32
             |  +--ro if-index?    uint32
             |  +--ro if-name?     if:interface-ref
             +--ro cost?                         uint32
             +--ro bandwidth?                    uint32
             +--ro flood-reduction?              boolean
             +--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
             +--ro received-link-capabilities
             |  +--ro bfd?                     boolean
             |  +--ro v4-forwarding-capable?   boolean
             +--ro 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

---

From: Bruno Rijsman <[email-masked]>
Date: Thu, 18 Feb 2021 09:22:44 +0100

Hi Sandy,

Thank you for the prompt update!

A few very last nitpicks:

+--ro protocol-major-version       uint16

^^^ Should be a uint8: the Thrift datamodel has separate data types for (major) VersionType (i8) and MinorVersionType (i16). In both cases the Thrift data model has a note saying that the signed Thrift type should be interpreted as an unsigned.

+--ro time-of-current-state?         uint64

^^^ Having a “time” as “uint64” is somewhat ambiguous. Suggest  changing to either (a) seconds-in-current-state uint64 or (b) last-state-change date-and-time. The latter is better IMO.

+--ro kv-store

^^^ Rename kv-store to key-value (ties > key-value is consistent with ties > node, ties > prefix)

      +--ro key?     uint16
      +--ro value?   uint32

^^^ Change key type from uint16 to binary and change value data type from uint32 to binary. In the RIFT draft, keys are currently defined as strings (type KeyIdType) and values are also currently defined as strings (element 1 in KeyValueTIEElement).  However in the e-mail with Tony on 30-Jan-2021 we agreed that string should be changed to binary.

Once again, I would like to call on other people on the mailing list to also review the Yang data model.  Another pair of eyes is definitely needed.

— Bruno

> On Feb 17, 2021, at 9:36 AM, [email-masked] wrote:
> 
> Hi Bruno, 
> 
> I updated the model according to your comments. 
> 
> Could you please review it and give me more comments?
> 
> Appreciate for your review! 
> 
> Thank you very much! 
> 
> Best regards,
> 
> Sandy
> 
> 原始邮件
> 发件人：BrunoRijsman
> 收件人：[email-masked];张征00007940;魏月华00019655;
> 日 期 ：2021年02月04日 15:48
> 主 题 ：Comments on most recent version of the RIFT YANG data model.
> <rift20210216.yang><rift-tree20210217.txt><Diff_ rift-tree20210128.txt - rift-tree20210217.txt.html>