Re: [Rift] RIFT YANG datamodel implementation

[Tony Przygienda <[e-mail]>]

On Tue, Mar 16, 2021 at 3:29 AM <[e-mail]> wrote:

> Hi tony,
>
> Sorry for the late response.
>
> Thank you very much for your detailed review!
>
> About you comments, could you please find my comments/question below with
> Sandy>?
>
> Much appreciate for it.
>
> Best regards,
>
> Sandy
> 原始邮件
> *发件人：*TonyPrzygienda
> *收件人：*Bruno Rijsman;
> *抄送人：*[e-mail];
> *日 期 ：*2021年03月10日 19:29
> *主 题 ：**Re: [Rift] RIFT YANG datamodel implementation*
> _______________________________________________
> RIFT mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/rift
>
> Just reviewed stuff carefully as well (also based on _real_ implemenation
> ;-) but not Yang implementation). comment on -02
>
>
>           +--ro hierarchy-indications?       enumeration
>
>
> that's bit more complex, we're lacking
>
>     leaf_only_and_leaf_2_leaf_procedures = 1,
>
> which should be configurable
>
> Sandy> The type of "hierarchy-indications" is enumeration, and "
> leaf_only_and_leaf_2_leaf_procedures" is included in it. Now the leaf is
> not configurable, do you mean this leaf should be changed to configurable?
>

yes, you want to be able to configure that


>
> also, observe that level == 24 that can be configued is NOT equivalent to
> top-of-fabric which is a special flag
>
> so
>
>        leaf configured-level {          type level;          description            "The configured level value of this node.";        }
>
> should be something much more complex around a union of
>
> tof
>
> leaf-only
>
> leaf-with-leaf-2-leaf
>
> numeric
>
> Sandy> I'd like to add more description: "The value '0' indicates leaf-only, the value "1" indicates leaf-with-leaf-2-leaf, and the value "2' indicates tof." Do you think it's OK?
>
>
>
well, you can do that but it's really an enum where the value should not
matter


> then
>
> overload
>
> in configuration sense should be
>
> overload {
>
> status: true/false
>
> timeout: optional timeout on config element
>
> }
>
> so I'd split between configured-overload and in-overload or just overload
> Sandy> What's the meaning of "timeout"? Now in the model, the local "overload" is configurable, the "overload" in node TIE isn't configurable.
>
>
timeout is used very often. Basically there should be 2 timeouts

on-startup

this means node goes into overload until this tiemr expires when starting
up

immediate

it means, set overload (but not on startup, right when configure) and
remove after timeout expired


>
> if we allow to configure maximum nonce delta we should probably also allow to configure lifetime delta (both are pretty dangerous since they can break convergence real fast)
>
> +--rw holdtime?
>
> should be global-holdtime
> Sandy> OK.
>
>
ack


> since that can be ultimately per link
>
> same for tide generation
>
>           +--rw tie-security-key-id?         uint32
>
> that should refer to standard yang security chain as e.g. OSPF uses
>
> that should be also originating-key-id or something since we need (missing right now) the keys we are willing to accept as RW
> Sandy> May I change this to the following, it's borrowed from OSPF YANG model. The crypto-algorithm has many types, does RIFT support all of them, or just some of them?
>           |     |     |     |  +--rw tie-security
>           |     |     |     |     +--:(auth-key-chain)?
>           |     |     |     |     |  +--rw key-chain?
>           |     |     |     |     |         key-chain:key-chain-ref
>           |     |     |     |     +--:(auth-key-explicit)
>           |     |     |     |        +--rw key-id?     uint32
>           |     |     |     |        +--rw key?        string
>           |     |     |     |        +--rw crypto-algorithm?
>           |     |     |     |                identityref
>
> on acceptance we need following variants
>
> SecurityChecking {
>    NoChecking,
>    Permissive,
>    Loose,
>    Strict,
> }
> Sandy> OK.
>
>
ack

> same @ link level for the security keys
> Sandy> OK. Once you confirm the security key format, it will be syned to interface.
>
>
ack, basically keep to whatever OSPF does,. I thought ACee abstracted the
whole keychain IGP stuff and we should use that


> look @ YAML model we use in open source RIFT to see further details
>
> own pod missing
> Sandy> The "pod" is the fourth leaf in node, please check it agin, do I make some mistakes?
>
>
maybe I missed


>
> @ interface level
>
> what is
>
>     +--ro hal?    level
>
> it seems backwards, HAL is of level type, no?
> Sandy> The "HAL" is "The highest defined level value seen from all valid level offers received." Does the description need to be improved?
>
>
well, name the node HAL and the type is level


> also list of HAL systemid offers is helpful here
> Sandy> OK. I can add the systemid which offer the level.
>
>
yepp, there maybe multiple


> also on link add instance-name
> Sandy> Do you mean the RIFT instance name? The whole model is following an instance, so the interface is also belong the same instance with the node. Different instances are distinguished by the node.
>
> DATABASE
>
>
>                 |  +--ro (type)?                 |  |  +--:(prefix)                 |  |  +--:(positive-disaggregation)                 |  |  +--:(negative-disaggregation)                 |  |  +--:(external)                 |  |  +--:(positive-external-disaggregation)                 |  |  +--:(pgp)
>
> oder strictly per thrift model since order has meaning here
> Sandy> OK. I'll modify the order according to section 8.2.28.1.
>
>
ack


>                 |  |  +--ro link-id-pair* [remote-id]                 |  |  |  +--ro local-id?    uint32                 |  |  |  +--ro remote-id    uint32                 |  |  |  +--ro if-index?    uint32  Rijsman, et al.          Expires August 26, 2021                [Page 7] Internet-Draft               RIFT YANG Model               February 2021                  |  |  |  +--ro if-name?     if:interface-ref                 |  |  +--ro cost?                         uint32                 |  |  +--ro bandwidth?                    uint32                 |  |  +--ro flood-reduction?              boolean                 |  |  +--ro received-link-capabilities                 |  |     +--ro bfd?                     boolean                 |  |     +--ro v4-forwarding-capable?   boolean
>
> missing
>
>    14: optional set<common.AddressFamilyType>
>        (python.immutable = "")                address_families;
> Sandy> OK. I'll add it.
>
>
ack