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

From: "Osborne, Eric" <[email-masked]>
Date: Thu, 4 Dec 2014 15:55:39 +0000

I forgot to cc the list so I'm resending my comments on this draft.

Here is my review:

- is the document coherent?
	Not really...it needs some serious copyediting.  See the wall of text below.  None of my copyedit comments should be considered blockers for WG adoption.

- is it useful?
	Very much so.  This mechanism will make troubleshooting easier and proactive monitoring much better.

- is it ready for WG adoption?
	Yes.  It needs some cleanup but the technology seems sound.  It's certainly mature enough to be adopted by the WG.

Document comments:

This isn't a line-by-line critique and I'm not a terribly good Real Editor(tm) anyways so I've tried to keep these comments general and constructive.

General: the document needs an infusion of pronouns and definite articles.  Things like "Result raises a limitation" (sec. 1.2) should be "The result raises.." or 'This result raises...'.  I've called out some of the places where this needs to happen, but not all of them.

Section 1.1: s/terminologies/terms/

Section 1.2: the first paragraph needs a rewrite, it's messy.  Also, the bullet points that end in '...is [succeeding|failing]' should be '...have [succeeded|failed]'.  And 'With above scenario' needs to be 'With the above scenario' or something.

Sections 3-5: These sections could use a reorg.  Starting with procedures for using new TLVs before defining the new TLVs feels awkward.  I suggest taking sections [3,4,5] and reordering as [5,3,4].  

Section 3: This section needs a little introduction.  Something like "This section describes the handling of the new TLVs by nodes which understand them.  There are two cases - nodes which understand the TLVs but which for some reason cannot describe LAG members separately, and nodes which both understand the new TLVs and are able to describe LAG members separately".  

Section 3.2: " The responder LSRs that understands the LAG Interface Info TLV and
   are able to describe outgoing LAG member links separately are to use
   the follow procedures" 
needs some singular/plural cleanup:
" A responder LSR that understands the LAG Interface Info TLV and
   is able to describe outgoing LAG member links separately uses
   the follow procedures"
or something like that.

Section 3.3: "Above procedures..." -> "The procedures above..."

Section 4.2: Same kind of cleanup as 3.2: "Responder LSRs that understands.." -> "Responder LSRs which understand..."

Section 4.3: "described procedures in this section" -> "the procedures described in this section"

Section 4.3: "following checks" -> "the following checks".  Also, the error case is a little confusing.  The first half of the error case is actually a success, and I spent some time going back and forth between the first half of the success case and the failure case to figure out whether they were different.  It would benefit from some clarifying text, perhaps something like "An error case is any case where the OAM across a LAG fails, either partial or fully.  A partial failure is where some LAG member traversals behave as they should and some don't, and a full failure is where no LAG member traversals behave as they should".

Sections 5-8: no comments on the TLVs themselves, I don't know OAM well enough to find any hidden mines here.  This is for the WG to pore over.

Section 9: "As result of" -> "as a result of".  Also, "additional processing are required" -> "additional processing is required"

Section 11: "Authors" -> "The authors".

Appendix A: I get that this section is intended to list out problems that aren't solved by the mechanisms in this document.  It's probably worth adding text in Section 2 that describes the limitations of the proposed mechanism, something like "Most[*] LAGs are built from p2p links, and thus router X and router X+1 have the same number of LAG members.  It is possible to build LAGs asymmetrically by using Ethernet switches in the middle.  Appendix A lists some cases which this document does not address; if an operator deploys LAGs in a manner similar to what's shown in Appendix A, the mechanisms in this document may not suit them".  I know Section 2 has a bullet at the end that hints at this, but I think it should be more explicit.

[*]  I believe this to be true.  I am willing to be wrong.  It has happened before.

eric

---

From: "Nobo Akiya (nobo)" <[email-masked]>
Date: Sat, 6 Dec 2014 02:17:18 +0000

Hi Eric,

Many thanks for thorough review of this document and providing constructive comments. We will go through your comments and update the document once other comments from MPLS-RT team come in.

Thanks!

-Nobo

