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 <noreply@xxxxxxxx> 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 ietf-rib-extension@xxxxxxxxxxxxxxx
to fix some formatting and statement ordering issues.
[Yingzhen]: some formatting and editorial changes were made.
-- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
