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 -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
