Subject: AD review of draft-ietf-bfd-multipoint-14

From: Martin Vigoureux <[email-masked]>
Date: Tue, 17 Apr 2018 17:06:02 +0200

[resend, wrong bfd wg address in first attempt ...]
Authors, WG,

thank you for this document. It is clear and well written.
I didn't find any technical comment to make so I've been nit picking :-)
Please find those comments below.

regards,
martin

---
Please check and address the nits:
https://tools.ietf.org/idnits?url=https://tools.ietf.org/id/draft-ietf-bfd-multipoint-14.txt

On that aspect, does this document really update 7880 as the header 
says? The Introduction only refers to 5880 and it is not clear in the 
body of the Document what effectively impacts 7880. The only thing I saw 
is the addition of new session types but that does not require an update 
in my opinion. Could you clarify?

    i.e. existence of a path between the sender and the receiver.
do you mean "forwarding path"?

Section 2. and Section 3. seem a bit redundant. They both state the same 
thing but from a different angle. Not critical.

    Although this document describes a single head and a set of tails
    spanned by a single multipoint path, the protocol is capable of
    supporting (and discriminating between) more than one multipoint path
    at both heads and tails.
There is no text to describe how one could achieve that. Wouldn't it be 
worth adding some?

    Point-to-point sessions, as described in [RFC5880], are of type
    PointToPoint.
Does this really fit in Section 4.2 which looks to be about the mpBFD 
session model.

    Sessions of type MultipointHead MUST NOT send BFD control packets
    with the State field being set to INIT, and MUST be ignored on
    receipt.
English is not my native language but I wonder if this really says what 
you want. It seems that "Sessions" is the subject of "MUST be ignored" 
while I think it's the packets which are the intended subject. So I'd write:
    and those packets MUST be ignored on receipt.

    Because there is no three-way handshake in Multipoint BFD, a newly
    started head (that does not have any previous state information
    available) SHOULD start with bfd.SessionState set to Down and with
    bfd.RequiredMinRxInterval set to zero in the MultipointHead session.

    To shut down a multipoint session a head MUST administratively set
    bfd.SessionState in the MultipointHead session to either Down or
    AdminDown and SHOULD set bfd.RequiredMinRxInterval to zero.  The
In both these paragraphs one could read that the head "SHOULD set 
bfd.RequiredMinRxInterval to zero" while 4.4.2 says MUST. Clarification 
needed?

s/M, P bit/M and P bits/
---

---

From: Greg Mirsky <[email-masked]>
Date: Tue, 17 Apr 2018 09:34:53 -0700

Hi Martin,
thank you for your thorough review, thoughtful comments and kind words.
Please find my answers to your questions in-line and tagged GIM>>.

Regards,
Greg

On Tue, Apr 17, 2018 at 8:06 AM, Martin Vigoureux <
[email-masked]> wrote:

> On that aspect, does this document really update 7880 as the header says?
> The Introduction only refers to 5880 and it is not clear in the body of the
> Document what effectively impacts 7880. The only thing I saw is the
> addition of new session types but that does not require an update in my
> opinion. Could you clarify?
> GIM>> Yes, you'correct, the only connection to RFC 7880 are the new values
> of bfd.sessionType. The proposal to list RFC 7880 as updated by this
> specification was inteded to address Errata to RFC 7880
> <https://www.rfc-editor.org/errata_search.php?rfc=7880>.
>
> GIM>> Yes. Updated to
i.e. existence of a forwarding path between the sender and the receiver

> There is no text to describe how one could achieve that. Wouldn't it be
GIM>> The question of applicability of this specification to mp2mp scenario
came up at BIER WG meeting in London. Perhaps we can say the these
questions are ouside the scope of this document and discuss whether there
are interested to work on mp2mp case as a separete document?

> Does this really fit in Section 4.2 which looks to be about the mpBFD
GIM>> I think that this short reminder is helpful to explain why this
document adds value PointToPoint, section 4.4.1, to the defined in RFC 7880
bfd.sessionType variable.

> bfd.RequiredMinRxInterval to zero" while 4.4.2 says MUST. Clarification
GIM>> Section 4.4.2 mandates initialization of  bfd.RequiredMinRxInterval and,
I think, applies to the first paragraph you've pointed. Would the following
be clear and consistent:
     Because there is no three-way handshake in Multipoint BFD, a newly
   started head (that does not have any previous state information
   available) SHOULD start with bfd.SessionState set to Down and
   bfd.RequiredMinRxInterval *MUST be* set to zero in the MultipointHead
session.
The second paragraph describes manipulation with the value of
bfd.RequiredMinRxInterval
which process, as noted in section 4.10, "is outside the scope of this
document". That may be reason to use SHOULD and not MUST.

> s/M, P bit/M and P bits/
GIM>> Thanks, done.

---

From: Martin Vigoureux <[email-masked]>
Date: Tue, 17 Apr 2018 18:49:38 +0200

