Telechat Review of draft-ietf-mpls-mldp-node-protection-06
review-ietf-mpls-mldp-node-protection-06-genart-telechat-davies-2015-09-21-00

Request Review of draft-ietf-mpls-mldp-node-protection
Requested rev. no specific revision (document currently at 08)
Type Telechat Review
Team General Area Review Team (Gen-ART) (genart)
Deadline 2015-09-15
Requested 2015-09-11
Draft last updated 2015-09-21
Completed reviews Genart Last Call review of -05 by Elwyn Davies (diff)
Genart Telechat review of -06 by Elwyn Davies (diff)
Secdir Last Call review of -05 by Shaun Cooley (diff)
Assignment Reviewer Elwyn Davies
State Completed
Review review-ietf-mpls-mldp-node-protection-06-genart-telechat-davies-2015-09-21
Reviewed rev. 06 (document currently at 08)
Review result Ready
Review completed: 2015-09-21

Review
review-ietf-mpls-mldp-node-protection-06-genart-telechat-davies-2015-09-21

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 wait for direction from your
document shepherd or AD before posting a new version of the draft.

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-06.txt
Reviewer: Elwyn Davies
Review Date:  2015/09/15
IETF LC End Date: 2015/09/08
IESG Telechat date: 2015/09/17



Summary: Ready for publication on standards track.  Thanks for your 


generous comments on my review and the updated version -06 which fixes 


almost all of the issues.  The nits below are mostly suggestions related 


to the updated text apart from the last one on s2.3 which got missed.




Major issues:
None

Minor issues:
None

Nits/editorial comments:


s1:  The new text referring to the need for capability negotiation is 


not easy to parse.   Suggested alternative:



OLD:
   In order for a node to be protected, the protecterd node, the PLR and
   the MPT MUST support the procedures as described in this draft.
   Detecting the protected node, PLR and MPT support these procedures is
   done using [RFC5561].
NEW:


   In order to allow a node to be protected against failure, the LSRs 


providing



   the PLR and the MPT functionality as well as the protected node MUST
   support the functionality described in this document.  RSVP capability


   negotiation [RFC5561] is used to signal the availability of the 


functionality



   between the participating nodes; these nodes MUST support capability
   negotiation.
END

s2, last para: s/This because/This is because/



s2.1, last para; s2.2, last para: s/Procedures how to setup/The 


procedures for setting up/






s2, s2.2 and s3: s/this draft/this document/ (3 places) [A 4th instance 


is replaced in the suggested text for s1 above.]







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.



The fixes for that are fine and helpful IMO.



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.



Probably right.  Actually the fact that the bypass LSPs are 


bidirectional does sort out the differentiation of roles anyway. 


Incoming = MPT, Outgoing = PLR.  The note could be extended to mention 


this.




s2.3:



       Num PLR entry: Element as an unsigned, ***non-zero*** integer followed
       by that number of "PLR entry" fields in the format specified
       below.



Per the discussion of my last call comments, the Num PLR entry can be zero.