> -----Original Message-----
> From: Osborne, Eric [mailto:[email-masked]]
> Sent: Thursday, December 04, 2014 10:56 AM
> To: [email-masked]; mpls-
> [email-masked]; VIGOUREUX, MARTIN (MARTIN)
> Cc: [email-masked]
> Subject: MPLS-RT review of draft-akiya-mpls-lsp-ping-lag-multipath
> 
> 	Not really...it needs some serious copyediting.  See the wall of text
> below.  None of my copyedit comments should be considered blockers for
> WG adoption.
> 
> - is it useful?
> 	Yes.  It needs some cleanup but the technology seems sound.  It's
> certainly mature enough to be adopted by the WG.
> 
> 
> Document comments:
> 
> This isn't a line-by-line critique and I'm not a terribly good Real Editor(tm)
> anyways so I've tried to keep these comments general and constructive.
> 
> General: the document needs an infusion of pronouns and definite articles.
> Things like "Result raises a limitation" (sec. 1.2) should be "The result
> raises.." or 'This result raises...'.  I've called out some of the places where
> this needs to happen, but not all of them.
> 
> Section 1.2: the first paragraph needs a rewrite, it's messy.  Also, the bullet
> points that end in '...is [succeeding|failing]' should be '...have
> [succeeded|failed]'.  And 'With above scenario' needs to be 'With the above
> scenario' or something.
> 
> Sections 3-5: These sections could use a reorg.  Starting with procedures for
> using new TLVs before defining the new TLVs feels awkward.  I suggest
> taking sections [3,4,5] and reordering as [5,3,4].
> 
> Section 3: This section needs a little introduction.  Something like "This
> section describes the handling of the new TLVs by nodes which understand
> them.  There are two cases - nodes which understand the TLVs but which for
> some reason cannot describe LAG members separately, and nodes which
> both understand the new TLVs and are able to describe LAG members
> separately".
> 
> Section 4.2: Same kind of cleanup as 3.2: "Responder LSRs that
> understands.." -> "Responder LSRs which understand..."
> 
> Section 4.3: "following checks" -> "the following checks".  Also, the error
> case is a little confusing.  The first half of the error case is actually a success,
> and I spent some time going back and forth between the first half of the
> success case and the failure case to figure out whether they were different.
> It would benefit from some clarifying text, perhaps something like "An error
> case is any case where the OAM across a LAG fails, either partial or fully.  A
> partial failure is where some LAG member traversals behave as they should
> and some don't, and a full failure is where no LAG member traversals
> behave as they should".
> 
> Sections 5-8: no comments on the TLVs themselves, I don't know OAM well
> enough to find any hidden mines here.  This is for the WG to pore over.
> 
> Section 9: "As result of" -> "as a result of".  Also, "additional processing are
> required" -> "additional processing is required"
> 
> Appendix A: I get that this section is intended to list out problems that
> aren't solved by the mechanisms in this document.  It's probably worth
> adding text in Section 2 that describes the limitations of the proposed
> mechanism, something like "Most[*] LAGs are built from p2p links, and thus
> router X and router X+1 have the same number of LAG members.  It is
> possible to build LAGs asymmetrically by using Ethernet switches in the
> middle.  Appendix A lists some cases which this document does not
> address; if an operator deploys LAGs in a manner similar to what's shown in
> Appendix A, the mechanisms in this document may not suit them".  I know
> Section 2 has a bullet at the end that hints at this, but I think it should be
> more explicit.
> 
>

---

From: "Nobo Akiya (nobo)" <[email-masked]>
Date: Sat, 13 Dec 2014 00:57:51 +0000

Hi Eric,

Again, thanks for reviewing this document and providing comments.

Please see in-line for [NOBO].

> below.  None of my copyedit comments should be considered blockers for

[NOBO] English is so much more difficult than technical problems, thanks for the wall of texts :)

> and proactive monitoring much better.

[NOBO] I am very happy to hear that you found it this extension to be beneficial.

> certainly mature enough to be adopted by the WG.

[NOBO] Thanks!

> this needs to happen, but not all of them.

[NOBO] I appreciate it. I have addressed the one you pointed out, and will ask co-authors if they can do a English review.

> Section 1.1: s/terminologies/terms/

[NOBO] Fixed.

> [succeeded|failed]'.  And 'With above scenario' needs to be 'With the above

[NOBO] ACK.

> taking sections [3,4,5] and reordering as [5,3,4].

[NOBO] Pre-00 version originally had the document structure similar to [5, 3, 4], but describing the procedures prior to the new TLVs was suggested from a very long timer IETF participant. With that said, "feel awkward" is something that I'd like to address. After the text you suggested in the next comment, let me add following text.

   Note that Section 5, Section 6,
   Section 7 and Section 8 describe the new TLVs, and looking over those
   sections first may make it easier to read this section.

Would that work?

> both understand the new TLVs and are able to describe LAG members

[NOBO] Like the texts and thanks! Have added.

>    is able to describe outgoing LAG member links separately uses

[NOBO] Incorporated :)

> Section 3.3: "Above procedures..." -> "The procedures above..."

[NOBO] Fixed.

> understands.." -> "Responder LSRs which understand..."

[NOBO] Done.

> described in this section"

