Re: [RTG-DIR] Rtgdir last call review of draft-ietf-bess-mvpn-fast-failover-11

[Greg Mirsky <[e-mail]>]

Hi Adrian,
many thanks for the review and comments. Please find my notes in-line below
tagged GIM>>.
Attached, please find the diff highlighting the updates and new working
version of the draft.

Regards,
Greg

On Mon, Oct 19, 2020 at 1:09 PM Adrian Farrel via Datatracker <
[e-mail]> wrote:

> Reviewer: Adrian Farrel
> Review result: Has Issues
>
> Hello,
>
> I have been selected as the Routing Directorate reviewer for this draft.
> The
> Routing Directorate seeks to review all routing or routing-related drafts
> as
> they pass through IETF last call and IESG review, and sometimes on special
> request. The purpose of the review is to provide assistance to the Routing
> ADs.
> For more information about the Routing Directorate, please see
> http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir
>
> Although these comments are primarily for the use of the Routing ADs, it
> would
> be helpful if you could consider them along with any other IETF Last Call
> comments that you receive, and strive to resolve them through discussion
> or by
> updating the draft.
>
> Document: draft-ietf-bess-mvpn-fast-failover-11.txt
> Reviewer: Adrian Farrel
> Review Date: 2020-10-18
> IETF LC End Date: 2020-10-19
> Intended Status: Proposed Standard
>
> ==Summary:==
>
> I have some minor concerns about this document that I think should be
> resolved
> before publication.
>
> ==Comments:==
>
> This document is fairly easy to read, but demands a thorough understanding
> of
> RFCs 6513 and 6514. That is not unreasonable.
>
> I also hope that the IDR working group has had a good opportunity to review
> this work.
>
> ==Major Issues:==
>
> None
>
> ==Minor Issues:==
>
> Abstract
>
> I think the Abstract should mention explicitly that this document
> extends BGP (and how).
>
GIM>> Update the Abstract to emphasize introduced extensions to BGP:
OLD TEXT:
   This document defines multicast VPN extensions and procedures that
   allow fast failover for upstream failures, by allowing downstream PEs
   to take into account the status of Provider-Tunnels (P-tunnels) when
   selecting the Upstream PE for a VPN multicast flow, and extending BGP
   MVPN routing so that a C-multicast route can be advertised toward a
   Standby Upstream PE.
NEW TEXT:
   This document defines multicast VPN extensions and procedures that
   allow fast failover for upstream failures by allowing downstream PEs
   to consider the status of Provider-Tunnels (P-tunnels) when selecting
   the Upstream PE for a VPN multicast flow.  The fast failover is
   enabled by using RFC 8562 BFD for Multipoint Networks and the new BGP
   Attribute - BFD Discriminator.  Also, the document introduces a new
   BGP Community, Standby PE, extending BGP MVPN routing so that a
   C-multicast route can be advertised toward a Standby Upstream PE.
>
>
> ---
>
> Section 3 notes that the procedure (presumably the procedure defined
> in this section) is OPTIONAL. I didn't see anything similar in sections
> 4 and 5 stating that those procedures are optional. Presumably, since
> this document is not updating any other RFCs, all of these procedures
> are optional.
>
> Actually it would be good to clarify how all these procedures fit in
> with "legacy" deployments, and how they are all optional procedures. I
> think that needs a short statement in the Introduction and a small
> section of its own (maybe between 6 and 7).
>
GIM>> Thank you for the suggestion. I've updated the Introduction in this
way:
OLD TEXT:
   Section 4 describes protocol extensions that can speed up failover by
   not requiring any multicast VPN routing message exchange at recovery
   time.

   Moreover, section 5 describes a "hot leaf standby" mechanism, that
   uses a combination of these two mechanisms.  This approach has
   similarities with the solution described in [RFC7431] to improve
   failover times when PIM routing is used in a network given some
   topology and metric constraints.
NEW TEXT:
   Section 4 describes optional protocol extensions that can speed up
   failover by not requiring any multicast VPN routing message exchange
   at recovery time.

   Moreover, Section 5 describes a "hot leaf standby" mechanism that can
   be used to improve failover time in MVPN.  The approach combines
   mechanisms defined in Section 3 and Section 4 has similarities with
   the solution described in [RFC7431] to improve failover times when
   PIM routing is used in a network given some topology and metric
   constraints.

