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

Bruno Rijsman <brunorijsman@gmail.com> Tue, 30 June 2020 10:21 UTC

Return-Path: <brunorijsman@gmail.com>
X-Original-To: rift@ietfa.amsl.com
Delivered-To: rift@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 139703A1177 for <rift@ietfa.amsl.com>; Tue, 30 Jun 2020 03:21:41 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.098
X-Spam-Level:
X-Spam-Status: No, score=-2.098 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 8DqmJJV_nCaG for <rift@ietfa.amsl.com>; Tue, 30 Jun 2020 03:21:39 -0700 (PDT)
Received: from mail-ed1-x52d.google.com (mail-ed1-x52d.google.com [IPv6:2a00:1450:4864:20::52d]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 7393C3A1175 for <rift@ietf.org>; Tue, 30 Jun 2020 03:21:38 -0700 (PDT)
Received: by mail-ed1-x52d.google.com with SMTP id by13so5777292edb.11 for <rift@ietf.org>; Tue, 30 Jun 2020 03:21:38 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:message-id:mime-version:subject:date:in-reply-to:cc:to :references; bh=owJThwCknUWYjthMONQKxQSN0+T6ZLbnC1cMTwtZnMo=; b=BU3EOXmmyyQtjw13g9dO5+gNscwEqDOSBLogjh4xvaTxG8wjUIhzSW7Cdwn0HsUYhI +LGwkjEpN6Cf5JpcqQhjH9rr8IxOxQfc0IuhnD3Jdc8F1cqcQXQZvIqNcok7zV458Li+ OqketzfFJ3Cq1CgSfwCWDY0DbiFSNrUua5yRpKHS3JrsUXytRX0QMamClf1w648fUYI3 ieuP4GwhMeRzK6l1W2jnTsq2or8GwlmuZPg46YtAaQroU5giGbpZ5j5nWLa3D7aqNMzC a75oESgZVuuqu7/6zp4CXHBymMbdG700EGI8tlZIIrpszq/U96lcojWBStEA9854G6Aw P1BA==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:message-id:mime-version:subject:date :in-reply-to:cc:to:references; bh=owJThwCknUWYjthMONQKxQSN0+T6ZLbnC1cMTwtZnMo=; b=Db5TBU7X3vlX5CMjJBmjteV8djQZGTztHXu5XCf7fTQhVriGXNkWBrzddlH+rt3igz HexMdYssXWhk/sO1sIRRcD22JlPOJByMQVg8TShUz9AfE6lq7nSDjvRGUl7IXvFFKTy8 oPtkmvSgJfBs1Hp45DGBOwBP07wpTJGCpqN3ldw4hSm84GgDZloLNYzLFT46ar/pTBLj Jxmc1MR+nEkaUNeUMpgoFjHXyoBP4UqlytWS4oU6+RZ5fcOq6BzOXNyEWe/yK1jHfZYu L4uC8j8s0DiQYnPsuqGJZtq4nlIr1lca/Z41XSukAVfvUrou3kk+CwEEPUt0uyCriW+/ E5TQ==
X-Gm-Message-State: AOAM531ov2/RzTC/hC5smerxsNrQNJsSNZRsUXDQ9CiZ3bi1yhOlCTkT oOilsV09I/zwVhPj3vSas+A=
X-Google-Smtp-Source: ABdhPJzXjL2/NGItdxRy9F34CVhsN84dbLiHs3PalGeweDktPaB2jJ1gZvSx/3Gfcjm58qGxZ1NgyA==
X-Received: by 2002:a05:6402:1614:: with SMTP id f20mr21252055edv.129.1593512496763; Tue, 30 Jun 2020 03:21:36 -0700 (PDT)
Received: from [192.168.2.13] (ip4da78921.direct-adsl.nl. [77.167.137.33]) by smtp.gmail.com with ESMTPSA id p4sm1638192eji.123.2020.06.30.03.21.33 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Tue, 30 Jun 2020 03:21:36 -0700 (PDT)
From: Bruno Rijsman <brunorijsman@gmail.com>
Message-Id: <FB7E6F7F-3BEA-4F33-866D-8598865E6D99@gmail.com>
Content-Type: multipart/alternative; boundary="Apple-Mail=_146301B1-D066-43D3-A7F8-A08728BCEF0B"
Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3608.60.0.2.5\))
Date: Tue, 30 Jun 2020 12:21:17 +0200
In-Reply-To: <B35D4561-DB7F-4A32-AA62-1FB68156E7B4@gmail.com>
Cc: Antoni Przygienda <prz@juniper.net>, alankar_sharma@comcast.com, "Pascal Thubert (pthubert)" <pthubert@cisco.com>, Dmitry Afanasiev <fl0w@yandex-team.ru>, zzhang_ietf@hotmail.com, wei.yuehua@zte.com.cn, mashaowen@gmail.com, xufeng.liu.ietf@gmail.com, Jeff Tantsura <jefftant.ietf@gmail.com>, "Jeffrey (Zhaohui) Zhang" <zzhang@juniper.net>, rift@ietf.org
To: zhang.zheng@zte.com.cn
References: <95E6F23B-AC7D-4379-842E-01CB1B2D2BD6@juniper.net> <202006291715550243774@zte.com.cn> <B35D4561-DB7F-4A32-AA62-1FB68156E7B4@gmail.com>
X-Mailer: Apple Mail (2.3608.60.0.2.5)
Archived-At: <https://mailarchive.ietf.org/arch/msg/rift/lj70oWlpYXhJLFbT-WF_4hQiNOU>
X-Mailman-Approved-At: Tue, 30 Jun 2020 08:01:44 -0700
Subject: Re: [Rift] Dear RIFT protocol and RIFT YANG co-author, pls review the updateof RIFT YANG model
X-BeenThere: rift@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Discussion of Routing in Fat Trees <rift.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/rift>, <mailto:rift-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/rift/>
List-Post: <mailto:rift@ietf.org>
List-Help: <mailto:rift-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/rift>, <mailto:rift-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 30 Jun 2020 10:21:41 -0000

+ mailing list

> On Jun 30, 2020, at 12:16 PM, Bruno Rijsman <brunorijsman@gmail.com> wrote:
> 
> 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).
> 
>        |  +--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.
> 
>        |  |  +--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.
> 
>        |  |  +--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.
> 
>        |  |  +--rw nonce-delta?              uint8 {nonce-delta-adjust}?
> 
> Why is this under capabilities?
> 
>        |  +--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.
> 
>        |  |  +--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
> 
>        |  +--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:
> 
>   +--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).
> 
>        |  |     +--ro if-index?                      uint32
> 
> Belongs in the interface YANG model, not in the RIFT YANG model.
> 
>        |  |     +--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
> 
>        |  +--ro nbrs* [system-id]
> 
> Spell nbrs out (neighbors)
> 
>        |     +--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
> 
>        |  +--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)
> 
>           +--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
>