Greg,

thanks. please see in-line.

once I see the update published I will request IETF LC.

-m

Le 2018-04-17 à 18:34, Greg Mirsky a écrit :
> On Tue, Apr 17, 2018 at 8:06 AM, Martin Vigoureux 
> <[email-masked] <mailto:[email-masked]>> wrote:
> 
>     <https://tools.ietf.org/idnits?url=https://tools.ietf.org/id/draft-ietf-bfd-multipoint-14.txt>
> 
>     says? The Introduction only refers to 5880 and it is not clear in
>     the body of the Document what effectively impacts 7880. The only
>     thing I saw is the addition of new session types but that does not
>     require an update in my opinion. Could you clarify?
>     GIM>> Yes, you'correct, the only connection to RFC 7880 are the new
>     values of bfd.sessionType. The proposal to list RFC 7880 as updated
>     by this specification was inteded to address Errata to RFC 7880
I am not sure how this document relates to or addresses the errata. So I 
still think it does not update 7880.

>     i.e. existence of a forwarding path between the sender and the receiver 
thx
> 
>     Section 2. and Section 3. seem a bit redundant. They both state the
>     same thing but from a different angle. Not critical.
> 
> 
>     There is no text to describe how one could achieve that. Wouldn't it
>     be worth adding some?
> 
> GIM>> The question of applicability of this specification to mp2mp 
> scenario came up at BIER WG meeting in London. Perhaps we can say the 
> these questions are ouside the scope of this document and discuss 
> whether there are interested to work on mp2mp case as a separete document?
I don't read this part of the document as talking about mp2mp but rather 
as talking about multiple p2mp trees.
>     Does this really fit in Section 4.2 which looks to be about the
>     mpBFD session model.
> 
> document adds value PointToPoint, section 4.4.1, to the defined in RFC 
> 7880 bfd.sessionType variable.
Well, I would move the text to 4.4.1 then, but not critical.
>     English is not my native language but I wonder if this really says
>     what you want. It seems that "Sessions" is the subject of "MUST be
>     ignored" while I think it's the packets which are the intended
>     subject. So I'd write:
You chose to ignore that one or simply missed it?
>     bfd.RequiredMinRxInterval to zero" while 4.4.2 says MUST.
>     Clarification needed?
> 
> GIM>> Section 4.4.2 mandates initialization of 
> bfd.RequiredMinRxInterval and, I think, applies to the first paragraph 
> you've pointed. Would the following be clear and consistent:
>     bfd.RequiredMinRxInterval _MUST be_ set to zero in the 
> MultipointHead session.
> bfd.RequiredMinRxInterval which process, as noted in section 4.10, "is 
> outside the scope of this document". That may be reason to use SHOULD 
> and not MUST.
Yes, i'd live with that. But then you might also want to clarify in 4.4.2.:
OLD:
          This variable MUST be set to 0 for session type MultipointHead.
NEW:
          This variable MUST be initialized to 0 for session type
          MultipointHead.

---

From: Greg Mirsky <[email-masked]>
Date: Tue, 17 Apr 2018 11:40:16 -0700

Hi Martin,
I have not ignored that comment but missed to ack its acceptance. Two other
outstanding questions:

   - the text, that I've misinterpreted earlier, is in the Overview section:

> scenario came up at BIER WG meeting in London. Perhaps we can say the these
I don't read this part of the document as talking about mp2mp but rather as
talking about multiple p2mp trees.

Sections 4.7 and 4.13.2 provides details on demultiplexing BFD control
packets at a MultipointTail. Would the reference to these sections be
sufficient?

   - yes, moved the reference to Point-to-Point session to section 4.4.1

Attached please find the diff between -14 and the working version of the
draft. Please let me know if the changes address your comments. Will upload
the new version promptly.

Regards,
Greg

On Tue, Apr 17, 2018 at 9:49 AM, Martin Vigoureux <
[email-masked]> wrote:

>> [email-masked] <mailto:[email-masked]>> wrote:
>>
>> scenario came up at BIER WG meeting in London. Perhaps we can say the these
>>     bfd.RequiredMinRxInterval _MUST be_ set to zero in the MultipointHead
>> session.
>> outside the scope of this document". That may be reason to use SHOULD and
>> not MUST.
>>

---

From: Martin Vigoureux <[email-masked]>
Date: Tue, 17 Apr 2018 21:31:47 +0200

Greg,

I'm fine your proposal below.
Please post the final update whenever you can.
Thx

-m