I think that Section 5 is intended to explain how introduced BGP extensions
and their use described in Section 3 and Section 4 enable operators to
provide protection for multicast services. Would you suggest adding a new
text to the section to highlight particular aspects of introducing
protection in MVPN?


> ---
>
> It is curious (to me) that 3.1.1 describes a way to know that a P-tunnel
> is up.  You don't say, however, if being unable to determine that the
> P-tunnel is up using this method is equivalent to determining that the
> P-tunnel is down. (Previously in 3.1 you have talked about the "tunnel's
> state is not known to be down".)
>
GIM>> This method, as noted in the document, is similar to BGP next-hop
tracking, may be computationally intensive, and cannot be run frequently.
So, in periods between checking whether the root address in the x-PMSI
Tunnel attribute is reachable the state is "not known to be down".

>
> By the way, do you ever say that a P-tunnel has just these two statuses
> (up and down) because that could make a big difference?
>
GIM>> I think that the document then needs to discuss what impact detection
time has on MVPN. For example, if the detection time is in single-digit
seconds, a two-state model can be used. But would it be a useful model if
the detection time is in tens of seconds? Should a "not known to be down"
state be introduced?

>
> Note that 3.1.2 etc also establish ways to know that the tunnel is up,
> but not ways to determine whether the tunnel is down.
>
GIM>> In this section the state of a P-tunnel is equated with the state of
the last link of that tunnel. The document notes that if the link is Up,
then the P-tunnel is considered down. It is implied, that if it is
determined that the link is Down, then the state of the P-tunnel is
considered Down. Would you recommend adding an explanation to the document?

>
> To reiterate, "I don't know if it is up" is not the same as "I know it
> is down."
>
GIM>> Indeed. It is analogous to "it was Up the last time I've checked on
it". It meant to be used when the interval between checking is significant.

>
> ---
>
> 3.1.2
>
>    Using this method when a fast restoration mechanism (such as MPLS FRR
>    [RFC4090]) is in place for the link requires careful consideration
>    and coordination of defect detection intervals for the link and the
>    tunnel.  In many cases, it is not practical to use both protection
>    methods at the same time.
>
> OK, I considered them carefully. Now what? :-)
>
> I think you have to give implementation guidance.
>
GIM>> I agree, an operational recommendation could be helpful. Usually, in
case of multi-layered protection, detection intervals on the higher layer
are 10 times of guaranteed restoration time of the lower layer. Would you
recommend adding this to the text as an example of a deployment?

>
> ---
>
> All of 3.1.x are timid about the use of the mechanisms they describe.
>
> I think that the end of 3.1 should say that an implementation may choose
> to use any of these mechanisms to determine the status of the P-tunnel.
>
GIM>> Will the following text reflect that:
NEW TEXT:
   An implementation may support any combination of the methods
   described in this section and provide a network operator with control
   to choose which one to use in the particular deployment.


>
> This is quite stark, however, in 3.1.3 where you have...
>
>    When signaling state for a P2MP TE LSP is removed (e.g., if the
>    ingress of the P2MP TE LSP sends a PathTear message) or the P2MP TE
>    LSP changes state from Up to Down as determined by procedures in
>    [RFC4875], the status of the corresponding P-tunnel SHOULD be re-
>    evaluated.  If the P-tunnel transitions from Up to Down state, the
>    Upstream PE that is the ingress of the P-tunnel SHOULD NOT be
>    considered a valid UMH.
>
> The use of SHOULD and SHOULD NOT is puzzling. Is this "if this mechanism
> is being used, the status SHOULD..." or is it "if a P2MP MPLS-TE tunnel
> is being used, this mechanism SHOULD be used"? In the former case, the
> SHOULD is presumably a MUST. In the latter case, why is this worthy of
> BCP 14 language when:
> - this whole document is optional
> - the mechanisms in 3.1.x are all optional
>
> But 3.1.4, 3.1.5, 3.1.6, 3.1.7 also use BCP 14 language. I'm pretty sure
> you mean "if this mechanism is being used..."
>
> In case you determine to keep any use of "SHOULD" you need to describe
> under what circumstances an implementation might diverge from this
> strong advice.
>
GIM>> Thank you for pointing this ambiguity that, upon careful reading,
could be confusing. You're right, it is meant to follow the "if this
mechanism is used" clause. But once we define the mechanism itself, the
normative language to use here is MUST/MUST NOT. Below is the updated text
for your consideration:
NEW TEXT:
   When using this method and if the signaling state for a P2MP TE LSP
   is removed (e.g., if the ingress of the P2MP TE LSP sends a PathTear
   message) or the P2MP TE LSP changes state from Up to Down as
   determined by procedures in [RFC4875], the status of the
   corresponding P-tunnel MUST be re-evaluated.  If the P-tunnel
   transitions from Up to Down state, the Upstream PE that is the
   ingress of the P-tunnel MUST NOT be considered a valid UMH.

