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 protected]> 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 [email protected] > > to fix some formatting and statement ordering issues. > > [Yingzhen]: some formatting and editorial changes were made.
_______________________________________________ rtgwg mailing list [email protected] https://www.ietf.org/mailman/listinfo/rtgwg
