Re: [mpls] Routing directorate review of draft-ietf-mpls-lsp-ping-lag-multipath-03

[Mach Chen <[e-mail]>]

Hi Jon,

Thanks for the detailed review and useful comments!

Please see some responses inline...

> From: Jonathan Hardwick [mailto:[e-mail]]
> Sent: Tuesday, May 22, 2018 3:39 AM
> To: [e-mail]; [e-mail]; mpls-
> [e-mail]
> Cc: [e-mail]
> Subject: Routing directorate review of draft-ietf-mpls-lsp-ping-lag-multipath-03
> 
> Hello
> 
> I have been selected to do a routing directorate “early” review of this draft.
> https://datatracker.ietf.org/doc/draft-ietf-mpls-lsp-ping-lag-multipath/
> 
> The routing directorate will, on request from the working group chair, perform
> an “early” review of a draft before it is submitted for publication to the
> IESG.  The early review can be performed at any time during the draft’s
> lifetime as a working group document.  The purpose of the early review
> depends on the stage that the document has reached.  As this document is close
> to working group last call, my focus for the review was to determine whether
> the document is ready to be published.  Please consider my comments along
> with the other working group last call comments.
> 
> For more information about the Routing Directorate, please see 
> http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir
> 
> Best regards
> Jon
> 
> 
> Document: draft-ietf-mpls-lsp-ping-lag-multipath-03.txt
> Reviewer: Jonathan Hardwick
> Review Date: 21 May 2018
> Intended Status: Standards Track
> 
> Summary
> This document looks ready for working group last call.  I have a few minor
> issues that I am sure can be resolved during the last call.
> 
> 
> Section 2
> First paragraph: the reference to section 3.3 of [RFC8029] looks wrong.  Should
> it be a reference to section 4?

It was intended to refer to Section 3.3 RFC4079 (Downstream Mapping). 

How about the following text:
"Reader is expected to be familiar with mechanics of Downstream Mapping described in Section 3.3 of RFC8029 and Downstream Detailed Mapping TLV (DDMAP) described in Section 3.4 of RFC8029."


> 
> Section 3
> “When the responder LSR receives an MPLS echo reply message” <- you mean
> “MPLS echo request message”.

Yes.

> 
> Section 5.1
> This is fine, but I found it a bit cumbersome to read.  How about this rewording?
> NEW
>   If the downstream LSR does not return Remote Interface Index sub-TLVs in
>   the DDMAP, then the initiator LSR validates LAG member link traversal by
>   traversing all available LAG member links and then using the procedure
> described
>   below.  This section provides the mechanism for the initiator LSR to obtain
> additional information from the downstream LSRs and describes the additional
> logic in the initiator LSR to validate the L2 ECMP traversal.
> END

This looks good to me, thanks for the new text!

> 
> Section 5.1.3
> For my interest, why are you using “entropy” here?  It sounds like you mean
> “probability”, but I might have misunderstood your meaning.

The "entropy" is used to select specific LAG member link, it has the similar concept as "entropy label". 

> 
> Top of page 13:
>    The initiator LSR sends two MPLS echo request messages to traverse
>    the two LAG members at TTL=1:
> “TTL=1” should be “TTL=n”.

Good catch, fixed.

> 
> Section 6
> Typo “in the in the”

Fixed.

> 
> Section 8 and 9
> This draft only discusses using the new Local & Remote Interface Index Sub-
> TLVs in the context of a DDMAP for a LAG interface, so I was surprised to read
> that it is permissible to set M=0 in these TLVs.  You should describe how the
> TLV is used in that case, if you are going to allow it.
> Does the M flag need to be set consistently in all Local & Remote Interface
> Index Sub-TLVs  in a given DDMAP TLV?
> In fact, isn’t the M flag redundant, given that the enclosing DDMAP has the
> "LAG Description Indicator flag"?

Indeed, seems redundant, I will do double check on it. 

> 
> Section 10
> Why do you need the Sub-TLV length field?  It can be inferred from the TLV
> length and the address type.

Indeed, and I personally agree, I will talk to the co-authors, if there is no further reasons, will remove the sub-TLV length field.

> Section 10.1.1 – if the LSR received no labels (e.g. PHP case) then should it omit
> this sub-TLV, or include an empty sub-TLV?

The sub-TLV is derived from Label Stack Sub-TLV defined in 8029, it has the same usage as Label Stack Sub-TLV. So, for that case, the sub-TLV should be included and an Implicit Null label returned. 

> 
> Other nits
> Throughout, English grammar needs to be fine-tuned e.g. there are definite
> and indefinite articles missing.  However, I found the document perfectly
> readable, so perhaps this can be left for the RFC editor.

Sure, thanks.

Best regards,
Mach