<WG Chair Hat Off also> Hi Alvaro,
Some responses in-line. Thanks for taking the time to give a good review. Alia On Wed, Oct 9, 2013 at 2:49 PM, Alvaro Retana (aretana) <[email protected]>wrote: > On 9/27/13 8:53 AM, "Alvaro Retana (aretana)" <[email protected]> wrote: > > We want to hear from people who have read and understood the draft > (besides the authors!) about this topic. Please provide some explanation > as to why you support or not the adoption of the draft — avoid "+1". > > > <WG Chair Hat Off> > > Hi! > > I reviewed the draft and put some comments below. In general, I think > the draft is well written and surprisingly (given the potential complexity > of the topic) relatively easy to follow..but not in one sitting! > [Alia] It does take a couple reads to sink in. If you have suggestions on how to improve it, please do send them along. We've tried to make it as readable as possible - but after being heads-down in it for a while, it's hard to get that perspective. > I do need the authors to clarify one thing before I "cast my vote" about > adoption: it's Intended Status. The draft has an intended status of > Informational. However, the Introduction reads: "This document defines > the MRT Lowpoint algorithm to be used as a standard in the default MRT > profile for MRT-FRR." Which one is it? > [Alia] I think that the algorithm draft needs to be standards track. I apologize for not updating the intended status - I was focused on the contents. IMHO, there's a significant difference in saying "this is an algorithm > than can be used" (informational) vs "this is the algorithm that MUST be > used" (standard). For the record, I have no issues with adopting this > draft with an intended status of Informational. > [Alia] The intent is to have signaling that says this is the algorithm being used. There's capabilities to indicate different algorithms - but I do think that this particular algorithm and details must be standards-track for interoperability. Thanks! > > Alvaro. > > > Major Issues: > > 1. The draft has an intended status of Informational. However, the > Introduction reads: "This document defines the MRT Lowpoint algorithm > to be used as a standard in the default MRT profile for MRT-FRR." I > would like to see a clarification on what the intended status is. > > [Alia] Agreed - it should be Standards Track and I'll update the next version. > > 1. I found it interesting that, even though the document is defining > an algorithm that when implemented will need to have consistency across > nodes, there was no reference to RFC2119 — but some of the words were > capitalized anyway. Even then, only 4 "MUST" and 1 "SHOULD" appeared. I > realize that the count of MUST/SHOULD is no indication of anything, but it > doesn't feel right for a 40+ page document. I would appreciate if the > authors would not just add the 2119 reference (nit), but if they would also > comb the text for parts where a little more normative direction might help > (which doesn't necessarily translate into MUSTs). > > [Alia] Agreed - in general, what I intend to do is to have the explanatory text that explains the logic of the algorithm and how it works much as it is - and then to pull together the different parts of the pseudo-code into a fully detailed algorithm (some by reference to the earlier parts) that indicates the MUST/SHOULD aspects. The specific request though seems more to be to take a careful read through and clarify what is MUST and what is SHOULD. > > 1. For example (all the text is from Section 4 (Algorithm Sections)): > - "The different parts of this algorithm are described below. > These work on a network graph after, for instance, its interfaces > are ordered as per Figure 14." It sounds as if Figure 14 is optional or > maybe ordering the interfaces is not mandatory.. Later it is > clarified: > "To ensure consistency in computation, all routers MUST order interfaces > identically. . . The required ordering . . . Is given in Figure 14." > But > the first read sounds not normative enough to me. > > [Alia] Sure - I can clarify that one. > > 1. > - Figure 14 is not deterministic. The text says that the order > doesn't matter in some cases, but then offers an example of a potential > tie-breaker. > > [Alia] Yes - which parallel link is used doesn't matter. That is a point to clarify - probably by breaking the function into the normative and optional parts. It's useful to have an interface-ordering function that always returns a result for use in data-structures and calling functions. > > 1. > - Root Selection. No algorithm is provided. There's a reference to > I-D.atlas-ospf-mrt, where a suggestion is made ("..the router with the > highest GADAG Root Selection Priority is picked to be the GADAG Root"). > IMHO, the algorithm should be specified in this draft, where the > requirement to carry the Priority is defined so that the extensions > draft(s) can show how to implement it in OSPF (or any other > protocol)..not > the other way around. > > [Alia] The Root Selection is described in draft-ietf-rtgwg-mrt-frr-architecture in Sec 7 as '"GADAG Root Selection Priority: Among the routers in the MRT Island and with the highest priority advertised, an implementation MUST pick the router with the highest Router ID to be the GADAG root." > > 1. > - "Initially, all interfaces must be initialized to UNDIRECTED." > Should that be "MUST"? > - "It is possible that some links and nodes will be marked as > unusable.." Same comment as the one above for the root selection: this > draft should define the requirements ("Due to operational or other > issues, > the capability to xxx SHOULD....") so that other docs (like > I-D.atlas-ospf-mrt) can then implement the extensions.. The text right > now > says: "because OSPF can mark links as unusable then we'll do this.." > > [Alia] ?? The desire to be able to mark links as unusable comes out of the manageability draft and basic requirements. The general requirements are defined in the draft-ietf-rtgwg-mrt-frr-architecture - but you are right that this is only indirectly mentioned by " For any MRT profile, the MRT Island is created by starting from the computing router. If the computing router supports the default MRT profile, add it to the MRT Island. Add a router to the MRT Island if the router supports the default MRT profile and is connected to th MRT Island via bidirectional links eligible for MRT." > > 1. > - "There are two methods to find ears; both are required." > REQUIRED? > > [Alia] Yes - both are part of the algorithm. In explanatory text, I don't think it needs normative language. That can/should be given with a reference to the pseudo-code. > > 1. Algorithm Alternatives and Evaluation. There are a couple of > alternatives described in the appendices, but there is no evaluation as to > why the Lowpoint Algorithm is better. In fact, the text seems to read as > if the appendices describe options to the main algorithm (?) -- begging the > question, are there instances where these options would/could be used? > > [Alia] True - in our initial evaluations, we didn't find significant differences in the path lengths for the alternates. Chris Bowers can comment on that a bit. I thought that I'd cleaned up the text in the main draft to clarify that the lowpoint algorithm - which has much less computation - is preferable. Can you point to where the algorithms in the appendices are considered equivalent? I will, of course, take another read through as well - trying to reduce that issue. Minor Issues: > > 1. Some important terms are not properly defined, or at least not > defined in Section 2. > - "cut-edge" The term is not defined in Section 2. Later in the > document it is described as "The algorithm identifies cut-edges simply > as > links where both ends of the link are cut-vertices.", which is the > definition for cut-link. It looks like cut-edges is used more through > out. > > [Alia] cut-edge and cut-link are the same thing. If we've got inconsistent terminology, I'll fix it. Vertex/edge come from graph theory while we talk about nodes and links. > > 1. > - "MRT Island" I know it is defined in the architecture draft. It > would be nice to carry the definition over to Section 2. > > [Alia] OK > > 1. > - "UNDIRECTED/OUTGOING/INCOMING" These are used in the algorithm > in various points, but are not really defined as to > > [Alia] umm, how would you define them beyond their obvious meaning? Outgoing is from the interface's router to the remote end. Incoming is from the remote router to the interface's router. > > 1. > 2. I think that the algorithm in Figure 6 is not right — of course, I > may be misunderstanding it. When talking about the endpoints of the ear (X > and Y) it says "if Y is root"..but a couple of paragraphs above the text > reads: "Once a partial ADAG is already present, we can always add ears to > it: just select a non-ready neighbor N of a ready node Q, such that Q > is not the root.." It then sounds like the starting point can't be the > root.. > > [Alia] I believe this is a misunderstanding. The description of how an ear can be added is basically a proof that one can always add a new node into the ADAG (in a 2-connected graph) by starting from a neighbor N of a node Q that isn't the root. Then an SPF with Q removed can be done to find a path from Q to the root. The reason that Q can't be the root is just b/c Q is removed from the graph. Figure 6 accurately describes the high-level algorithm. An ear will have 2 ends. If the ear were found via an SPF to the root R and didn't intersect any other nodes already in the ADAG, then that ear would have two ends that are Q and R. > > 1. The explanation of the motivation of the low-point values (after > Figure 9) needs either better examples or a better explanation — or maybe I > got lost. You first start explaining about when 'L(c) >= D(x)', but the > example mentions the 'L(x) < D(x)' case. An example for the second reason > would be nice. > > [Alia] I am not yet sure where you got lost or how I could thus improve the explanation. There are two points that we tried to make. First, if the L(c) >= D(x) then the child c (and its descendents) have no paths to an ancestor of the parent x except via that parent x. Thus x is a cut-vertex. The point with L(c) < D(x) (typo of L(x) ) is to describe a special case where x is a cut-vertex and has multiple DFS children which are in different blocks. As show in the example, some of those children of a node C - such as D in the example - will have L(D) < D(C) ( 0 < 3) while other children - such as F in the example will have L(F) >= D(C) ( 3 >= 3). SO - to determine if a node is a cut-vertex, all of that node's DFS-children must be checked. [Alia] The second point is that by tracing where each node inherited its lowpoint value from, a path to attach to a node above the DFS parent can be found. This is critical - since this lowpoint inheritance path is used to find the ears for DFS children in the algorithm. It's a fast way to find a diverse path from the DFS tree. > > 1. Algorithm Evaluation. While it is interesting to see MRT compared > to rLFA, it would be much more interesting to see it compared to the > options in the appendices or to ARC. I would move the comparison to rLFA > to an appendix. > > [Alia] It is being compared to other options that are being seriously used for fast-reroute. That includes rLFA. I don't understand why you think the comparison isn't relevant? We didn't quite have time to get the comparison fully done for MRT lowpoint compared to the SPF-based and hybrid versions. I think that is something we can add - but again, we didn't find a significant impvorement. > > > > Nits: > > 1. Section 2. "standard pseudo-node representation" Reference? > > [Alia] Do you just want me to refer to OSPFv2? I can go hunt a reference for better normative explanation or just describe transforming all broadcast interfaces into a pseudo-node with p2p links with appropriate costs. > > 1. Section 2; in the definition of MRT-Red and MRT-Blue: > s/described/describe > 2. Section 2; ADAG definition: "whith" ?? > 3. "strictly greater/lower" These terms are used in a couple of > places, but the << or >> notation is used most of the time. It would be > nice to be consistent to avoid confusion. > 4. Section 3.2: "If in the partial order where an ear's two ends are X > and Y, X << Y, then there must already be a directed path from X to Y > already in the ADAG." Too many "already". > 5. It would be nice to include "partial ADAG" in the definitions > section. Yes, it is defined elsewhere, but a central place is nice in > case you forget. ;-) > > [Alia] Sure to all the above. > > 1. I know sometimes it's hard, but please try to format the text so > that the figures are not split between pages. > > [Alia] Do you know a magic xml2rfc method for doing that? > > 1. Section 3.4, after Figure 10. "This observation means that if we > want to find a pair of MRTs rooted at R, then we need to build up a > pair of RTs in block 3 with K as a root. Similarly, we need to find > another one in block 2 with C as a root, and finally, we need the last > one in block 1 with R as a root." When talking about blocks 1 and 2, you > really mean a pair to RTs (just like in block 3), not just "one", right? > > [Alia] Yes, probably rephrasing as "another pair of RTs" and "last pair of RTs" would help. > > 1. Section 3.5: "The method for local-root computation is used in the > MRT Lowpoint algorithm for computing a GADAG using Low-Point > inheritance and the essence of it is given in Figure 12." s/is > used/used and s/inheritance and/inheritance, and To improve readability. > 2. There are a couple of out of date references. > > [Alia] Sure - will fix. > > _______________________________________________ > rtgwg mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/rtgwg > >
_______________________________________________ rtgwg mailing list [email protected] https://www.ietf.org/mailman/listinfo/rtgwg
