Hi John, Many thanks for the detailed review and comments!
Please some response inline... > -----Original Message----- > From: John G.Scudder [mailto:[email protected]] > Sent: Saturday, June 04, 2016 6:13 AM > To: [email protected]; [email protected] > Cc: [email protected]; [email protected] > Subject: Routing Directorate QA review of draft-ietf-i2rs-rib-data-model-05 > > Hello, > > I have been selected as the Routing Directorate QA reviewer for this draft. My > review is below. > > Regards, > > --John > > > Document: draft-ietf-i2rs-rib-data-model-05 > Reviewer: John Scudder > Review Date: June 3, 2016 > > > Summary: > > The document is reasonably understandable, although I did find myself with a > fair number of questions, which I've written below. As noted below, many of > these questions may only have arisen because I am not skilled in the subject > area. I did find one major issue, with the Security Considerations section, > which > will certainly need to be resolved somehow before the document is progressed. > > > Comments: > > In preparing my review, I reviewed the draft itself, but did not carefully > read all > the references. As a result, it's possible some of my observations might be in > the rough, when taken in context of the full body of work. It's also the case > that > I am not an experienced Yang practitioner, so some practices and idioms that > may be well known could easily have escaped me, and a similar disclaimer > applies. > > Finally, I did not review Section 3, YANG Modules, in detail, although I made > a > few suggestions. > > > Major Issues: > > 1. There are two problems with the Security Considerations section. The first > is > that the section isn't good (sorry). More on this below. The second is that it > references the i2rs-architecture document normatively, but that document is > listed under Informative References. The second problem is easily fixed, of > course, by simply moving the architecture document to Normative References. > > The security section, in its entirety, is: > > This document introduces no extra new security threat and SHOULD > follow the security requirements as stated in > [I-D.ietf-i2rs-architecture]. > > However, if I go look at the architecture document to see what "the security > requirements" are that I am supposed to follow, I find a fairly long Security > Considerations section which states nothing that formally appears to be a > requirement. It also says that instantiations of i2rs will provide more > detailed > analysis of security properties. Since this document doesn't do that, there > must > be some other i2rs document that does, right? And this document should > reference that one? > > It seems perfectly reasonable to state that this document doesn't introduce > any security considerations of its own, and to reference some foundational > document. However, the current security section doesn't do that effectively, > doesn't reference the correct document, and I wouldn't expect it to survive a > Security Directorate review. Suggesting a rewrite is beyond the scope of this > QA review, sorry, but one is needed. I don't think it necessarily needs to be > very > long, but it does need to correct these issues. Thanks for pointing this out, we will rewrite the security consideration. > > > Minor Issues: > > 1. The author list of six people exceeds the current RFC Editor guidance of > five > or fewer named authors. See RFC 7322: > > The total number of authors or editors on the first page is generally > limited to five individuals and their affiliations. If there is a > request for more than five authors, the stream-approving body needs > to consider if one or two editors should have primary responsibility > for this document, with the other individuals listed in the > Contributors or Acknowledgements section. Will discuss this among author and follow the guidance from the WG chairs and AD. > > 2. A number of symbols are not defined in Section 1.2, Tree Diagrams. I > noticed > the following as needing to be defined: { } x w n. OK. > > 3. Possibly this is covered in a companion document or is well-known to those > better versed in Yang than me, but I found myself wondering what the default > values for optional items were. In particular, every writable boolean is > optional. > Many have no default value, for example sharing-flag has none. But > return-failure-detail does have a default of false. Is it okay that some of > these > optional items have no default values? Is the expected behavior > implementation-specific in that case? Yes, an optional item can have no a default value and the behavior is implementation-specific. > > 4. "route-statistic" should probably be "route-statistics" (plural). Then > again, > looking at the content of "route-statistic", it's not what I would refer to as > statistics at all. The content is state (active or inactive), installation > state > (installed, or not) and reason (I guess this indicates the reason the route > entered its current state, see also comment 11 below). None of this seems like > statistical data to me. Maybe "status"? The name is inherited from the information model, "status" sounds like a better name. > > 5. The model doesn't seem to capture any restrictions on how nexthops can > be chained. Presumably there are some restrictions, for example it wouldn't > seem to make a whole lot of sense to chain egress-interface and > egress-interface-ipv4-nexthop together, the more so since the respective > outgoing-interfaces might conflict. Possibly capturing this is outside the > scope > of this document. I tend to believe this should be out of scope. How to use the nexthops chain depends on the use case. Section 7.2 of https://tools.ietf.org/html/draft-ietf-i2rs-rib-info-model-08 gives a set of use cases to describe how to use next hop combinations. > > 6. In various parts of the model, it appears to be possible to reference an > object > either by an opaque identifier or by value. Maybe this is a common idiom, but > it > wasn't obvious to me why this should be, it seems redundant. For example, > every route has a route-index, but when I want to operate on a route with > route-update or route-delete, I have to identify it by rib name and prefix. > The > utility of the index, then, is not clear to me. Another example is in Section > 2.5: Some of the index/identifier are really useless except for satisfying the grammar requirement of YANG, for example, a list must have a key that can be used to uniquely identify an item of the list. Some of the reference/identifier have their usage, for example, the nexthop identifier that is designed to provide a level of indirection hence to improve the route/nexthop update efficiency. > > o nh-delete: Delete a nexthop from a rib. A name > of a rib and a nexthop or nexthop identifier are passed as the > input parameters. > > Again, it's unclear to me why it's desirable to be able to delete the NH by > either > reference or value. For the NH case, IMHO, either the reference or value can be used for NH deletion. Which is used is up to the client/controller. > > 7. It was unclear to me why a reason is required for a route change > notification, > but not for a next hop change notification. Seems that an "interface" (physical or logic) up/down is the only reason for the status change of a next hop. Are there any other reasons? > > 8. This construct appears three times: > > leaf rib-family { > type rib-family-def; > mandatory true; > description > "A reference to address family of a rib."; > } > > However, in two locations the description is "The address family of a rib." > and > in the third, the "reference to" language is used. Probably this is just a > cut and > paste error, but because a reference is different from the thing itself > (presumably a value), maybe the choice of language really does indicate some > subtle difference? Because I haven't exhaustively reviewed Section 3, I think > it > is likely more such inconsistencies exist, and I think it would be good to > check > for them. OK. > > 9. "To download N > nexthops to the FIB, the N nexthops with the lowest > value are selected." > > What if more than N nexthops are tied for having the lowest value? As written, > this is underspecified. Will add some text to clarify this potential case. > > 10. "Nhop-lb-weight is a number between 1 and 99."; > > First, this doesn't use the correct name of the value it is describing. > Second, it's > not an adequate description. (It tells the reader nothing helpful.) OK, will fix it. > > 11. "Indicate the route reason." > > Again, this doesn't tell the reader anything helpful. Reason for what? OK, will fix it. > > > > Nits: OK, will fix the following nits. > > I found a number of editorial nits. Rather than enumerating them here, I > edited > my suggested changes into a copy of > https://www.ietf.org/id/draft-ietf-i2rs-rib-data-model-05.txt. I have > provided > both that copy, and a diff against the original, in attachments. Please review > my changes and don't just accept them blindly, while I intended that all my > changes were strictly editorial there's always the chance I altered the > meaning > unintentionally. > > In addition, I noticed some other issues where I'm not sure how they should be > resolved: > > 1. The term "RIB" is used inconsistently throughout the document. In some > places it's capitalized, as "RIB". In others, it's lowercase, as "rib", or > even > mixed-case, "Rib". I didn't change this in my marked up copy, for two reasons. > First, global search and replace would not be straightforward, because the > string also occurs where lowercase is clearly appropriate, for example > "ietf-i2rs-rib" (there are 331 occurrences of RIB in any combination of upper > and lowercase by my count). Second, I didn't completely discount the > possibility > of the authors are deliberately using the lowercase term sometimes. I > recommend either the term should be uppercase ("RIB") throughout, or if a > distinction between "RIB" and "rib" is intended, that should be explained in > the > Definitions and Acronyms section. > > 2. "there should be a limitation on how many levels of lookup can be > practically > performed." I suspect what the authors mean here is "there might be a > limitation", meaning a practical limitation might exist (indeed, probably does > exist) in the hardware implementing forwarding. I've suggested that change in > my marked up copy. However, may be the authors really do mean that there > needs to be a configurable limitation to allow restriction to something less > than > what the hardware implements. The fact that lookup-limit is a rw value > seems to support this – I don't see why that would be a configurable value if > it > represents an expression of what the hardware is capable of. On the other > hand, the document is silent about what an implementation is supposed to do > with that value once configured. (Maybe one of the companion documents > explains?) > > 3. The rib-list under routing-instance is indexed by a field called "name". > Under > the notification hierarchy the corresponding field is called "rib-name". it > works as written, of course, but it caused a little dissonance for me. (See #4 > below as well.) > > 4. The name "rib-family" wasn't self-explanatory for me. "rib-address-family" > would have been. For that matter, why prefix "rib-" onto it since you haven't > prefixed "rib-" onto the other variables (see #3 above as well). > > 5. Shouldn't there be an ellipsis right below rw-route-vendor-attributes in > Figure 1? > > 6. In Section 2.1, capability negotiation is referred to twice, once in the > first > paragraph and once in the last. I think you probably mean capability > advertisement. The distinction is that in advertisement, an entity (the > router) is > telling another entity what it's capabilities are. There is nothing to > negotiate > about per se, if the other entity doesn't like the router's capabilities, it > can't > very well convince the router to change them. I have changed "negotiation" to > "advertisement" in my marked up copy accordingly, if that's not right you > should revert it but also clarify. > > 7. "nexthop-lbs" is a strange choice of name for load-balancing. I would > suggest > either dropping the S and just making it "nexthop-lb", or spelling it out in > full, > "nexthop-load-balance". But something the casual reader is likely to see as > "next hop pounds" ("lbs" is the common abbreviation for the English system > unit of weight pounds, of course) seems problematic. > > 8. I couldn't understand this sentence at all: > > * failure-detail: shows the specific failed routes that failure > reason. > > Since I didn't understand it, I can't suggest a rewrite, sorry. (The text > recurs three times in section 2.5.) > > 9. nh-add in Section 2.5 talks about "the network node" doing something: > > o nh-add: Add a nexthop to a rib. A name of the > rib and a nexthop are passed as the input parameters. The network > node is required to allocate a nexthop identifier to the nexthop. > The outputs include the result of the nexthop add operation. > > As far as I can tell, the entire rest of the document talks about "the i2rs > agent" doing something. Should "network node" the rewritten accordingly? > > 10. Similar to #9, "A RIB data-model MUST support sending 2 kind of > asynchronous notifications" Doesn't seem right. Surely the data model per se > doesn't support sending anything at all, it's the i2rs agent that supports > it? I've > tentatively replaced the text with "An implementation of this RIB data model" > (cribbed from lower down in the doc). Best regards, Mach _______________________________________________ i2rs mailing list [email protected] https://www.ietf.org/mailman/listinfo/i2rs
