Subject: [mpls] MPLS-RT review of draft-akiya-mpls-lsp-ping-lag-multipath

From: Mach Chen <[email-masked]>
Date: Thu, 11 Dec 2014 09:58:12 +0000

Hi Authors,

I have finished my MPLS-RT review of draft-akiya-mpls-lsp-ping-lag-multipath, here are my comments, hope they are help.

1. 
This is a useful draft that tries to address a real network problem. The solution part works but may need some optimization. 

2.
The draft uses the LAG Interface Info TLV to implicitly notify the responders to return the LAG member information. IMHO, the G bit is more appropriate for the purpose, and then there is no need to carry the LAG Interface Info TLV in the echo request. 

3.
According to the definition of LAG Interface Info TLV, it is used to return the LAG process capable of the responder LSR (not specific LAG interface). Actually, it can be generalized as an LSR capability TLV that is not only limited to LAG capability, it may be used for future extension.

4.
Section 3 and Section 4 have large amount of overlap context. I think Section 3 is actually a subset (just for discovery) of Section 4. I'd suggest to merge the two sections. 

5.
Section 3.2:

  0                   1                   2                   3
      0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |   DDMAP fields describing LAG interface with DS Flags G set   |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
I got confused when I firstly see this, suggest to use the whole DDMAP structure or just show the sub-TLV part of DDMAP. If you really want to use this style, it's better to replace "|" with "~".

There is the same issue for Figure 3.

6.
Section 4.3:
   o  Error case:
      *  Traversing LAG member=1 at TTL=n results in LAG member=1' as
         the incoming interface at TTL=n+1.
      *  Traversing LAG member=2 at TTL=n results in LAG member=1' as
         the incoming interface at TTL=n+1.

For the first bullet, I guess you intend to say:
      *  Traversing LAG member=1 at TTL=n results in LAG member=2' as
         the incoming interface at TTL=n+1.

BTW, why the above cases are error cases?  If 1 (or 2) and 1' (or 2') are supposed to be the two ends of a member link. If a packet traversing 1 at TTL=n, how will the packet come out of other interfaces other than 1'? 

I guess you intend to say a packet is expected to be traversed 1 at TTL=n, but mis-forwarded to other link, then result in the packet comes out of the non-desired link at TTL=n+1. Right?

Best regards,
Mach

---

From: "Nobo Akiya (nobo)" <[email-masked]>
Date: Sun, 14 Dec 2014 02:28:52 +0000

Hi Mach,

Many thanks for reviewing this document and providing great comments. I particularly liked your generic LSR Capability TLV suggestion :)

Please see in-line for [NOBO].

> -----Original Message-----
> From: mpls [mailto:[email-masked]] On Behalf Of Mach Chen
> Sent: Thursday, December 11, 2014 4:58 AM
> To: [email-masked]
> Cc: mpls; [email-masked]
> Subject: [mpls] MPLS-RT review of draft-akiya-mpls-lsp-ping-lag-multipath
> 
> Hi Authors,
> 
> I have finished my MPLS-RT review of draft-akiya-mpls-lsp-ping-lag-
> multipath, here are my comments, hope they are help.
> 
> 1.
> This is a useful draft that tries to address a real network problem. The
> solution part works but may need some optimization.
> 
> 2.
> The draft uses the LAG Interface Info TLV to implicitly notify the responders
> to return the LAG member information. IMHO, the G bit is more appropriate
> for the purpose, and then there is no need to carry the LAG Interface Info
> TLV in the echo request.

[NOBO] Right, I see what you mean there. The presence of the G bit will be similar to the I bit in the DS Flags then, a flag to request a specific behavior on the responder and to request inclusion of a specific TLV. It's slightly overloading (i.e., the flag will have different meaning in MPLS echo request/reply messages) but I think this optimization is reasonable. I will make this change in the next revision.

> 
> 3.
> According to the definition of LAG Interface Info TLV, it is used to return the
> LAG process capable of the responder LSR (not specific LAG interface).
> Actually, it can be generalized as an LSR capability TLV that is not only
> limited to LAG capability, it may be used for future extension.

[NOBO] This is a good idea Mach. I will rename this TLV to "LSR Capability TLV" and expand the flags field from 16 bits to 32 bits. Although when combining your comment (2) and (3), there's a bit of a conflict. The LSR Capability TLV should be a generic TLV that initiator LSR includes in the MPLS echo request when wanting to query the capability of the responder LSR, not something that the responder LSR includes automatically into the MPLS echo reply. Thus, I have changed the document such that:

a. Defines a generic LSR Capability TLV.
b. Recommends the initiator LSR to include the LSR Capability TLV in the MPLS echo request when using the LAG multipath mechanism.
c. If MPLS echo request contains LSR capability TLV, then the responder TLV includes the LSR Capability TLV in the MPLS echo reply, with appropriate bits set.

> 
> 4.
> Section 3 and Section 4 have large amount of overlap context. I think Section
> 3 is actually a subset (just for discovery) of Section 4. I'd suggest to merge
> the two sections.