>
> ---
>
> 3.1.6
>
> What should I do if I don't recognise or support the setting of the BFD
> Mode field?
>
GIM>> I think that the same handling applies as for the malformed attribute:
   If malformed, the UPDATE
   message SHALL be handled using the approach of Attribute Discard per
   [RFC7606].
I propose to extend the applicability of the rule with the following update
to the sentence:
NEW TEXT:
   The BFD Discriminator attribute MUST be considered malformed if its
   length is not a non-zero multiple of four.  If the setting of the BFD
   Mode field is not recognized or not supported, or the attribute
   considered malformed, the UPDATE message SHALL be handled using the
   approach of Attribute Discard per [RFC7606].

>
> ---
>
> 4.1
>
>    The normal and the standby C-multicast routes must have their Local
>    Preference attribute adjusted
>
> Should this be "MUST"?
>
GIM>> I think that is not an actionable 'must'. It could be expressed as

The Local Preference attribute of the normal and the standby C-multicast
route needs to be adjusted.

Would you recommend using the re-worded passage?

>
> ---
>
> 7.1
>
>    IANA is requested to allocate the BGP "Standby PE" community value
>    (TBA1) from the Border Gateway Protocol (BGP) Well-known Communities
>    registry.
>
> There are three ranges. You need to tell IANA which range to use.
> Presumably not Private Use (because they are not assigned). But do you
> want an assignment from the FCFS range or the Standards Action range?
>
GIM>> Thank you for pointing to the missing detail. FCFS range is fine. The
updated text:
NEW TEXT:
   IANA is requested to allocate the BGP "Standby PE" community value
   (TBA1) from the Border Gateway Protocol (BGP) Well-known Communities
   registry using the First Come First Served registration policy.

>
> ==Nits:==
>
> Abstract
>
> Notwithstanding the terminology difference between "upstream" and
> "Upstream" defined in Section 2, the distinction made in the text
> here is unclear. I think that lowercase "upstream" would not be
> confusing in this text.
>
GIM>> Agree. s/Upstream/upstream/

>
> ---
>
> Requirements Language
>
> Please move this to a new section 2.1 to be consistent with the RFC
> Editor style guide.
>
GIM>> Done.

>
> ---
>
> Section 1
>
>    In the context of multicast in BGP/MPLS VPNs
>
> That could use a reference.
>
GIM>> Added reference to RFC 6513.

>
> ---
>
> Section 1
>
> I don't think the description of what is in which section of the
> document is quite accurate. Maybe the document has moved on? In any
> case, a more specific mention of which protocols are extended/modified
> would be good.
>
GIM>> I've updated the section to be more specific:
NEW TEXT:
   Section 3 describes local procedures allowing an egress PE (a PE
   connected to a receiver site) to take into account the status of
   P-tunnels to determine the Upstream Multicast Hop (UMH) for a given
   (C-S, C-G).  One of the optional methods uses [RFC8562] and the new
   BGP Attribute - BFD Discriminator.  None of these methods provide a
   "fast failover" solution when used alone, but can be used together
   with the mechanism described in Section 4 for a "fast failover"
   solution.

   Section 4 describes an optional BGP extension, a new Standby PE
   Community. that can speed up failover by not requiring any multicast
   VPN routing message exchange at recovery time.

