Re: AD review for draft-ietf-bfd-multipoint-active-tail

[Martin Vigoureux <[e-mail]>]

Greg,
thanks in return for your answers.
Please see in-line. Please publish a new version.
I'll go through it and then might request LC.

-m

Le 2018-05-23 à 5:56, Greg Mirsky a écrit :
> Hi Martin,
> thank you for the thorough review, thoughtful comments, and questions 
> that require discussion.
> Please find my answers, notes in-line tagged GIM>>.
> 
> Attached are the new working version of the draft and the diff to -07. 
> I'll update the references and share an update after that's done.
> 
> Regards,
> Greg
> 
> On Fri, May 18, 2018 at 10:35 AM, Martin Vigoureux 
> <[e-mail] <mailto:[e-mail]>> wrote:
> 
>     WG, Authors,
> 
>     hello.
> 
>     thank you for this Document. I have done my AD review.
>     I find the document much harder to apprehend than mpBFD. I have a
>     number of comments but may have another round of them based on your
>     replies.
> 
>     -m
> 
>     ---
> 
>     General:
>     I find the use of reliable (and of its variants) not appropriate.
>     You refer to reliability in terms disambiguating failure scenarios,
>     you don't make packet transfer/delivery more reliable which is
>     usually the context that comes in mind when talking about
>     reliability. So I'd really prefer if you could use another word.
> 
> GIM>> Trying to characterize polling tails by the head over the 
> multicast path as unreliable mechanism vs. over the unicast as relaible 
> may be is as a strech. I think that we should replace 
> "unreliable/semi-reliable/reliable" references with ones that 
> characterize the polling. The "unreliable" notification to the head 
> doesn't use polling tail(s) and may be referred to as no-polling method. 
> For the two other methods I can propose two options:
> 
>   * "in-band" for polling using the multicast tree and "out-band" -
>     polling using unicast;
>   * "fate-sharing" for polling using the multicast tree and "disjoint" -
>     polling using unicast.
I don't like any of these :-)

I'd simply call them: "no-poll", "1-poll", "2-polls".
And I would rewrite first paragraph as:
    This application of BFD is an extension to Multipoint BFD
    [I-D.ietf-bfd-multipoint], which allows tails to notify the head of
    the lack of multipoint connectivity.  As a further option, heads can
    request a notification from the tails by means of a polling
    mechanism.  Notification to the head can be enabled for all tails, or
    for only a subset of the tails.

>     The discussion on fate sharing between unicast and multipoint paths
>     is really reduced to the bare minimum while it is of key importance
>     on the operation of the protocol and on the deduction the head can
>     make of what it receives or not.
> 
> GIM>> The new text to introduce and explain the terms may be good place 
> to expand on how selection of the path to use for tail polling impacts 
> on how useful is the proposed extension.
Maybe, though I'm not convinced. To do so you'd have to get a bit into 
the details of the three modes of operation. That would look a lot like 
4.1/2/3/4.
I think it would fit nicely in 4. because you introduce here the types 
of paths. Have a paragraph there which raises the awareness of the 
reader to the fate sharing issue. Doesn't need to be long.

>     Abstract
> 
>     Please specify here which document(s) this one updates. Please see
>     further down on the Update point.
You still need to address that.
This Document updates mpBFD. It is also missing in the header now.
> 
> 
>     1.  Introduction
> 
>         Multipoint BFD base document [I-D.ietf-bfd-multipoint] describes
>         procedures to verify only the head-to-tail connectivity over the
>         multipoint path.  Although it may use unicast paths in both
>         directions, Multipoint BFD does not verify those paths (and in fact
>         it is preferable if unicast paths share as little fate with the
>         multipoint path as is feasible, so to increase probability of
>         delivering the notification from the tail to the head).
>     This is unclear. The first sentence sets the reader in the context
>     of I-D.ietf-bfd-multipoint where unicast paths are not discussed at
>     all. And the rest of this paragraph discusses the unicast paths
>     which are in fact introduced later in this document. So this is
>     totally confusing. One only understands this after having read the
>     whole document...
>     I would suggest to completely remove, or rephrase to indicate to the
>     user that this is an aspect that is introduced later, or move to the
>     relevant place in the document.
> 
> GIM>> The text is confusing, agree. And I think that removing this 
> paragraph would not lose any helpful information as the very next 
> paragraph gives clear motivation for this extension:
>     The goal of this application is for the head to reasonably rapidly
>     have knowledge of tails that have lost connectivity from the head.
agreed.

