[Rift] RIFT YANG model review, iteration rift-yang-tree200706

[Bruno Rijsman <[e-mail]>]

Hi Sandy,

Here is the next review iteration (for rift-yang-tree200706).

Once again, there is more reviewing to be done, but I am talking it step by step in bite-sized chunks.

— Bruno

module: ietf-rift
  augment /rt:routing/rt:control-plane-protocols/rt:control-plane-protocol:
    +--rw rift!
       +--rw node
          +--rw name?                        string

Node needs to be a list (indexed by name); in the case of multi-instance RIFT there can be more than one RIFT node per RIFT engine. That also implies that name is the key and hence mandatory. This brings us to:

+--rw node* [name]
   +--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 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 local-link-id?                  linkid-type

We should be consistent. Whenever an attribute foo is both advertised to a neighbor vs receive from a neighbor, either do (a) or (b) or (c) or (d) below. The current YANG model does a mixture of all of these:
(a) Call it foo under interface and also foo (same name) under neighbor (e.g. interface cost vs neighbor cost). The context (interface vs neighbor) determines whether it is local/sent/advertised vs remote/received.
(b) Call it advertised-foo under interface and received-foo under neighbor (e.g. advertised-lie-tie-addresses vs received-lie-tie-addresses)
(c) Call it local-foo under interface and remote-foo under neighbor (e.g. local-link-id vs remote-link-id).
(d) Nest foo in advertised-in-lies for the interface, and nest foo in received-in-lies for the neighbor (e.g. interface advertised-in-lies you-are-flood-repeater vs neighbor received-in-lies you-are-flood-repeater)

I prefer (a) but I could live with (b) or (c) or (d) as well, as long as we are consistent. For example, if we pick (a) you would remove local- from local-link-id.

          |  +--rw name                            if:interface-ref
          |  +--rw metric?                         uint32

Rename metric to cost, as discussed in separate e-mail.

          |  +--rw advertised-lie-tie-addresses
          |  |  +--ro rx-lie-source-address
          |  |  |  +--ro ipv4?   inet:ipv4-address
          |  |  |  +--ro ipv6?   inet:ipv6-address
          |  |  +--ro flooding-source-address
          |  |     +--ro ipv4?   inet:ipv4-address
          |  |     +--ro ipv6?   inet:ipv6-address
          |  +--ro direction-type?                 enumeration
          |  +--ro was-the-last-lie-accepted?      boolean

I suggest adding an optional last-lie-reject-reason (string) that explains why the LIE was rejected if was-the-last-lie-accepted is false.

          |  +--rw advertised-in-lies

YANG question: does it make sense to have a read-write container with only read-only attributes?

          |  |  +--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
          |  +--rw state?                          enumeration

State should be read-only and mandatory. The adjacency (and hence the interface) always has an operational (not a configured) state: one-way, two-way, three-way, or multiple-neighbors.

          |  +--ro neighbor

Neighbor should be optional. In state ONE_WAY there is no neighbor.

          |     +--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 rx-lie-source-address
          |     |  |  +--ro ipv4?   inet:ipv4-address
          |     |  |  +--ro ipv6?   inet:ipv6-address
          |     |  +--ro flooding-source-address
          |     |     +--ro ipv4?   inet:ipv4-address
          |     |     +--ro ipv6?   inet:ipv6-address
          |     +--ro remote-id?                    uint32
          |     +--ro cost?                         uint32
          |     +--ro metric?                       uint32
          |     +--ro bandwidth?                    uint32
          |     +--ro flood-reduction?              boolean
          |     +--ro received-link-capabilities
          |     |  +--ro bfd?                     boolean
          |     |  +--ro v4-forwarding-capable?   boolean
          |     +--ro received-from-lies

If we end up picking (d) in the comment near the top, rename received-from-lies => 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
          +--rw (algorithm-type)?
          |  +--:(spf)
          |  +--:(all-path)

I don't know this :(foo) syntax. What does it mean?

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


There is one sent offer per neighbor, and one received offer per neighbor (see example output of RIFT-Python "show node" below)

