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

From: <[email-masked]>
Date: Wed, 1 Jul 2020 17:18:10 +0800 (CST)

Hi Bruno,

Thank you very much for your comments!

Please find my answer inline with Sandy>.

Thanks,

Sandy

原始邮件

发件人：BrunoRijsman <[email-masked]>
收件人：张征00007940;
抄送人：Antoni Przygienda <[email-masked]>;[email-masked] <[email-masked]>;[email-masked] <[email-masked]>;[email-masked] <[email-masked]>;[email-masked] <[email-masked]>;魏月华00019655;[email-masked] <[email-masked]>;[email-masked] <[email-masked]>;[email-masked] <[email-masked]>;[email-masked] <[email-masked]>;
日 期 ：2020年06月30日 18:17
主 题 ：Re: Dear RIFT protocol and RIFT YANG co-author, pls review theupdateof RIFT YANG model

I have not yet finished reviewing the complete data model, but here are some initial additional comments from me:

module: ietf-rift

  augment /rt:routing/rt:control-plane-protocols/rt:control-plane-protocol:

    +--rw rift!

       +--rw node

       |  +--rw name?                         string

       |  +--rw level?                        level

We need to distinguish configured-level vs derived-level (see section 4.2.7.1. in draft-ietf-rift-rift-12).

Sandy> In fact this is the effective value in the device. It's not directly according to the CLI configuration. 

The leaf hal below indicates the potential drived level value. It seems like it is not necessary to add a dedicated configured value for it? What do you think of it?

       |  +--rw system-id                     system-id

       |  +--rw pod?                          uint32

       |  +--rw overload?                     boolean

       |  +--rw node-capability

The neighbors container (below) should contain the capabilities advertised by each neighbor.

Sandy> OK. I'll add it. 

       |  |  +--ro protocol-minor-version?   uint16

This should not be optional; the RIFT engine always has a specific minor version and it is a required field in the Thrift model.

Sandy> OK. I'll modify it to mandatory. 

       |  |  +--ro hierarchy-indications?    enumeration

       |  |  +--rw flood-reducing-capable?   boolean {flood-reducing}?

Linguistic hairsplitting: is capable the right word? If we set this to false, it only means that flood reduction has been disabled, not that the node is not capable of flood reduction.

Sandy> You are right. But I modified the name according to Tony's comments. Do you think it's better to modify the feature name to "flood-reducing" other than leaf name?  

       |  |  +--rw nonce-delta?              uint8 {nonce-delta-adjust}?

Why is this under capabilities?

Sandy> I'll move this leaf out.

       |  +--rw lie-ipv4-multicast-address?   rt-types:ipv4-multicast-group-address

       |  +--rw lie-ipv6-multicast-address?   rt-types:ipv6-multicast-group-address

       |  +--rw link-capability

The link capabilities are per link, so they should be in the neighbor table below.

Sandy> I will move it to interface and neighbor. But do we need a global setting for it?

       |  |  +--rw bfd?                     boolean {bfd}?

       |  |  +--rw v4-forwarding-capable?   boolean

       |  +--rw flood-port?                   inet:port-number

       |  +--rw lie-rx-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 {authentication}?

       |  +--rw local-nonce?                  uint16

This should be ro instead of rw

Distinguish nonce-local (sent) versus nonce-remote (received)
Nonces are per interface, so it should be in the neighbor table below

Sandy> Is this value can be adjusted? 

Do we use the same nonce on all the interface? If not, do you think it's OK to move the leaf to link-id-pair? 

       |  +--ro interfaces

       |  |  +--ro interface* [name]

       |  |     +--ro local-link-id?                 linkid-type

       |  |     +--ro name                           if:interface-ref

YANG newbie question, all tables in this document have this approach using 2 levels of nesting. Why is it not simply 1 level of nesting like this:

Sandy> OK. I'll modify it.

  +--ro interfaces

     +--ro interface* [name]

     +--ro local-link-id

     +--ro name

     ...

       |  |     +--ro address?                       inet:ip-address

The address of the interface does not belong here in the RIFT model; there is a separate IETF YANG data model for interfaces.
You could make an argument that the source address used for sending RIFT packets could be modeled; if so, must use separate fields for lie-ipv4-source-address, lie-ipv6-source-address (LIEs are sent BOTH on IPv4 and IPv6), flooding-source-address (flooding is done on IPv4 OR IPv6).

Sandy> OK. I'll add four leaves instead of it.

       |  |     +--ro if-index?                      uint32

Belongs in the interface YANG model, not in the RIFT YANG model.

Sandy> OK. I'll delete it.

       |  |     +--ro direction-type?                enumeration

       |  |     +--ro you-are-flood-repeater?        boolean

       |  |     +--ro not-a-ztp-offer?               boolean

       |  |     +--ro you-are-sending-too-quickly?   boolean

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

In RIFT you cannot have multiple (>1) neighbors per interface.

Thus, a neighbor should be an optional single-element container under neighbor.
Even if you do not agree with that and want to keep neighbor ourside of interface, it should be under node and not under rift

Sandy> I understand your point. But there may be multiple parallel links connect to a same neighbor, right?

       |  +--ro nbrs* [system-id]

Spell nbrs out (neighbors)

Sandy> No problem. I'll modify it.

       |     +--ro name?           string

       |     +--ro level?          level

       |     +--ro system-id       system-id

       |     +--ro pod?            uint32

       |     +--ro address?        inet:ip-address

       |     +--ro cost?           uint32

       |     +--ro remote-nonce?   uint16

       |     +--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 bfd-up?                  boolean

       |     |  +--ro outer-security-key-id?   uint8 {authentication}?

       |     |  +--ro address-families

       |     |     +--ro address-family* [address-family]

       |     |        +--ro address-family    iana-rt-types:address-family

       |     +--ro bandwidth?      uint32

       |     +--ro state?          enumeration

       +--ro database

The database container should be under node and not under rift

Sandy> Yes. The database container is under node, not under rift. Please reconfirm it. Thank you very much!

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

       |     |  |  +--ro protocol-minor-version?   uint16

       |     |  |  +--ro hierarchy-indications?    enumeration

       |     |  |  +--ro flood-reducing-capable?   boolean {flood-reducing}?

       |     |  |  +--ro nonce-delta?              uint8 {nonce-delta-adjust}?

       |     |  +--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?        inet:ip-address

       |     |  |  +--ro cost?           uint32

       |     |  |  +--ro remote-nonce?   uint16

       |     |  |  +--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 bfd-up?                  boolean

       |     |  |  |  +--ro outer-security-key-id?   uint8 {authentication}?

       |     |  |  |  +--ro address-families

       |     |  |  |     +--ro address-family* [address-family]

       |     |  |  |        +--ro address-family    iana-rt-types:address-family

       |     |  |  +--ro bandwidth?      uint32

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

Replace nbr by neighbor (spell it out)

Sandy> No problem. I'll modify it.

          +--ro nbrs* [system-id]

             +--ro name?           string

             +--ro level?          level

             +--ro system-id       system-id

             +--ro pod?            uint32

             +--ro address?        inet:ip-address

             +--ro cost?           uint32

             +--ro remote-nonce?   uint16

             +--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 bfd-up?                  boolean

             |  +--ro outer-security-key-id?   uint8 {authentication}?

             |  +--ro address-families

             |     +--ro address-family* [address-family]

             |        +--ro address-family    iana-rt-types:address-family

             +--ro bandwidth?      uint32