> 
> 
>         This document effectively modifies and adds to the base BFD
>         specification [RFC5880] and base BFD multipoint document
>         [I-D.ietf-bfd-multipoint].
>     In the same was as for mpBFD, I don't see how this document updates
>     7880. Please clarify.
> 
> GIM>> It is all related, or so we thought, to bfd.SessionType that has 
> been added as the new BFD variable in RFC 7880 and the new values added 
> in mp BFD. In this draft a new value, MultipointClient, is added:
>      bfd.SessionType
> 
>        The type of this session as defined in [RFC7880].  A new value
>        introduced is:
> 
>           MultipointClient: A session on the head that tracks the state
>           of an individual tail, when desirable.
> 
> We've discussed whether the base mp BFD specification updates RFC 7880 
> and had agreed to remove it from the list. I'll be glad to do the same 
> for this draft.
I view Update as a way to indicate a modification rather than an 
addition. As an example 7471 does not Update 3630. From that point of 
view, adding a value to a variable defined in 7880 does not change and 
thus does not Update 7880.

> 
>     In fact I also think this document does not update 5880.
>     This document updates mpBFD so in principle that should be reflected
>     in the header, but I'm not sure if we can reference anything else
>     than RFCs there... But I'll push the two at the same time to IESG so
>     that might work.
>     And one could wonder why these two documents are separate and not
>     merged.
> 
> GIM>> I think that you're right that this specification does not  update 
> RFC 5880 but does update mp BFD specification (which, in turn, updates 
> RFC 5880). Should references to sections of RFC 5880 in 14.13.1 through 
> 14.13.3 of this draft be replaced with references to corresponding 
> sections 4.13.1 through 4.13.3 of I-D.ietf-bfd-multipoint?
Yes, please make sure that you point to the correct rfc/section that you 
update.
Please also add what you update in the header and in the abstract.

>     2.  Overview
> 
>         A head may wish to be alerted to the tails' connectivity (or lack
>         thereof), there are a number of options.
>     Something's wrong with the structure of that sentence.
you missed that one.

>     I find this:
>         First, if all that is needed is an unreliable failure notification,
>         as discussed in Section 3.2, the head can request the tails to
>         transmit unicast BFD Control packets back to the head when the path
>         fails, as described in Section 4.4.
>     somehow in conflict with what is said in 3.2 (to which the reader is
>     pointed) and which says:
>         In this scenario, the tail sends back unsolicited BFD packets in
>         response to the detection of a multipoint path failure.  It uses the
>         reverse unicast path, but not the forward unicast path.
>     On one hand you say "request" on the other you say "unsolicited",
>     and just before that word you say "sends back" which gives a sense
>     of "in response to". Could you clarify?
>     I have more comments on this section, and more precisely on the
>     different modes of operations, but I'll discuss theses as part of
>     the review of Section 3.x
> 
> GIM>> The new state variable bfd.ReportTailDown controls whether a 
> MultipointClient sends periodic, i.e. unsolicited, BFD control packets 
> to the MultipointHead, as
Did you really mean "MultipointClient to MultipointHead" here?

Also, as far as I understand, in the "no-poll" scenario the head is not 
requesting anything so this needs to be reworded:
    the head can request the tails to transmit unicast BFD Control
    packets back to the head when the path fails, as described in Section
    4.4.
You could say:
    the tails can send unsolicited unicast BFD Control packets to the
    head when the path fails.

