Re: [Gen-art] Gen-art LC review of draft-ietf-mpls-mldp-node-protection-05
Hi Elwyn, First of all, thanks for the great comments. See a few responses inline below. I’ll submit the new version now, hope it addresses all your comments. > In particular, there doesn't seem to be any explanation of why the PLRs and > MPTs have to be directly connected to the protected node (RFC 6388 is happy > to find non-directly connected nodes) and I cannot see why direct connection > is vital to this specification. This may of course because I am not an MPLS > afficionado! There are, however, a considerable number of language Ice: mLDP consults the RIB in order to find the next LDP neighbor to reach the Root node. The RIB only has information about the directly connected nodes and does not have visibility beyond that. If mLDP would consult the IGP (OSPF/ISIS) database I might be able to figure that out, but that is outside the scope of the document. I’ll add a note to make that clear. > nits which I have documented - another editorial pass by a reviewer with > English mother tongue after fixing the nits I found would probably help. > > Major issues: > None. > > Minor issues: > s1: This specification uses LDP capability negotiation (RFC 5561). The > introduction should be clear that nodes involved in the node protection > scheme MUST support RFC 5561 negotiations. Ice: Fixed. > > s1, para 2: There is a very dangerous slang phrase here: "most ... should > just work"! Does this have the literal meaning that there are some > circumstances in which they might NOT work and/or there are other > capabilities that aren't enabled - if so, what are they? Or, I guess more > likely, the slang meaning that using this method will allow these > capabilities to work without additional effort. Please use a non-slang > phrase and be more precise as to what will/won't work. Ice: Fixed. > > s2.1: >>The upstream LSR in figure 1 is LSR1 >>because it is the ***first hop*** along the shortest path to reach the >> root >>address. >> > Does it have to be the 'first hop' (I read this as the first LSR on the path > from N towards the root)? > RFC 6388 s2.4.1.1 allows the upstream to be any LSR along the path according > to some local selection policy. Can't the PLR be any such upstream LSR? Ice: Added text to explain this. > > s2.2: Following on from the previous comment, do LSR1..3 *have* to be > 'directly connected'? Can they not be just one from the set of LSRs along > each separate LSR emanating from the root? Ice: It may be possible to support this, but we need more information than we can currently find in the RIB. It will significantly complicate the solution. I’ve made this outside the scope of the document as at this point I don’t think we need that support. > > s2.2: If I understand correctly, the bypass LSPs have to be bidirectional > (or they could be two unidirectional ones) unlike those in s2.1 which will be > unidirectional. I think this ought to be mentioned, assuming I am right - > and presumably one could do a bit of optimisation in setup. This has some > knock-on effects as regards what happens when the node fails. I wonder if > there should be some explanation of what happens in an extra sub-section in > s4 - just that the various LSRs need to think about what role they are > playing depending on where the incoming packets are coming from, I guess. Ice: Yes, that is a good observation about unidirectional and bidirectional LSPs. I’ll add a node to make that clear. Since the MPT will receive packets with the MPLS label it originally expected, it does not really care where the packets are coming from. So I’m not sure anything else needs to be added here. > > s2.3, next to last para: >> To remove all PLR addresses belonging to >>the encoded Address Family, an LSR N MUST encode PLR Status Value >>Element with no PLR entry and "Num PLR entry" field MUST be set to >>zero. >> > This is inconsistent with the statement in the "PLR Status Value Element" > figure: > PLR entry (1 or more) > I guess this last should be 'zero or more’. Ice: Good catch! > > Nits/editorial comments: > == > Abstract: Need to expand LSR on first use. Ice: Fixed. > > Abstract and s1: s/that has been built by/that have been built by the/ (2 > places) Ice: Fixed. > > s1, para 1: Need to expand RSVP-TE and LFA on first use, and give references > for them - probably RFC 5420 for RSVP-TE and RFC 5286 for LDP LFA. Ice: Done. > > s1, para 2: s/Targetted/Targeted/ Ice: Ok. > > s1.2: mLDP probably needs a reference - RFC 6388, I guess Ice: Ok. > > s1, para 2: the concept 'Target(t)ed LDP session' comes from RFC 7060 - > please add a reference. Ice: Ok. > > s2: It might be helpful to mention that the tLDP sessions are established > over the bypass LSPs from PLR to MPT and reiterate the comments in s1 about > how these LSPs might be established to avoid the protected node. Ice: The tLDP
[Gen-art] Gen-art LC review of draft-ietf-mpls-mldp-node-protection-05
I am the assigned Gen-ART reviewer for this draft. The General Area Review Team (Gen-ART) reviews all IETF documents being processed by the IESG for the IETF Chair. Please treat these comments just like any other last call comments. For more information, please see the FAQ at http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq. Document: draft-ietf-mpls-mldp-node-protection-05 Reviewer: Elwyn Davies Review Date: 2015/08/28 IETF LC End Date: 2015/09/08 IESG Telechat date: (if known) - Summary: Almost ready. The distinction that (I believe) is the case, between the need for unidirectional bypass LDPs in the non-root case and bidirectional (or two unidirectional) ones in the root case appears to merit some additional explanation. Otherwise, there are a couple of minor issues that are only just above the level of nits. In particular, there doesn't seem to be any explanation of why the PLRs and MPTs have to be directly connected to the protected node (RFC 6388 is happy to find non-directly connected nodes) and I cannot see why direct connection is vital to this specification. This may of course because I am not an MPLS afficionado! There are, however, a considerable number of language nits which I have documented - another editorial pass by a reviewer with English mother tongue after fixing the nits I found would probably help. Major issues: None. Minor issues: s1: This specification uses LDP capability negotiation (RFC 5561). The introduction should be clear that nodes involved in the node protection scheme MUST support RFC 5561 negotiations. s1, para 2: There is a very dangerous slang phrase here: most ... should just work! Does this have the literal meaning that there are some circumstances in which they might NOT work and/or there are other capabilities that aren't enabled - if so, what are they? Or, I guess more likely, the slang meaning that using this method will allow these capabilities to work without additional effort. Please use a non-slang phrase and be more precise as to what will/won't work. s2.1: The upstream LSR in figure 1 is LSR1 because it is the ***first hop*** along the shortest path to reach the root address. Does it have to be the 'first hop' (I read this as the first LSR on the path from N towards the root)? RFC 6388 s2.4.1.1 allows the upstream to be any LSR along the path according to some local selection policy. Can't the PLR be any such upstream LSR? s2.2: Following on from the previous comment, do LSR1..3 *have* to be 'directly connected'? Can they not be just one from the set of LSRs along each separate LSR emanating from the root? s2.2: If I understand correctly, the bypass LSPs have to be bidirectional (or they could be two unidirectional ones) unlike those in s2.1 which will be unidirectional. I think this ought to be mentioned, assuming I am right - and presumably one could do a bit of optimisation in setup. This has some knock-on effects as regards what happens when the node fails. I wonder if there should be some explanation of what happens in an extra sub-section in s4 - just that the various LSRs need to think about what role they are playing depending on where the incoming packets are coming from, I guess. s2.3, next to last para: To remove all PLR addresses belonging to the encoded Address Family, an LSR N MUST encode PLR Status Value Element with no PLR entry and Num PLR entry field MUST be set to zero. This is inconsistent with the statement in the PLR Status Value Element figure: PLR entry (1 or more) I guess this last should be 'zero or more'. Nits/editorial comments: == Abstract: Need to expand LSR on first use. Abstract and s1: s/that has been built by/that have been built by the/ (2 places) s1, para 1: Need to expand RSVP-TE and LFA on first use, and give references for them - probably RFC 5420 for RSVP-TE and RFC 5286 for LDP LFA. s1, para 2: s/Targetted/Targeted/ s1.2: mLDP probably needs a reference - RFC 6388, I guess s1, para 2: the concept 'Target(t)ed LDP session' comes from RFC 7060 - please add a reference. s2: It might be helpful to mention that the tLDP sessions are established over the bypass LSPs from PLR to MPT and reiterate the comments in s1 about how these LSPs might be established to avoid the protected node. s2: The term 'root node' (presumably from RFC 6388) should be in the terminology section. s2: s/e.g./e.g.,/ s2.1, s2.2, s5: The use of phrases such as 'we are ...ing' is deprecated for RFCs - please rephrase using, for example, 'this document does something'. s2.1, Fig 1 caption: s/to the LSR2/to LSR2/ s2.1: 'which is feature enabled': Although it is probably obvious it would be clearer to say 'which has the node protection feature enabled' s2.1: s/i.e./i.e.,/ s2.1: s/for Capability negotiation/during Capability negotiation/ s2.2, last para: s/don't/doesn't/, s/will
