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

From: Jonathan Hardwick <[email-masked]>
Date: Mon, 21 May 2018 19:39:05 +0000

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?

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

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

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.

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”.

Section 6
Typo “in the in the”

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"?

Section 10
Why do you need the Sub-TLV length field?  It can be inferred from the TLV length and the address type.
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?

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.

---

From: Mach Chen <[email-masked]>
Date: Wed, 23 May 2018 10:03:59 +0000

Hi Jon,

Thanks for the detailed review and useful comments!

Please see some responses inline...

> From: Jonathan Hardwick [mailto:[email-masked]]
> Sent: Tuesday, May 22, 2018 3:39 AM
> To: [email-masked]; [email-masked]; mpls-
> [email-masked]
> Cc: [email-masked]
> Subject: Routing directorate review of draft-ietf-mpls-lsp-ping-lag-multipath-03
> 
> Hello
> 
> 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
> 
> 
> 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

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."

> “MPLS echo request message”.

Yes.

>   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". 

> “TTL=1” should be “TTL=n”.

Good catch, fixed.

> 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.

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

> 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

---

From: Jonathan Hardwick <[email-masked]>
Date: Wed, 23 May 2018 10:13:53 +0000

Thanks Mach.  Your proposed markups all look good to me.
Cheers
Jon

-----Original Message-----
From: Mach Chen [mailto:[email-masked]] 
Sent: 23 May 2018 11:04
To: Jonathan Hardwick <[email-masked]>; [email-masked]; [email-masked]; [email-masked]
Cc: [email-masked]
Subject: RE: Routing directorate review of draft-ietf-mpls-lsp-ping-lag-multipath-03

Hi Jon,

> To: [email-masked]; [email-masked]; 
> mpls- [email-masked]
> Cc: [email-masked]
> Subject: Routing directorate review of 
> draft-ietf-mpls-lsp-ping-lag-multipath-03
> 
> Hello
> 
> 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.
> 
> 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?

Yes.

>   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

> 
> 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.

Good catch, fixed.

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"?

> 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?

> 
> 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

---

From: George Swallow <[email-masked]>
Date: Thu, 24 May 2018 09:13:33 -0400

Mach -

>> “TTL=1” should be “TTL=n”.

At this point in the procedure you have already reached the node at ttl=n.
You are now probing a LAG that extends to the node at TTL=n+1.

S0 the text should be "TTL=n+1".

Thanks,

George

On Wed, May 23, 2018 at 6:03 AM, Mach Chen <[email-masked]> wrote:

> mpls-
> perform
> minor
> > First paragraph: the reference to section 3.3 of [RFC8029] looks wrong.
> Should
> > it be a reference to section 4?
>
> "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
> obtain
> mean
> Sub-
> > In fact, isn’t the M flag redundant, given that the enclosing DDMAP has
> the
> > "LAG Description Indicator flag"?
>
> > 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.
>
> should it omit
> 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
> definite

---

From: Mach Chen <[email-masked]>
Date: Fri, 25 May 2018 01:04:22 +0000

Hi George,

> From: George Swallow [mailto:[email-masked]]
> Sent: Thursday, May 24, 2018 9:14 PM
> To: Mach Chen <[email-masked]>
> Cc: Jonathan Hardwick <[email-masked]>; draft-ietf-mpls-
> [email-masked]; [email-masked]; [email-masked]; rtg-
> [email-masked]
> Subject: Re: Routing directorate review of draft-ietf-mpls-lsp-ping-lag-
> multipath-03
> 
> Mach -
> >>
> >> Top of page 13:
> At this point in the procedure you have already reached the node at ttl=n.  You
> are now probing a LAG that extends to the node at TTL=n+1.

Yes, you are right.

Best regards,
Mach

> > 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.
> 
> 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