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

[[e-mail]]

Hi Bruno,






Thank you very much for your comments!


Please find my answer inline with Sandy>.






Thanks,


Sandy














原始邮件



发件人：BrunoRijsman <[e-mail]>
收件人：张征00007940;
抄送人：Antoni Przygienda <[e-mail]>;[e-mail] <[e-mail]>;[e-mail] <[e-mail]>;[e-mail] <[e-mail]>;[e-mail] <[e-mail]>;魏月华00019655;[e-mail] <[e-mail]>;[e-mail] <[e-mail]>;[e-mail] <[e-mail]>;[e-mail] <[e-mail]>;
日 期 ：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