At this point in the YANG model we are already in the neighbor container. This implies that 
(a) We only need to keep track of one single sent offer and one received offer (not a list of offers indexed by system-id).
(b) We already know the system-id of the neighbor (it is an attribute of the neighbor container)
(c) For both sent and received offers, we need to keep track of the level and not-a-ztp-offer
(d) FOr the received offer, we additionally need to keep track of whether it is the best offer (in the Python implementation I keep track of some additional information for easier debugging, e.g. whether or not it was removed from consideration and if so why, once again see output below).

All of this leads to the following possible YANG model:

          +--rw interface* [name]
          |  +--ro local-link-id?                    linkid-type
          |  +--rw name                              if:interface-ref
          |  +--ro neighbor
          |     +--ro name?                           string
          |     +--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


agg_101> show node
Node:
+---------------------------------------+------------------+
| Name                                  | agg_101          |
[...]
+---------------------------------------+------------------+

Received Offers:
+-------------+-----------+-------+-----------------+-----------+-------+------------+---------+----------------+
| Interface   | System ID | Level | Not A ZTP Offer | State     | Best  | Best 3-Way | Removed | Removed Reason |
+-------------+-----------+-------+-----------------+-----------+-------+------------+---------+----------------+
| if_101_1    | 1         | 24    | False           | THREE_WAY | True  | True       | False   |                |
+-------------+-----------+-------+-----------------+-----------+-------+------------+---------+----------------+
| if_101_1001 | 1001      | 0     | False           | THREE_WAY | False | False      | True    | Level is leaf  |
+-------------+-----------+-------+-----------------+-----------+-------+------------+---------+----------------+
| if_101_1002 | 1002      | 0     | False           | THREE_WAY | False | False      | True    | Level is leaf  |
+-------------+-----------+-------+-----------------+-----------+-------+------------+---------+----------------+
| if_101_2    | 2         | 24    | False           | THREE_WAY | False | False      | False   |                |
+-------------+-----------+-------+-----------------+-----------+-------+------------+---------+----------------+

Sent Offers:
+-------------+-----------+-------+-----------------+-----------+
| Interface   | System ID | Level | Not A ZTP Offer | State     |
+-------------+-----------+-------+-----------------+-----------+
| if_101_1    | 101       | 23    | True            | THREE_WAY |
+-------------+-----------+-------+-----------------+-----------+
| if_101_1001 | 101       | 23    | False           | THREE_WAY |
+-------------+-----------+-------+-----------------+-----------+
| if_101_1002 | 101       | 23    | False           | THREE_WAY |
+-------------+-----------+-------+-----------------+-----------+
| if_101_2    | 101       | 23    | True            | THREE_WAY |
+-------------+-----------+-------+-----------------+-----------+

(Reviewed until here in this review iteration).

          +--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 rx-lie-source-address
          |     |  |  |  |  +--ro ipv4?   inet:ipv4-address
          |     |  |  |  |  +--ro ipv6?   inet:ipv6-address
          |     |  |  |  +--ro flooding-source-address
          |     |  |  |     +--ro ipv4?   inet:ipv4-address
          |     |  |  |     +--ro ipv6?   inet:ipv6-address
          |     |  |  +--ro remote-id?                    uint32
          |     |  |  +--ro cost?                         uint32
          |     |  |  +--ro metric?                       uint32
          |     |  |  +--ro bandwidth?                    uint32
          |     |  |  +--ro flood-reduction?              boolean
          |     |  |  +--ro received-link-capabilities
          |     |  |  |  +--ro bfd?                     boolean
          |     |  |  |  +--ro v4-forwarding-capable?   boolean
          |     |  |  +--ro received-from-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
          +--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 rx-lie-source-address
             |  |  +--ro ipv4?   inet:ipv4-address
             |  |  +--ro ipv6?   inet:ipv6-address
             |  +--ro flooding-source-address
             |     +--ro ipv4?   inet:ipv4-address
             |     +--ro ipv6?   inet:ipv6-address
             +--ro remote-id?                    uint32
             +--ro cost?                         uint32
             +--ro metric?                       uint32
             +--ro bandwidth?                    uint32
             +--ro flood-reduction?              boolean
             +--ro received-link-capabilities
             |  +--ro bfd?                     boolean
             |  +--ro v4-forwarding-capable?   boolean
             +--ro received-from-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