Also:
s/the tail sends back unsolicited/the tail sends unsolicited/
> 
> 
>     In the two subsequent paragraphs a pointer to 3.3. and 3.4 would not
>     be superfluous.
> 
> GIM>> Will add.
Please do.
> 
> 
>         If the head wishes to know the identity of the tails on the
>         multipoint path, it may solicit membership by sending a multipoint
>     I don't think it is appropriate to talk about identity and
>     membership. Head is polling a set of tails. You can't say much more
>     than that.
> 
> GIM>> Agree. Would the following update be acceptable:
> OLD TEXT:
>     If the head wishes to know the identity of the tails on the
>     multipoint path, it may solicit membership by sending a multipoint
>     BFD Control packet with the Poll (P) bit set ...
> NEW TEXT:
>     If the head wishes to know of the active tails on the
>     multipoint path, it may send a multipoint
>     BFD Control packet with the Poll (P) bit set ...
I'm ok with that, but that is not what you changed the text to in the 
Document.
> 
> 
> 
>         Individual tails may be configured so that they never send BFD
>         control packets to the head, even when the head wishes notification
>         of path failure from the tail.  Such tails will never be known
>     to the
>         head, but will still be able to detect multipoint path failures from
>         the head.
>     ok, but how does the head knows of this config? How can the head
>     distinguish between a failure and this? I guess the answer is oos of
>     the document but I think that this situation is worth more than 4 lines.
>     Or is there a requirement that a Head SHOULD/MUST NOT have a
>     MulticastClient session with a tail who is silent?
> 
> GIM>> I think this refers to the case when bfd.SilentTail being set to 
> 1. In this case the tail is invisible to the head as it would not 
> respond to any polling, muticast or unicast. As result, such tail would 
> not notify the head of the detected failure either. These tails acting 
> as in the mode defined in the base mp BFD specification.
Ok. understood. Then I think there is a slight clarification to bring.
Indeed the first sentence says:
    even when the head wishes notification of path failure from the tail.
This gives the impression that the head knows *the tail*. You can't be 
wishing a notification from something you don't know exists.
Maybe simply remove that part of the sentence.

>     3.x Operational Scenarios
> 
>     I find the description of the different modes of operation quite
>     confusing. Beyond this I have other comments/questions on 3.x that
>     you'll find after.
I would like to see some text saying:
For the different modes described below the setting of new state 
variables are given even if these are only introduced later in the 
document (see Section X.Y).

And that for each mode you say how should the variables be set, in which 
session.


>     3.1 is plain multipoint BFD I guess. Correct?
> 
> GIM>> Yes the behavior of MultipointTail as defined 
> in I-D.ietf-bfd-multipoint (can we refer to it as base mp BFD 
> specification?). But with active tail extension this behavior is the 
> result of setting the new BFD variables to very specific values. Section 
> 4.4 explains that the base mp BFD mode is when bfd.ReportTailDown to 0 
> bfd.SessionType is  MultipointHead.
So I believe it is important to inform the reader of that. With 
something like:
    This mode emulates the behaviour of mpBFD.


