Subject: Yangdoctors last call review of draft-ietf-rtgwg-yang-rib-extend-06

From: Martin Björklund via Datatracker <[email-masked]>
Date: Wed, 14 Apr 2021 04:51:02 -0700

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/

o  2. Terminology

  You import a couple of terms that are not used, I suggest you remove
  them.

  In 2.1 you introduce RIB, but since this term is defined in RFC
  8349, I suggest you import it instead.

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.

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.

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.

o  revision

  We usually write "Initial revision.".

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?

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.";

o  formatting

  I suggest you run the module through:

    pyang -f yang --yang-canonical [email-masked]

  to fix some formatting and statement ordering issues.

---

From: Yingzhen Qu <[email-masked]>
Date: Thu, 22 Apr 2021 21:13:39 -0700

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 <
[email-masked]> wrote:

> [Yingzhen]: fixed.

> [Yingzhen]: removed a few terms not used.

> [Yingzhen]: modified as suggested.

> [Yingzhen]: fixed.

> [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.

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

> [Yingzhen]: fixed.

> [Yingzhen]: changed the names and the descriptions. Now the RIB and
protocol level are consistent.

> [Yingzhen]: Please see my reply above.

> [Yingzhen]: some formatting and editorial changes were made.

---

From: Martin Björklund <[email-masked]>
Date: Fri, 23 Apr 2021 09:19:03 +0200 (CEST)

Hi,

Thank you for addressing my comments.  Pruning to the one remaining
question.

> > This is a well-written draft, and my comments are minor.

[...]

> suggestions for a replacement of "vendor-specific"?

All models can be augmented so I don't think this needs to be spelled
out.  When it is spelled out like this I expect some discussion about
how it differ from the "normal" augment that always can be done.

Some modules define some generic common structure, but are not
very useful unless they are augmented; they need to define how vendors
(or sdos) should extend the module.  However, I think that this module
is useful on its own, and thus I would remove the sentence "It is
intended ...".  

/martin

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Fri, 23 Apr 2021 17:10:52 +0000

Hi Martin, 
See inline below.

On 4/23/21, 3:19 AM, "Martin Björklund" <[email-masked]> wrote:

    Hi,

    [...]

I'm not sure the source of this text but it has been almost boiler plate for protocol YANG models. Having said that, I don't have any problem removing it.

Thanks,
Acee

    /martin

---

From: Yingzhen Qu <[email-masked]>
Date: Fri, 23 Apr 2021 12:22:40 -0700

Hi Martin,

I just published version -08 and removed the sentence in the module
description and made a few editorial changes.

Thank you again for the review and please let us know if you have more
comments.

Thanks,
Yingzhen

On Fri, Apr 23, 2021 at 10:10 AM Acee Lindem (acee) <[email-masked]> wrote:

> I'm not sure the source of this text but it has been almost boiler plate
> for protocol YANG models. Having said that, I don't have any problem
> removing it.
>
> Thanks,
> Acee
>
>
>     /martin
>
>

---

From: tom petch <[email-masked]>
Date: Mon, 26 Apr 2021 09:03:54 +0000

From: rtgwg <[email-masked]> on behalf of Yingzhen Qu <[email-masked]>
Sent: 23 April 2021 20:22

Hi Martin,

I just published version -08 and removed the sentence in the module description and made a few editorial changes.

Thank you again for the review and please let us know if you have more comments.

<tp>
using a recent post, this is from Tom not Martin.

I do not see an improvement, perhaps -08 is worse.  I find the introductory text confusing.

A small but for me a telling point; RIB needs an article, 'a' or 'the' which have slightly different meanings and is mostly lacking

More technically, when RFC8349 was being developed. the sense was that 'path' had no meaning and so RFC8349 never uses it, only  'route'.  You use 'path' so you have to define it which experience with RFC8349 suggests is not straightforward.

You use 'best' in several places which is meaningless without criteria; best availability, best for delay variance, best for ...?  Again RFC8349 gets it right by using 'preferred' but that could be wrong here.  You seem to miss the concept that a RIB is a list of routes for one address family.

I expect the Abstract to lead me into the Introduction which leads me into subsequent sections.  Here I get confused as to just what this module is adding.  The Abstract has 'multiple next hops',  The Introduction has multiple paths, route metrics and administrative tags. s.3 then has augmentations for static routes to support multiple next-hop and more next-hop. which is then contradicted by 3.1 and 3.2  so what is the point of this I-D?  I should be clear before I start to look at the details of the YANG module and I am not.

So, Not Ready IMHO.

Tom Petch

Thanks,
Yingzhen

On Fri, Apr 23, 2021 at 10:10 AM Acee Lindem (acee) <[email-masked]<mailto:[email-masked]>> wrote:
Hi Martin,
See inline below.

On 4/23/21, 3:19 AM, "Martin Björklund" <[email-masked]<mailto:[email-masked]>> wrote:

    Hi,

    Yingzhen Qu <[email-masked]<mailto:[email-masked]>> wrote:
    > [email-masked]<mailto:[email-masked]>> wrote:
    >

    [...]

Thanks,
Acee

    /martin