>
> I am pretty sure that the reader has no hope of understanding this work
> without having first read and absorbed RFC 6513 and RFC 6514. It would
> be worth adding a short statement like "It is assumed that the reader is
> familiar with the workings of multicast MPLS/BGP IP VPNs as described in
> [RFC6513] and [RFC6514]."
>
GIM>> Thank you for the suggestion. Indeed, I kept both RFC open while
editing the draft.

>
> ---
>
> Section 2
>
>    x-PMSI: I-PMSI or S-PMSI
>
> This is too brief!  I think you need.
>
>    PMSI: P-Multicast Service Interface
>    I-PMSI: Inclusive PMSI
>    S-PMSI: Selective PMSI
>    x-PMSI: Either an I-PMSI or an S-PMSI
>
> It would be also good to list the other imported terms:
>
>    P-tunnel: Provider-Tunnels
>    UMH: Upstream Multicast Hop
>
> I think you might collect some of the abreviations into a table in this
> section. MVPN, RD, RP, NLRI, VRF, EC, AC, MED, ...
>
GIM>> Thank you for the suggested expansions. I've moved them all to a new
Acronyms section (except for EC and AC) (not in the alphabetical order):
2.3.  Acronyms

   PMSI: P-Multicast Service Interface

   I-PMSI: Inclusive PMSI

   S-PMSI: Selective PMSI

   x-PMSI: Either an I-PMSI or an S-PMSI

   P-tunnel: Provider-Tunnels

   UMH: Upstream Multicast Hop

   VPN: Virtual Private Network

   MVPN: Multicast VPN

   RD: Route Distinguisher

   RP: Rendezvous Point

   NLRI: Network Layer Reachability Information

   VRF: VPN Routing and Forwarding Table

   MED: Multi-Exit Discriminator

>
> ---
>
> Section 3
>
> s/Section 5.1 [RFC6513]/Section 5.1 of [RFC6513]/
>
GIM>> Done.

>
> ---
>
> Section 3 has
>
>    selection, which will result in the downstream PE to failover to the
>    Upstream PE, which is next in the list of candidates.
>
> The language is a little unclear. Maybe...
>
>    selection.  This will result in the downstream PE failing over to
>    use the next Upstream PE in the list of candidates.
>
GIM>> Thank you for the suggested text it is much better than what in the
document. Done.

>
> ---
>
> Section 3 has
>
>    Because of that, procedures described in Section 9.1.1 of [RFC6513]
>    MUST be used when using I-PMSI P-tunnels.
>
> Aren't those procedures already mandatory? That section of 6513 already
> uses "MUST" (although it oes go on to say that it might not be possible
> to apply the procedure and delegates processing to 9.1.2 and 9.1.3 -
> peculiarly using lowercase must for that delegation). I wonder whether
> you are saying "this case is covered by the procedures of Section 9.1.1
> of [RFC6513]" or are you actually defining new normative behaviour?
>
GIM>> I think that the use of lower case 'must' is ambiguous and somewhat
confusing. You are right, the intention is to refer to Section 9.1.1 as the
mandatory behavior. But neither 9.1.2, nor 9.1.3 use the normative
language. What would you recommend?

>
> ---
>
> Section 3
>
> s/tunnel' state/tunnel's state/
>
GIM>> Fixed

>
> ---
>
> Section 3.1 has
>
>    The
>    optional procedures proposed in this section also allow that all
>    downstream PEs don't apply the same rules to define what the status
>    of a P-tunnel is (please see Section 6)
>
> A little confusing. Maybe...
>
>    The
>    optional procedures described in this section also handle the case
>    the downstream PEs do not all apply the same rules to define what the
>    status of a P-tunnel is (please see Section 6)
>
GIM>> Great suggestion and gladly applied to the text.

>
> ---
>
> 3.1.2
>
>    A condition to consider a tunnel status as Up can be that the last-
>    hop link of the P-tunnel is Up.
>
> I like that you are using "Up" rather than "up". Maybe change throughout
> the document to use "Up" and "Down"?
>
GIM>> I went through the document and uppercased when referring to the
status or state. I hope I've got them all.

>
> ---
>
> 3.1.6
>
> s/TLV 's Type/TLV's Type/
>
GIM>> Got it!