> 
>     In 3.2 you say that tails send packets to the source when they
>     detect a failure (stop receiving packets). At this point of the
>     reading it is not clear which element makes a difference between 3.1
>     and 3.2 such that, suddenly in 3.2, tails can send packets.
> 
> GIM>> For one, bfd.SilentTail must be set to 0.
> 
>     I believe it is worth clarifying that, though not giving too many
>     details. Relates to 4.4?
>     Also at this stage it is not clear what are those packets that tails
>     send in 3.2. Are they replies to Polls? If so what is the difference
>     between 3.2 and 3.3?
> 
> GIM>> The MultipointTail may periodically send BFD control packets with 
> Poll set. If the MultipointHead does not send unicast BFD control packet 
> with Poll cleared and the Final set, then, I believe, the MultipointTail 
> will continue sending its Poll packets periodically. Hence the 
> difference between polling by the MultipointHead per 3.3 and 3.4 and the 
> unsolicited periodic Polls from the tail.
> 
> 
> 
>     3.1.  No Head Notification
> 
>     You say:
>         Since the only path used in this scenario is the multipoint path
>     as if it had been stated before that this scenario only uses the mpPath.
>     It would be much more comprehensible if it was saying:
>         In this scenario only the multipoint path is used.
> 
> GIM>> Thank you, accepted. The text now is:
> In this scenario only the multipoint path is used and none of the others 
> matter.
ok
> 
> 
> 
>     3.3.  Semi-reliable Head Notification and Tail Solicitation
> 
>     You say (twice):
>         the head will see the BFD session fail
>     Could you clarify which session fails,?
> 
> GIM>> It is the MultipointClient session. Would the new text make it 
> clearer:
> OLD TEXT:
> ... the head will see the BFD session fail, but the state of the
>     multipoint path will be unknown to the head.
> NEW TEXT:
> ... the head will see that the particular MultipointClient
> session fail ...
ok

> 
> 
> 
>     3.4.  Reliable Head Notification
> 
>     same comment as for 3.3
> 
> GIM>> Would the text proposed above be acceptable?
yes. please apply it
> 
> 
> 
>     4.  Protocol Details
> 
>         This section is an update to section 4 of [I-D.ietf-bfd-multipoint].
>     Should you rather say that it's only some parts of this section
>     which update mpBFD, and say which ones.
> 
> GIM>> Here's the proposed new text:
> OLD TEXT:
>     This section is an update to section 4 of [I-D.ietf-bfd-multipoint].
> NEW TEXT:
>     This section updates Section 4 of [I-D.ietf-bfd-multipoint] as the 
> following:
>     - Section 4.3 introduces new state variables and modifies the usage 
> of a few existing ones;
>     - Section 4.13 replaces the corresponding sections in the base BFD 
> for multipoint networks specification.
ok

> 
> 
> 
> 
>     4.1.  Multipoint Client Session
> 
>         If the head is keeping track of some or all of the tails, it has a
>         session of type MultipointClient per tail that it cares about.  All
>         of the MultipointClient sessions for tails on a particular
>     multipoint
>         path are grouped with the MultipointHead session to which the
>     clients
>         are listening.
>     What do you mean by "grouped", associated?
> 
> GIM>> Yes, "associated" is better term, I agree. Will update.
ok

> 
> 
>         These sessions receive any BFD Control packets sent by the
>     tails, and
>         never transmit periodic BFD Control packets other than Poll
>     Sequences
>         (since periodic transmission is always done by the MultipointHead
>         session).
>     Should it be "MUST never send"?
> 
> GIM>> Would s/never/MUST NOT/ to make it into "MUST NOT transmit" be 
> acceptable?
yes, thanks
> 
> 
>         A BFD Poll Sequence may be sent over such a session to a tail if the
>         head wishes to verify connectivity.
>     It is not clear here if you are talking about sending a multipoint
>     Poll sequence to all tails over the MultipointHead session or a
>     unicast Poll sequence to a single tail over one MultipointClient
>     session.
> 
> GIM>> s/such a session/a MultipointClient session/
thanks

> 
> 
> 
>     4.3.2.  New State Variable Value
> 
>            This variable MUST be initialized to the appropriate type
>     when the
>            session is created, according to the rules in section 4.13 of
>            [I-D.ietf-bfd-multipoint].
>     There is nothing in 4.13 of mpBFD that talks about the
>     initialization of bfd.SessionType.
> 
> GIM>> This is the problem with keeping cross-references while updating 
> both documents. The correct reference now is to Section 4.4 
> of[I-D.ietf-bfd-multipoint].

