Last Call Review of draft-ietf-bess-mvpn-fast-failover-11
review-ietf-bess-mvpn-fast-failover-11-rtgdir-lc-farrel-2020-10-19-00

Request Review of draft-ietf-bess-mvpn-fast-failover
Requested rev. no specific revision (document currently at 15)
Type Last Call Review
Team Routing Area Directorate (rtgdir)
Deadline 2020-10-18
Requested 2020-10-03
Requested by Martin Vigoureux
Authors Thomas Morin, Robert Kebler, Greg Mirsky
Draft last updated 2020-10-19
Completed reviews Rtgdir Last Call review of -11 by Adrian Farrel (diff)
Secdir Last Call review of -11 by Daniel Migault (diff)
Secdir Telechat review of -13 by Daniel Migault (diff)
Assignment Reviewer Adrian Farrel 
State Completed
Review review-ietf-bess-mvpn-fast-failover-11-rtgdir-lc-farrel-2020-10-19
Posted at https://mailarchive.ietf.org/arch/msg/rtg-dir/EWyrrBzG9o0MH4w3yzr4cPALJ80
Reviewed rev. 11 (document currently at 15)
Review result Has Issues
Review completed: 2020-10-19

Review
review-ietf-bess-mvpn-fast-failover-11-rtgdir-lc-farrel-2020-10-19

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).


---

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).

---

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".)

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?

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.

To reiterate, "I don't know if it is up" is not the same as "I know it 
is down." 

---

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.

---

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.

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.

---

3.1.6

What should I do if I don't recognise or support the setting of the BFD
Mode field?

---

4.1

   The normal and the standby C-multicast routes must have their Local
   Preference attribute adjusted

Should this be "MUST"?

---

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?

==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.

---

Requirements Language

Please move this to a new section 2.1 to be consistent with the RFC
Editor style guide.

---

Section 1

   In the context of multicast in BGP/MPLS VPNs

That could use a reference.

---

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.

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]."

---

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, ...

---

Section 3

s/Section 5.1 [RFC6513]/Section 5.1 of [RFC6513]/

---

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.

---

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?

---

Section 3

s/tunnel' state/tunnel's state/

---

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)

---

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"?

---

3.1.6

s/TLV 's Type/TLV's Type/

---

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".

---

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]/

OLD
 VPN routes (VPN-IPv4 or VPN-IPv6) routes
NEW
 VPN routes (VPN-IPv4 or VPN-IPv6)
END

---

4.

s/would refer to/refers to/

---

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?

---

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

---

4.1

   Also, a LOCAL_PREF attribute MUST be set to zero.

Maybe...

   The LOCAL_PREF attribute MUST also be set to zero.

---

4.2

You might want to tidy up whether you use "a)" and "b)" or "(a)" and
"(b)"

---

4.4.1

s/Additionally, to?Additional to/

---

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.

---

4.4.2

s/MED attribute set of/MED attribute set to/

---

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.

---

5.

s/semantic for is that/semantic is that/

---

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])."

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).

---

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