[NOBO] Done.

> success case and the failure case to figure out whether they were different.

[NOBO] You are right, I had to spend a minute myself comparing the success/error cases.

> and some don't, and a full failure is where no LAG member traversals

[NOBO] I've taken your idea, but restructured as follows.

   With additional logics, the initiator LSR can perform following
   checks in a scenario where the initiator knows that there is a LAG,
   with two LAG members, between TTL=n and TTL=n+1, and has the
   multipath information to traverse the two LAG members.

   The initiator LSR sends two MPLS echo request messages to traverse
   the two LAG members at TTL=1:

   o  Success case:

      *  One MPLS echo request message reaches TTL=n+1 on an LAG member
         1.

      *  The other MPLS echo request message reaches TTL=n+1 on an LAG
         member 2.

      The two MPLS echo request messages sent by the initiator LSR reach
      two different LAG members at the immediate downstream LSR.

   o  Error case:

      *  One MPLS echo request message reaches TTL=n+1 on an LAG member
         1.

      *  The other MPLS echo request message also reaches TTL=n+1 on an
         LAG member 1.

      One or two MPLS echo request messages sent by the initiator LSR
      does not reach the immediate downstream LSR, or the two MPLS echo
      request messages reach a same LAG member at the immediate
      downstream LSR.
> enough to find any hidden mines here.  This is for the WG to pore over.

[NOBO] :)

> required" -> "additional processing is required"

[NOBO] Fixed.

> Section 11: "Authors" -> "The authors".

[NOBO] Fixed.

> Section 2 has a bullet at the end that hints at this, but I think it should be

[NOBO] Good idea. I have added the suggested text at the end of Section 2.

> [*]  I believe this to be true.  I am willing to be wrong.  It has happened

[NOBO] Not that I think you are wrong, and often you are not, I have replaced "Most [*]" with "Many" in the suggested text to soften the statement, just to be on the safe side :)

There are comments from Mach and Yimin that I need to go through and reflect into the document. I will publish a revision shortly after doing so.

Thanks!

-Nobo

---

From: "Osborne, Eric" <[email-masked]>
Date: Thu, 18 Dec 2014 15:37:40 +0000

Hi Nobo-

  I've trimmed this a bit and replied to specific comments inline with EO#.  Where there's no reply, please take that as acceptance of whatever changes you have proposed or made.

...

> [NOBO] Pre-00 version originally had the document structure similar to [5, 3,
> 4], but describing the procedures prior to the new TLVs was suggested from a
> very long timer IETF participant. With that said, "feel awkward" is something
> that I'd like to address. After the text you suggested in the next comment, let
> me add following text.
> 

EO#  Sure, that works.

...

>       request messages reach a same LAG member at the immediate

EO#  Yes, I think that's cleaner.  

Thanks!

eric

---

From: "Nobo Akiya (nobo)" <[email-masked]>
Date: Thu, 18 Dec 2014 21:44:36 +0000

Hi Eric, Mach, Yimin,

Consolidating the reply to all MPLS-RT reviewers.

Again, many thanks all for taking time to review and provide very good comments.

We have an updated version of the document circulating amongst the authors now, and should be able to post -05  within few days.

Thanks!

-Nobo

> Sent: Thursday, December 18, 2014 10:38 AM
> To: Nobo Akiya (nobo); draft-akiya-mpls-lsp-ping-lag-
> [email-masked]; [email-masked]; VIGOUREUX, MARTIN
> (MARTIN)
> Cc: [email-masked]
> Subject: RE: MPLS-RT review of draft-akiya-mpls-lsp-ping-lag-multipath
> 
> Hi Nobo-
> 
>   I've trimmed this a bit and replied to specific comments inline with EO#.
> Where there's no reply, please take that as acceptance of whatever changes
> you have proposed or made.
> 
> ...
> 
> 
> > > Sections 3-5: These sections could use a reorg.  Starting with
> > > procedures for using new TLVs before defining the new TLVs feels
> > > awkward.  I suggest taking sections [3,4,5] and reordering as [5,3,4].
> >
> > [NOBO] Pre-00 version originally had the document structure similar to
> > [5, 3, 4], but describing the procedures prior to the new TLVs was
> > suggested from a very long timer IETF participant. With that said,
> > "feel awkward" is something that I'd like to address. After the text
> > you suggested in the next comment, let me add following text.
> >
> > > It would benefit from some clarifying text, perhaps something like
> > > "An error case is any case where the OAM across a LAG fails, either
> > > partial or fully.  A partial failure is where some LAG member
> > > traversals behave as they should and some don't, and a full failure
> > > is where no LAG member traversals behave as they should".
> >