ok
> 
> 
> 
>     4.3.3.  State Variable Initialization and Maintenance
> 
>         Some state variables defined in section 6.8.1 of the [RFC5880] needs
>         to be initialized or manipulated differently depending on the
>     session
>         type (see section 4.4.2 of [I-D.ietf-bfd-multipoint]).
>     s/of the/of/
>     s/needs/need/
>     s/ (see section 4.4.2 of [I-D.ietf-bfd-multipoint])./. The values of
>     some of these variables relate to those of the same variables of a
>     MultipointHead session (see section 4.4.2 of
>     [I-D.ietf-bfd-multipoint])./
> 
> GIM>> All accepted and applied to the working version.
thanks
minor typo:
s/type/type./
> 
> 
> 
>            bfd.RequiredMinRxInterval
>               It should be noted that for sessions of type MultipointTail,
>               this variable only affects the rate of unicast Polls sent by
>               the head; the rate of multipoint packets is necessarily
>               unaffected by it.
>     what is specific to MultipointClient here? If nothing, please remove.
>     If something, please add it clearly.
> 
> GIM>> I propose the new text below.
> OLD TEXT:
>           It should be noted that for sessions of type MultipointTail,
>           this variable only affects the rate of unicast Polls sent by
>           the head; the rate of multipoint packets is necessarily
>           unaffected by it.
> NEW TEXT:
> It MAY be set to zero at the head BFD system to suppress traffic from 
> the tails.
> Setting it to zero in the MultipointHead session suppresses traffic from 
> all tails,
> setting in a MultipointClient session suppresses traffic form a single tail.

ok
> 
> 
> 
>     4.4.  Controlling Multipoint BFD Options
> 
>         The most basic form of operation, as explained in
>         [I-D.ietf-bfd-multipoint], in which BFD Control packets flow only
>         from the head and no tracking is desired of tail state at the head,
>         is accomplished by setting bfd.ReportTailDown to 0 in the
>         MultipointHead session (Section 3.1).
>     I am a bit concerned that mpBFD in fact works with a state variable
>     defined in another document. Wouldn't it be better to introduce this
>     variable in mpBFD and set it to always be zero and then allow in
>     this doc to be set at 1. A bit like the M bit.
> 
> GIM>> Great idea, thank you! If we do that, would such update to mpBFD 
> document require re-start of WGLC?
thinking twice about that, let's keep the way things are.

> 
> 
>     You have text relating to 3.1. What about 3.2/3/4?
> 
> GIM>> The fifth paragraph can be back referenced to section 3.4. The 
> fourth paragrah describes use of bfd.ReportTailDown common to 
> unsolicited notifications, multicast and unicast polling, i.e. sections 
> 3.2, 3.3, and 3.4.
please do.
> 
> 
>         If the head wishes to know the identity of the tails, it sends
>         multipoint Polls as needed.  Previously known tails that don't
>         respond to the Polls will be detected (as per Section 3.3).
>     Again, I'd prefer not to talk about identity, but simply about
>     messages received from tails or not.
>     I don't see the purpose of this paragraph here. What is the relation
>     with state variables?
> 
> GIM>> It may be better to move this paragraph down by one, swap 
> paragraphs. And would the following re-wording mak text clearer:
> OLD TEXT:
>     If the head wishes to know the identity of the tails, it sends
>     multipoint Polls as needed.  Previously known tails that don't
>     respond to the Polls will be detected (as per Section 3.3).
> NEW TEXT:
>     If the head wishes to know of the active tails, it sends
>     multipoint Polls as needed.  Previously known tails that don't
>     respond to the Polls will be detected (as per Section 3.3).
ok, ok.
> 
> 
>         If the head wishes to be notified by the tails when they lose
>         connectivity, it sets bfd.ReportTailDown to 1 in either the
>         MultipointHead session (if such notification is desired from all
>         tails) or in the MultipointClient session (if notification is
>     desired
>         from a particular tail).  Note that the setting of this variable
>     in a
>         MultipointClient session for a particular tail overrides the setting
>         in the MultipointHead session.
>     Does that correspond to 3.2, 3.3, 3.4?
> 
> GIM>> Yes, it enables all three modes.
Still not clear.
ReportTailDown = 0 : don't poll tails
ReportTailDown = 1 : poll tails
is that correct?

