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

[Greg Mirsky <[e-mail]>]

Hi Adrian,
thank you for the review, detailed questions, and helpful suggestions. I'll
work through and respond within several days.

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