[NOBO] I'd like to keep the discovery (Section 3) and validation (Section 4) to be separate sections. I am pretty sure that there'll be implementations that can easily implement the discovery but will take longer time to implement the validation aspect ... which is why capability U/D bits also exists.

> I got confused when I firstly see this, suggest to use the whole DDMAP
> structure or just show the sub-TLV part of DDMAP. If you really want to use
> this style, it's better to replace "|" with "~".
> 

[NOBO] You are right. Let me go with "~".

>          the incoming interface at TTL=n+1.

[NOBO] Eric also suggested to clean up the success/error cases. I think they will be much cleaner in the next revision. Stay tuned.

> 
> BTW, why the above cases are error cases?  If 1 (or 2) and 1' (or 2') are
> supposed to be the two ends of a member link. If a packet traversing 1 at
> TTL=n, how will the packet come out of other interfaces other than 1'?

[NOBO] It can happen the hashing logic is broken or hash buckets are corrupted and pointing to unexpected LAG member.

> 
> I guess you intend to say a packet is expected to be traversed 1 at TTL=n, but
> mis-forwarded to other link, then result in the packet comes out of the non-
> desired link at TTL=n+1. Right?

[NOBO] Exactly!

Thanks!

-Nobo

> 
> 
> Best regards,
> Mach
> 
> _______________________________________________
> mpls mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/mpls

---

From: Mach Chen <[email-masked]>
Date: Mon, 15 Dec 2014 03:38:09 +0000

Hi Nobo,

Thanks for considering my comments! And I am OK with your proposals to my comments.

Best regards,
Mach

> From: Nobo Akiya (nobo) [mailto:[email-masked]]
> Sent: Sunday, December 14, 2014 10:29 AM
> To: Mach Chen; [email-masked]
> Subject: RE: MPLS-RT review of draft-akiya-mpls-lsp-ping-lag-multipath
> 
> Hi Mach,
> 
> Many thanks for reviewing this document and providing great comments. I
> particularly liked your generic LSR Capability TLV suggestion :)
> 
> > This is a useful draft that tries to address a real network problem.
> > The solution part works but may need some optimization.
> >
> > 2.
> > The draft uses the LAG Interface Info TLV to implicitly notify the
> > responders to return the LAG member information. IMHO, the G bit is
> > more appropriate for the purpose, and then there is no need to carry
> > the LAG Interface Info TLV in the echo request.
> 
> [NOBO] Right, I see what you mean there. The presence of the G bit will be
> similar to the I bit in the DS Flags then, a flag to request a specific behavior on the
> responder and to request inclusion of a specific TLV. It's slightly overloading (i.e.,
> the flag will have different meaning in MPLS echo request/reply messages) but I
> think this optimization is reasonable. I will make this change in the next revision.
> 
> >
> > 3.
> > According to the definition of LAG Interface Info TLV, it is used to
> > return the LAG process capable of the responder LSR (not specific LAG
> interface).
> > Actually, it can be generalized as an LSR capability TLV that is not
> > only limited to LAG capability, it may be used for future extension.
> 
> [NOBO] This is a good idea Mach. I will rename this TLV to "LSR Capability TLV" and
> expand the flags field from 16 bits to 32 bits. Although when combining your
> comment (2) and (3), there's a bit of a conflict. The LSR Capability TLV should be a
> generic TLV that initiator LSR includes in the MPLS echo request when wanting to
> query the capability of the responder LSR, not something that the responder LSR
> includes automatically into the MPLS echo reply. Thus, I have changed the
> document such that:
> 
> b. Recommends the initiator LSR to include the LSR Capability TLV in the MPLS
> echo request when using the LAG multipath mechanism.
> c. If MPLS echo request contains LSR capability TLV, then the responder TLV
> includes the LSR Capability TLV in the MPLS echo reply, with appropriate bits set.
> 
> >
> > 4.
> > 3 is actually a subset (just for discovery) of Section 4. I'd suggest
> > to merge the two sections.
> 
> [NOBO] I'd like to keep the discovery (Section 3) and validation (Section 4) to be
> separate sections. I am pretty sure that there'll be implementations that can
> easily implement the discovery but will take longer time to implement the
> validation aspect ... which is why capability U/D bits also exists.
> 
> >
> > 5.
> > Section 3.2:
> >
> >   0                   1                   2                   3
> > structure or just show the sub-TLV part of DDMAP. If you really want
> > to use this style, it's better to replace "|" with "~".
> >
> [NOBO] Eric also suggested to clean up the success/error cases. I think they will
> be much cleaner in the next revision. Stay tuned.
> 
> >
> > BTW, why the above cases are error cases?  If 1 (or 2) and 1' (or 2')
> > are supposed to be the two ends of a member link. If a packet
> > traversing 1 at TTL=n, how will the packet come out of other interfaces other
> than 1'?
> 
> > I guess you intend to say a packet is expected to be traversed 1 at
> > TTL=n, but mis-forwarded to other link, then result in the packet
> > comes out of the non- desired link at TTL=n+1. Right?
> 
> [NOBO] Exactly!
> 
> Thanks!
> 
> -Nobo
> 
> >
> >
> > Best regards,
> > Mach
> >