>
> ---
>
> 3.1.6.1
>
> You use "p2mp BFD Session" rather than using "P2MP". This looks
> intentional but also looks really odd. Section 7.2 uses "P2MP
> BFD Session".
>
GIM>> Changed to P2MP through the document (also added to the Acronyms)

>
> ---
>
> 3.1.7
>
> s/section 6.8.17 [RFC5880]/Section 6.8.17 of [RFC5880]/
>
> ---
>
> 4.
>
> s/section 5.1.3 [RFC6513]/Section 5.1.3 of [RFC6513]/
>
GIM>> Done.

>
> OLD
>  VPN routes (VPN-IPv4 or VPN-IPv6) routes
> NEW
>  VPN routes (VPN-IPv4 or VPN-IPv6)
> END
>
GIM>> Thank you.

>
> ---
>
> 4.
>
> s/would refer to/refers to/
>
GIM>> Thanks!

>
> ---
>
> 4.1
>
>    As long as C-S is reachable via the Primary
>    Upstream PE and the Upstream PE is the Primary Upstream PE.
>
> This sentence doesn't seem to be complete. What is the consequence of
> this condition?
>
GIM>> It suppose to be
   As long as
   C-S is reachable via the Primary Upstream PE, the Upstream PE is the
   Primary Upstream PE.
Is it better?

>
> ---
>
> 4.1
>
>    o  SHOULD carry the "Standby PE" BGP Community (this is a new BGP
>       Community.
>
> I think this needs guidance on when to not include the Community
>
GIM>> The question made me think that for the implementation that supports
this specification there's no reason not to include the Standby PE
Community. s/SHOULD/MUST/

>
> ---
>
> 4.1
>
>    Also, a LOCAL_PREF attribute MUST be set to zero.
>
> Maybe...
>
>    The LOCAL_PREF attribute MUST also be set to zero.
>
GIM>> Agree.

>
> ---
>
> 4.2
>
> You might want to tidy up whether you use "a)" and "b)" or "(a)" and
> "(b)"
>
GIM>> I opted for 'a)' and 'b)'

>
> ---
>
> 4.4.1
>
> s/Additionally, to?Additional to/
>
GIM>> Got it

>
> ---
>
> 4.4.2
>
>    When an Upstream ASBR receives a C-multicast route, and at least one
>    of the RTs of the route matches one of the ASBR Import RT, the ASBR,
>    that supports this specification, MUST locate an Inter-AS I-PMSI A-D
>    Route whose RD and Source AS respectively match the RD and Source AS
>    carried in the C-multicast route.  If the match is found, and the
>    C-multicast route carries the Standby PE BGP Community, then the ASBR
>    MUST perform as follows:
>
> Is that "MUST try to locate"? Because it seems to be countenanced that
> the attempt could fail.
>
GIM>> Thank you for pointing this out. Yes, the lookup might fail.
s/MUST/MUST try to/

>
> ---
>
> 4.4.2
>
> s/MED attribute set of/MED attribute set to/
>
GIM>> Thank you!

>
> ---
>
> 5.
>
>    The mechanisms defined in sections Section 4 and Section 3 can be
>    used together as follows.
>
> That's an XML feature. If you do
> "...defined in <xref target="section4"/><xref target="section3"/>..."
> then XML2RFC will sort things out for you.  Seems to be OK a couple of
> paragraphs later.
>
GIM>> Cleaned up

>
> ---
>
> 5.
>
> s/semantic for is that/semantic is that/
>
GIM>> Done

>
> ---
>
> 6.
>
>    Multicast VPN specifications [RFC6513] impose that a PE only forwards
>    to CEs the packets coming from the expected Upstream PE
>    (Section 9.1).
>
> There being no section 9.1 in this document, I think you mean...
>    "(see Section 9.1 of [RFC6513])."
>
GIM>> Yes, added the reference to RFC 6513

>
> Please also be clear in the next paragraph whether the references are to
> sections of this document (no need to qualify) or sections of RFC 6513
> (important to qualify).
>
GIM>> Thank you for the question. Both references are to sections in this
document.

>
> ---
>
> 6.
>
> OLD
>    We highlight the reader's attention to the fact that the respect of
> NEW
>    We draw the reader's attention to the fact that the respect of
> END
>
GIM>> Thank you. "Highlighting" attention - awkward.