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 < nore...@ietf.org> 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 > > >
_______________________________________________ BESS mailing list BESS@ietf.org https://www.ietf.org/mailman/listinfo/bess
