Last Call Review of draft-ietf-mpls-tp-shared-ring-protection-04
review-ietf-mpls-tp-shared-ring-protection-04-rtgdir-lc-chen-2017-02-27-00

Request Review of draft-ietf-mpls-tp-shared-ring-protection
Requested rev. no specific revision (document currently at 06)
Type Last Call Review
Team Routing Area Directorate (rtgdir)
Deadline 2017-02-23
Requested 2017-02-09
Requested by Min Ye
Other Reviews Secdir Last Call review of -05 by Paul Hoffman (diff)
Genart Last Call review of -05 by Christer Holmberg (diff)
Review State Completed
Reviewer Mach Chen
Review review-ietf-mpls-tp-shared-ring-protection-04-rtgdir-lc-chen-2017-02-27
Posted at https://mailarchive.ietf.org/arch/msg/rtg-dir/2DQ5r0LFSrGprpJjBmj-nnQ8lYg
Reviewed rev. 04 (document currently at 06)
Review result Has Nits
Last updated 2017-02-27

Review
review-ietf-mpls-tp-shared-ring-protection-04-rtgdir-lc-chen-2017-02-27

Hello, 

I have been selected as the Routing Directorate reviewer for this draft. The Routing Directorate seeks to review all routing or routing-related drafts as they pass through IETF last call and IESG review, and sometimes on special request. The purpose of the review is to provide assistance to the Routing ADs. For more information about the Routing Directorate, please see ​http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir 

Although these comments are primarily for the use of the Routing ADs, it would be helpful if you could consider them along with any other IETF Last Call comments that you receive, and strive to resolve them through discussion or by updating the draft. 

Document: draft-ietf-mpls-tp-shared-ring-protection-04.txt 
 Reviewer: Mach Chen 
 Review Date: 2017/2/23 
 IETF LC End Date: 
 Intended Status: Standards Track

Summary: 
I have some minor concerns about this document that I think should be resolved before publication. 

Comments: 
This document may need some paragraph restructuration or adjustment to make it more readable, for example , before jumping into the detail of some processes, it's better to give some introduction to the scenarios and relevant conditions, and the used terms need to be defined before being used. 

Major Issues: 
No major issues found.

Minor Issues: 

1.
The implicit fundamental pre-condition of the proposed MSRP is that, for each protected LSP, the egress and the  ingress LSR have to negotiate (or through NMS, or static configurations) the LSP label that will be pushed by the ingress. The label has to be a label that the egress must know how to handle it, otherwise, when the packet exits the ring tunnel, the packet will be mis-forwarded to non-intended node or dropped. This condition and probably the relevant mechanisms  should be explicitly pointed out and described in the document. 

2. 
Section 4.1
The Figure of "the logical layers of the ring" shows the innermost service is PW, is this document only applies to PW or other applications will be included. If it's later, some modifications are needed, and you need to check the whole document to make the relevant descriptions and usages consistent. 
Figure 2 has the same problem. 

3.
Section 4.1.3
s/The clockwise working ring tunnel label RcW_D/ The clockwise working ring tunnel label of RcW_D
And since RcW_D identifies a ring tunnel, not a label, and the author seems intend to use RcW_D(X) to identify a ring tunnel label, I'd suggest to add some clarification text to this notation.

4. 
Section 4.4.1
Suggest to move Figure 11 to under "Bullet  1.  Single-node interconnected rings."

5. 
Section 4.4.4, the penultimate paragraph
"...LSP1->R1cW_F&A(D)->R1aP_F&A(D->C->B->A)->R2cW_I(A->F->G->H->I)->LSP1.", what's reason to add "R1cW_F&A(D)" into the switching path? Seems it's redundant.

6.
Section 4.4.3,
In my understanding, the ring tunnels to virtual interconnected group are shared by all LSPs that needed to be forwarded to other rings, right? If so, it's better to add some text to explicitly state this.


7. 
Section 5.1
"   When the failure has cleared and the Wait-to-Restore (WTR) timer has
   expired, the nodes sourcing RPS requests MUST drop their respective
   switches (tail end) and MUST source an RPS request carrying the NR
   code.  The node receiving from both directions such an RPS request
   (head end) MUST drop its protection switches."

Above text introduces the "tail end" and "Head end" tems, but the document does not have detail definition and introduction to these terms. Suggest to give the definition of these term in the terminology and notation section.

8. 
Section 5.1
"A destination node is a node that is adjacent to a node that
   identified a failed link."

Based on the above text, the "destination node" cannot be uniquely determined, since a node in a ring has two adjacent nodes. Some clarification or more accurate description is needed.

9. 
Section 5.1.2
s/sane ring/same ring

10.
Section 5.1.3.  Ring Node RPS States

Are those RPS states per node or per ring tunnel or per egress?  Whatever for either case, the document should state it explicitly.

And according to the explanation of each state, seems that the states are mutex each other. Considering the steering mode, seems that a node may be at both switching and pass-through state, right?

BTW, for those RPS codes, it's better to add a reference to the section (Section 5.2.1.1) to give a hint to the reader that there will be detailed explanation in the following sections.

11. 
Section 5.1.3.2

"A node in the switching state MUST terminate RPS requests flow in
   both directions."

If a node in the switching state terminates RPS requests, how does the steering switching mode work?

12. 
Section 5.1.  RPS Protocol
"The described RPS protocol uses the short-
   wrapping mechanism described in section 4.3.2 as an example."
Are there any differences when the RPS protocol used for wrapping and steering, if no, it should be stated clearly. Otherwise, it needs to specify the differences, or for each switching mode, define its procedures and state machine.

13.
Section 5.1.3.2.  Switching State,

The third paragraph describes the rules in case of unidirectional failure, but reader can not recognized this only when read the next paragraph. This section needs some restructure to make it more readable. BTW, since there are differences between unidirectional failure and bidirectional failure, there need some text to describe the rules and procedures for the bidirectional case IMHO

14.
Section 5.2.2.  Initial States
a)
It just lists a table and let the reader to guess the meaning, it's better to add some clarification text to help the reader understand the meaning and intention.

b)
In addition:

           |  B  |  Pass-through               |  N/A           |
            |     |   Working: no switch        |                |
            |     |   Protection: pass through  |                |
For pass-through state, why the working and protection tunnel states are different?

c)
|  D  |  Idle - LW                  |  NR            | 

What is the LW?  I guess it means "Lock of Working", but it needs to be expanded it when first use. 

Best regards,
Mach