If so:
s/If the head wishes to be notified by the/If the head wishes to request 
a notification from the/

> 
> 
>         If the head wishes to verify the state of a tail on an ongoing
>     basis,
>         it sends a Poll Sequence from the MultipointClient session
>     associated
>         with that tail as needed.
>         If the head wants to more quickly be alerted to a session failure
>         from a particular tail, it sends a BFD Control packet from the
>         MultipointClient session associated with that tail.  This has the
>         effect of eliminating the initial delay, described in Section
>     4.13.3,
>         that the tail would otherwise insert prior to transmission of the
>         packet.
>     I don't see the link with state variables here neither. Consider
>     moving somewhere else.
> 
> GIM>> I read it as continuation of what described in the preceeding 
> paragraph regarding setting bfd.ReportTailDown in MultipointClient.
So please indicate it
> 
> 
>         If a tail wishes to operate silently (sending no BFD Control packets
>         to the head) it sets bfd.SilentTail to 1 in the MultipointTail
>         session.  This allows a tail to be silent independent of the
>     settings
>         on the head.
>     The implications of that option are not really discussed. The head
>     will likely consider the session down, no?
> 
> GIM>> The head would not learn of such tail nor it will be able to 
> notice the tail state change. I think that s/be silent/be invisible to 
> the head/ may make the text clearer.
understood
> 
> 
> 
>     4.5.  State Machine
> 
>         The state machine for session of type MultipointClient is same as
>         defined in section 4.5 of [I-D.ietf-bfd-multipoint].
>     Is that really the case? MultipointHead only fails administratively
>     while MultipointClient can fails based on received message, no?
> 
> GIM>> True. It is noted in Section 4.5 in mpBFD that for MultipointHead 
> all state transitions are administratively driven. But the diagram is 
> still applicable for BFD MultipointClient session.
So, is a small clarification needed? I would think so.
> 
> 
> 
>     4.6.  Session Establishment
> 
>         The head directly controls whether or not tails are allowed to send
>         BFD Control packets back to the head.
>     Not fully true, because of SilentTail, no?
> 
> GIM>> It can be done by setting bfd.RequiredMinRxInterval to zero in 
> MultipointHead session or a MultipointClient session. The former will 
> force all tails not to send any BFD packet to the head. The latter - 
> only the particular BFD tail. As stressed in the specification, the 
> value in MultipointClient overrides the value in MultipointHead.
So please remind that to the reader (pointing to mpBFD spec)
> 
> 
> 
>     4.13.1/2/3
> 
>     I think that, as said, you are not updating 5880. Also, you said
>     that you update sections but really you are updating parts of them.
>     I encourage you to find a clear way to indicate what you change/update.
> 
> GIM>> I'll remove from these sections references to sections 6.8.6 and 
> 6.8.7 of RFC 5880 and link updates to mpBFD specification.
please do
> 
> 
> 
>     7. Security Considerations
> 
>     Can't you elaborate a bit? This look a bit like the bare minimum.
> 
> GIM>> You're right and more should be said about possible impact 
> MultipointClient sessions. Proposed new text below:
> NEW TEXT:
>     Additionally, implementations that create
>     MultpointClient sessions dynamically upon receipt of BFD
>     Control packet from a tail MUST implement protective measures to 
> prevent an
>     infinite number of MultipointClient sessions being created.  Below are
>     listed some points to be considered in such implementations.
> 
>        When the number of MultipointClient sessions exceeds the number of
> expected tails, then the implementation should generate an alarm
> to users to indicate the anomaly.
> 
>        The implementation should have a reasonable upper bound on the
>        number of MultipointClient sessions that can be created, with the
>        upper bound potentially being computed based on the number of
>        multicast streams that the system is expecting.
> 
> The text may be inserted as the second paragraph or replace the current 
> second paragraph.

ok
> 
>     ---
> 
>