Le 2018-04-17 à 20:40, Greg Mirsky a écrit :
> Hi Martin,
> I have not ignored that comment but missed to ack its acceptance. Two 
> other outstanding questions:
> 
>   * the text, that I've misinterpreted earlier, is in the Overview section:
> 
>                  supporting (and discriminating between) more than one
>         multipoint
>              path
>                  at both heads and tails.
>         GIM>> The question of applicability of this specification to
>         mp2mp scenario came up at BIER WG meeting in London. Perhaps we
>         can say the these questions are ouside the scope of this
>         document and discuss whether there are interested to work on
>         mp2mp case as a separete document?
> 
>     I don't read this part of the document as talking about mp2mp but
>     rather as talking about multiple p2mp trees.
> 
>     Sections 4.7 and 4.13.2 provides details on demultiplexing BFD
>     control packets at a MultipointTail. Would the reference to these
>     sections be sufficient? 
> 
>   * yes, moved the reference to Point-to-Point session to section 4.4.1
> 
> draft. Please let me know if the changes address your comments. Will 
> upload the new version promptly.
> 
> Regards,
> Greg
> 
> On Tue, Apr 17, 2018 at 9:49 AM, Martin Vigoureux 
>         <[email-masked] <mailto:[email-masked]>
>         <mailto:[email-masked]
>         <mailto:[email-masked]>>> wrote:
> 
>         <https://tools.ietf.org/idnits?url=https://tools.ietf.org/id/draft-ietf-bfd-multipoint-14.txt
>         <https://tools.ietf.org/idnits?url=https://tools.ietf.org/id/draft-ietf-bfd-multipoint-14.txt>>
> 
>              <https://www.rfc-editor.org/errata_search.php?rfc=7880
>         <https://www.rfc-editor.org/errata_search.php?rfc=7880>>.
> 
>     I am not sure how this document relates to or addresses the errata.
>     So I still think it does not update 7880.
> 
> 
>                  supporting (and discriminating between) more than one
>         multipoint
>              path
>                  at both heads and tails.
>         GIM>> The question of applicability of this specification to
>         mp2mp scenario came up at BIER WG meeting in London. Perhaps we
>         can say the these questions are ouside the scope of this
>         document and discuss whether there are interested to work on
>         mp2mp case as a separete document?
> 
>     I don't read this part of the document as talking about mp2mp but
>     rather as talking about multiple p2mp trees.
> 
> 
> 
> 
>         GIM>> I think that this short reminder is helpful to explain why
>         this document adds value PointToPoint, section 4.4.1, to the
>         defined in RFC 7880 bfd.sessionType variable.
> 
>         bfd.RequiredMinRxInterval and, I think, applies to the first
>         paragraph you've pointed. Would the following be clear and
>         consistent:
>         bfd.RequiredMinRxInterval which process, as noted in section
>         4.10, "is outside the scope of this document". That may be
>         reason to use SHOULD and not MUST.
> 
>     Yes, i'd live with that. But then you might also want to clarify in
>     4.4.2. <http://4.4.2.>:
>     OLD:

---

From: Greg Mirsky <[email-masked]>
Date: Tue, 17 Apr 2018 13:20:52 -0700

Hi Martin,
I've added references to 4.7 and 4.13.2. The new version -15 has been published.
Thank you for your help and support.

Regards,
Greg

---

From: Martin Vigoureux <[email-masked]>
Date: Wed, 18 Apr 2018 15:22:42 +0200

Greg,

thanks. Sorry, but reading again the document I found:
          This variable MUST be initialized to the appropriate type when
          the session is created, according to the rules in Section 4.13
I might have parsed 4.13 (and subsections) too quickly but I did not 
find any rule regarding the initalization of this variable. Is that 
indeed the case? If so then I would suggest to simply remove the pointer.

-m

---

From: Greg Mirsky <[email-masked]>
Date: Wed, 18 Apr 2018 09:47:25 -0700

Hi Martin,
thank you for catching this empty reference. I've looked back and couldn't
find any text that describes the initialization of the state variable. The
new text simply states:

This variable MUST be initialized to the appropriate type when the session
is created.

Will upload the new version shortly.

Regards,
Greg

On Wed, Apr 18, 2018 at 6:22 AM, Martin Vigoureux <
[email-masked]> wrote:

> I might have parsed 4.13 (and subsections) too quickly but I did not find
> any rule regarding the initalization of this variable. Is that indeed the
> case? If so then I would suggest to simply remove the pointer.
>
> -m
>
>
>>>>          thank you for your thorough review, thoughtful comments and
>>>> kind
>>>>          words.
>>>>          <[email-masked] <mailto:[email-masked]
>>>> >
>>>>               the body of the Document what effectively impacts 7880.
>>>> The
>>>>          only
>>>>               GIM>> Yes, you'correct, the only connection to RFC 7880
>>>> are
>>>>          the new
>>>>               values of bfd.sessionType. The proposal to list RFC 7880
>>>> as
>>>>          updated
>>>>                   Point-to-point sessions, as described in [RFC5880],
>>>> are
>>>>          of type
>>>>                   PointToPoint.
>>>>               Does this really fit in Section 4.2 which looks to be
>>>> about
>>>> the
>>>>               mpBFD session model.
>>>>