Re: [yang-doctors] Yangdoctors last call review of draft-ietf-rtgwg-yang-rib-extend-06

[Yingzhen Qu <[e-mail]>]

Hi Martin,

Thank you for your review, and we've published version -07 to address your
comments.

Please see my answers below inline.

Thanks,
Yingzhen

On Wed, Apr 14, 2021 at 4:51 AM Martin Björklund via Datatracker <
[e-mail]> wrote:

> Reviewer: Martin Björklund
> Review result: Ready with Nits
>
> Here is my YANG doctors review of draft-ietf-rtgwg-yang-rib-extend-06.
> This is a well-written draft, and my comments are minor.
>
>
>
> o  1.  Introduction
>
>    This document defines a YANG [RFC6020][RFC7950] data model which
>    extends the generic data model for RIB by augmenting the ietf-routing
>    model as defined in [RFC8349].
>
>   Nit:   s/ietf-routing model/ietf-routing YANG module/
>
> [Yingzhen]: fixed.


> o  2. Terminology
>
>   You import a couple of terms that are not used, I suggest you remove
>   them.
>
> [Yingzhen]: removed a few terms not used.


>   In 2.1 you introduce RIB, but since this term is defined in RFC
>   8349, I suggest you import it instead.
>
> [Yingzhen]: modified as suggested.

>
> o  3. Design of the Model
>
>    The YANG definitions in this document augment the ietf-routing model
>    defined in [RFC8349], which provides a basis for routing system data
>    model development.  Together with modules defined in [RFC8349], a
>    generic RIB Yang model is defined to implement and monitor a RIB.
>
>   Perhaps:
>
>    The YANG definitions in this document augment the routing data model
>    defined in [RFC8349], which provides a basis for routing system data
>    model development.  Together with the YANG modules defined in
> [RFC8349], a
>    generic RIB YANG model is defined to implement and monitor a RIB.
>
> [Yingzhen]: fixed.


> o  3 & 4
>
>   To make the text in 3 easier to understand, and 4 easier to read, I
>   would move some snippets from 4 to 3.  For example:
>
>    3.1.  RIB Tags and Preference
>
>      Individual routes tags are supported at both the route and next-hop
>      level.  A preference per next-hop is also supported for selection of
>      the most preferred reachable static route.
>
>      augment /rt:routing/rt:control-plane-protocols
>              /rt:control-plane-protocol/rt:static-routes/v4ur:ipv4
>              /v4ur:route/v4ur:next-hop/v4ur:next-hop-options
>              /v4ur:simple-next-hop:
>        +--rw preference?   uint32
>        +--rw tag?          uint32
>
>      augment /rt:routing/rt:control-plane-protocols
>              /rt:control-plane-protocol/rt:static-routes/v6ur:ipv6
>              /v6ur:route/v6ur:next-hop/v6ur:next-hop-options
>              /v6ur:simple-next-hop:
>        +--rw preference?   uint32
>        +--rw tag?          uint32
>
>
>    Etc.
>
>    If each augment is explained in section 3, section 4 can be removed.
>
> [Yingzhen]: thank you for the good suggestion. Now tree snapshots are in
section 3. I kept section 4, just saying the full tree is in the appendix.

>
> o  module description
>
>      This YANG module extends the generic data model for
>      RIB by augmenting the ietf-routing model.  It is
>      intended that the module will be extended by vendors
>      to define vendor-specific RIB parameters.
>
>   I don't think I understand this description.  Here's my understanding,
>   but I don't think it is correct:
>
>     1. This module extends the existing RIB data model by using
>        augmentations.
>     2. The existing RIB data model is defined in the YANG module
>        ietf-routing.
>     3. The purpose of this new module is to allow vendors to extend the
>        the existing RIB data model with vendor-specific parameters.
>
>   It seems 3 is at least incomplete, since this module defines some
>   additional config param for static routes, and addtional state and
>   statistics for ribs.
>
>   It is not clear how vendors are expected to extend this model; the
>   word "vendor" doesn't show up anywhere else.
>
> [Yingzhen]: This module does define additional parameters and is
augmenting the existing RIB model. The module can be further augmented. Any
suggestions for a replacement of "vendor-specific"?


> o  revision
>
>   We usually write "Initial revision.".
>
> [Yingzhen]: fixed.


> o  rib-summary-statistics
>
>          leaf total-routes {
>            type uint32;
>            description
>              "Total routes in the RIB from all protocols";
>          }
>          leaf total-active-routes {
>            type uint32;
>            description
>              "Total active routes in the RIB";
>          }
>          leaf total-route-memory {
>            type uint64;
>            description
>              "Total memory for all routes in the RIB from all
>               protocol clients";
>
>   These three leafs have slightly different descriptions, so I wonder
>   if there is a difference between "from all protocols", "from all
>   protocol clients" and no mentioning of protocols?
>
> [Yingzhen]: changed the names and the descriptions. Now the RIB and
protocol level are consistent.

>
> o  naming of statistics
>
>   The draft defines:
>
>     augment /rt:routing/rt:ribs/rt:rib:
>       +--ro rib-summary-statistics
>          +--ro total-routes?              uint32
>          +--ro total-active-routes?       uint32
>          +--ro total-route-memory?        uint64
>          +--ro protocol-rib-statistics* []
>             +--ro rib-protocol?             identityref
>             +--ro protocol-total-routes?    uint32
>             +--ro protocol-active-routes?   uint32
>             +--ro protocol-route-memory?    uint64
>
>    The names seem to repeat some words where is it not necessary,
>    e.g., there's no reason to call it 'rib-summary-statistics' under the
>    'rib' list.  It is more that summary; so I suggest simply 'statistics'.
>    Also, what is a "rib protocol"?
>
>    Perhaps:
>
>     augment /rt:routing/rt:ribs/rt:rib:
>       +--ro statistics
>          +--ro total-routes?              uint32
>          +--ro total-active-routes?       uint32
>          +--ro total-route-memory?        uint64
>          +--ro protocol-statistics* []
>             +--ro protocol?             identityref
>             +--ro routes?               uint32
>             +--ro active-routes?        uint32
>             +--ro route-memory?         uint64
>
>
> o  protocol-rib-statistics
>
>          list protocol-rib-statistics {
>            description "List protocol statistics";
>
>    Perhaps:
>            description
>              "RIB statistics per protocol.";
>
>
>            leaf rib-protocol {
>              type identityref {
>                base rt:routing-protocol;
>              }
>              description "Routing protocol for statistics";
>
>   Perhaps just:
>
>              description
>                "Routing protocol.";
>
> [Yingzhen]: Please see my reply above.

>
> o  formatting
>
>   I suggest you run the module through:
>
>     pyang -f yang --yang-canonical [e-mail]
>
>   to fix some formatting and statement ordering issues.
>
> [Yingzhen]: some formatting and editorial changes were made.