Re: [Rift] Dear RIFT protocol and RIFT YANG co-author, pls reviewtheupdateofRIFT YANG model

[Bruno Rijsman <[e-mail]>]

Hi Sandy,

Thanks for the quick update to the datamodel.

If you don't mind, I would like to continue with the iterative approach of providing a few more additional feedback comments after each update until we converge.

It works rather nicely for me to send just a few comments in each batch, and then waiting for the next update.

But if that doesn't work for you, and you would rather have all comments in one huge batch, let me know, and I can do that too.

Here is my next batch of comments:

module: ietf-rift
  augment /rt:routing/rt:control-plane-protocols/rt:control-plane-protocol:
    +--rw rift!
       +--rw node
          +--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 lie-ipv4-multicast-address?   rt-types:ipv4-multicast-group-address
          +--rw lie-ipv6-multicast-address?   rt-types:ipv6-multicast-group-address
          +--rw global-link-capabilities
          |  +--rw bfd?                     boolean
          |  +--rw v4-forwarding-capable?   boolean
          +--rw flood-port?                   inet:port-number
          +--rw lie-rx-port?                  inet:port-number

Suggest moving this up a few lines, to keep it together with lie-ipv4/6-multicast-address.

We have to decide whether we want to allow implementations to use different LIE multicast addresses for TX and RX (my implementation allows this, and I think Juniper's also).
If so:
 * I suggest renaming lie-ipv4/6-multicast-address to lie-rx-ipv4/6-multicast-address for the sake of clarity and consistency.
 * I suggest adding lie-tx-ipv4/6-multicast-address and lie-tx-port
 * Distinguish rx-flood-port (and interface attribute) and tx-flood-port (a neighbor attribute since it is dynamically advertised in the LIE)

          +--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]

We should add the advertised RIFT metric to the interface (and the received RIFT metric as a neighbor attribute -- see longer comment below).

We could consider adding the following attributes to interface:  was the most recently received LIE accepted or rejected, and if rejected, why?

          |  +--ro local-link-id?                  linkid-type
          |  +--rw name                            if:interface-ref
          |  +--rw advertised-lie-tie-addresses

I don't see much value putting this in a separate container

          |  |  +--ro lie-ipv4-source-address?        inet:ipv4-address
          |  |  +--ro lie-ipv6-source-address?        inet:ipv6-address
          |  |  +--ro flooding-ipv4-source-address?   inet:ipv4-address
          |  |  +--ro flooding-ipv6-source-address?   inet:ipv6-address

I suggest renaming this from xxx-ipvx-source-address to rx-xxx-ipvx-address (this is address that the interface uses to receive packets).

          |  +--ro direction-type?                 enumeration
          |  +--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

Fields such as you-are-flood-repeater, not-a-ztp-offer, you-are-sending-too-quickly, etc. etc. are all fields that are present in the LIE packet.
For such fields we should make a clear distinction between the value that this node advertised to the neighbor and the values (plural!) that this node received from the neihghbors (plural!).
So, we should have:
 * you-are-flood-repeater as an interface attribute (our advertised value) and you-are-flood-repeater as a neighbor attribute (the value that we received from the neighbor). Or maybe to make it even more clear, we could call them tx-you-are-flood-repeater and rx-you-are-flood-repeater (or advertised-you-are-flood-repeater and received-you-are-flood-repeater).
 * Ditto for not-ztp-offer
 * Ditto for you-are-sending-too-quickly
 * Ditto for link-capabilities (=> this one you already did: we have link-capabilities under interface and received-link-capabilities under neighbor)
 * Etc. etc. etc.

          |  +--ro neighbors

There can only be zero or one neighbor per interface. Thus, you don't need a neighbors (plural) list with neighbor objects indexed by system-id. You just need a single optional neighbor object.

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

I am not a YANG expert, but wouldn't a leaf-list make more sense here?

          |        +--ro received-lie-tie-addresses
          |        |  +--ro lie-ipv4-source-address?        inet:ipv4-address
          |        |  +--ro lie-ipv6-source-address?        inet:ipv6-address
          |        |  +--ro flooding-ipv4-source-address?   inet:ipv4-address
          |        |  +--ro flooding-ipv6-source-address?   inet:ipv6-address
          |        +--ro remote-id?                    uint32
          |        +--ro cost?                         uint32
          |        +--ro bandwidth?                    uint32
          |        +--ro flood-reduction?              boolean
          |        +--ro received-link-capabilities
          |        |  +--ro bfd?                     boolean
          |        |  +--ro v4-forwarding-capable?   boolean
          |        +--ro bfd-up?                       boolean
          |        +--ro outer-security-key-id?        uint8
          |        +--ro state?                        enumeration

The state should be an attribute of the interface, not the neighbor. In state one-way the interface doesn't have a neighbor but it does have a state (namely one-way).

(I stopped reviewing here, will wait for the next iteration)

          +--ro miscabled-links*              linkid-type
          +--rw (algorithm-type)?
          |  +--:(spf)
          |  +--:(all-path)
          +--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
          +--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 lie-ipv4-source-address?        inet:ipv4-address
          |     |  |  |  +--ro lie-ipv6-source-address?        inet:ipv6-address
          |     |  |  |  +--ro flooding-ipv4-source-address?   inet:ipv4-address
          |     |  |  |  +--ro flooding-ipv6-source-address?   inet:ipv6-address
          |     |  |  +--ro remote-id?                    uint32
          |     |  |  +--ro cost?                         uint32
          |     |  |  +--ro bandwidth?                    uint32
          |     |  |  +--ro flood-reduction?              boolean
          |     |  |  +--ro received-link-capabilities
          |     |  |  |  +--ro bfd?                     boolean
          |     |  |  |  +--ro v4-forwarding-capable?   boolean
          |     |  |  +--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 lie-ipv4-source-address?        inet:ipv4-address
             |  +--ro lie-ipv6-source-address?        inet:ipv6-address
             |  +--ro flooding-ipv4-source-address?   inet:ipv4-address
             |  +--ro flooding-ipv6-source-address?   inet:ipv6-address
             +--ro remote-id?                    uint32
             +--ro cost?                         uint32
             +--ro bandwidth?                    uint32
             +--ro flood-reduction?              boolean
             +--ro received-link-capabilities
             |  +--ro bfd?                     boolean
             |  +--ro v4-forwarding-capable?   boolean
             +--ro bfd-up?                       boolean
             +--ro outer-security-key-id?        uint8


> On Jul 3, 2020, at 11:42 AM, <[e-mail]> <[e-mail]> wrote:
> 
> Hi Bruno,
> 
> 
> 
> Thank you very much!
> 
> I updated the model again according to your comments.
> 
> And sorry, I made mistakes for database/neighbor/kv-store level.
> 
> Please review it one more time, thank you very much!
> 
> 
> 
> Best regards,
> 
> Sandy
>