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

From: Antoni Przygienda <[email-masked]>
Date: Wed, 1 Jul 2020 13:54:39 +0000

You got my comments a while ago, I wait for next version and comment again.

Generally, I was struggling to understand what was config & what was “state” of the device. You can solve it by flattening all in one hierarchy but you need be very careful what is rw and what is ro  or you split into two hierarchies (AFAIR you did that). Lots of comments here seems to fall out of that (and my comments as well)

  *   Tony

From: "[email-masked]" <[email-masked]>
Date: Wednesday, July 1, 2020 at 2:18 AM
To: "[email-masked]" <[email-masked]>
Cc: Antoni Przygienda <[email-masked]>, "Sharma, Alankar" <[email-masked]>, "[email-masked]" <[email-masked]>, Dmitry Afanasiev <[email-masked]>, "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, Zhaohui Zhang <[email-masked]>, "[email-masked]" <[email-masked]>
Subject: Re:Dear RIFT protocol and RIFT YANG co-author, pls review theupdateof RIFT YANG model

[External Email. Be cautious of content]

Hi Bruno,

Thank you very much for your comments!

Please find my answer inline with Sandy>.

Thanks,

Sandy

<https://urldefense.com/v3/__http:/www.zte.com.cn/__;!!NEt6yMaO-gk!R4KSOVP91PsP-Qn8RbionB8mHXxGujjZOkiRjYbUA_WJzPlTAo2pSoLLSnU1CQ$>
原始邮件
发件人：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

Juniper Business Use Only

---

From: Bruno Rijsman <[email-masked]>
Date: Thu, 2 Jul 2020 12:07:37 +0200

CC to RIFT mailing list and removed all individual emails from the CC list because the IETF mailing list will reject it if the CC list is too long.

This e-mail contains the discussion around the first batch of comments.

There will be additional batches of comments in separate e-mails.

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

Yes, it is necessary to configure the level. Or at the very least, it is necessary to configure which of the nodes are the top-of-fabric nodes. This is in fact the only configuration that is really required in RIFT.

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

I was thinking about the attribute name itself in my comment (as opposed to the feature name). For the attribute, I would use the exact same same as in the RIFT draft, which is flood-reduction. It is clear that we are talking about a capability because it is part of a capability group (I will have more to say about this in follow-up e-mail #1 after this one).

I don’t really see a need for having a flood-reduction feature in the YANG model.

In your updated data model, you moved hierarchy-indications and flood-reduction from a capabilities container to the node level. That is not what I had in mind.

Specifically, this is what I have in mind:
1) Create a group for node-capabilities (note: plural, not singular)
2) At the node level, have an attribute advertised-node-capabilities of type node-capabilities
3) At the neighbor level, have an attribute received-node-capabilities of type node-capabilities

Similarly:
1) Create a group for link-capabilities (once again, plural)
2) At the neighbor level, have an attribute advertised-link-capabilities of type link-capabilities. I am assuming here that we might advertise different link capabilities to different neighbors. If that is not the case, then we can move the advertised-link-capabilities from the neighbor level to the node level.
3) Also at the the neighbor level, have an attribute received-link-capabilities of type link-capabilities.

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

The question “do we need a global setting” is a very interesting one.

In general, for every attribute that is configurable per-neighbor it is useful to have a configurable per-node attribute that allows the user to change the default value for all neighbor without having to explicitly configure it for every neighbor.  This is not only true for neighbors but for every table.  I am no YANG expert, but this seems such a general problem to me that it seems hard to imagine that YANG has not already solved this in a more general way.  Is there no YANG-idiomatic way to apply inheritance or something like this (neighbor foo inherits default values for all its attributes from some template neighbor, but overrides different values for attribute X and Y)?
> 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? 

The implementation chooses the nonce for each sent packet, so it has the potential to change on each packet.

The remote implementation chooses the nonce for each received packet, so once again it has the potential to change on each packet.

The remote implementation must also reflect the most recently sent nonce, and there will be some delta between the most recently sent nonce and the most recently received reflected nonce. 

It does not make any sense to configure any of these (sent nonce, received nonce, received reflected nonce).

I though what you were trying to model was “the most recently sent nonce” and the “most recently received nonce”.

It *does* make sense to make the following things configurable:

 * The minimum interval for increasing sent nonces (e.g. every packet, once per minute, once per 5 minutes, etc.)

 * The maximum acceptable delta between the most recently sent nonce any received reflected nonce.

> Sandy> OK. I'll modify it.

It might make sense for a YANG expert to weigh in on this as well.

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

Maybe Tony can weigh in on this, but my understanding (and my code!) deals with this as follows.

If multiple interfaces are connected to the same neighboring node, then each interface is treated as a separate adjacency.

So, each interface has either 0 or 1 adjacencies=neighbors. There is no attempt to track that neighbor on interface A is actually the same neighbor as on interface B. Each adjacency=neighbor is treated completely independently (at least in my code, and I don’t recall any language in the draft that says otherwise).

That said, flooding reduction may choose to flood only on a subset of the links, but that is a general mechanism.

If there is a compelling reason to model neighbors separately from interfaces, and having parallel interfaces all refer to the same neighbor object, I would be very interested to know about this reason (because it would trigger me to restructure my code to do the same).

> Sandy> Yes. The database container is under node, not under rift. Please reconfirm it. Thank you very much!
I checked the most recent file that you e-mail (rift-yang-tree200702) and as far as I can tell, database and  neighbors and kv-store are still under rift instead of node.

— Bruno