Thanks Ahmed. Looks good to me. --Bruno From: Ahmed Bashandy [mailto:[email protected]] Sent: Saturday, August 21, 2021 2:34 AM To: DECRAENE Bruno INNOV/NET <[email protected]>; [email protected]; Prodosh Mohapatra <[email protected]>; [email protected] Cc: [email protected]; [email protected]; [email protected] Subject: Re: Rtgdir last call review of draft-ietf-rtgwg-bgp-pic-12
Thanks a lot for the comments. See responses inline #Ahmed. They refer to version 15 which I just published to address your comments as weel the comments of other reviewers. Thanks Ahmed On 1/29/21 10:52 AM, Bruno Decraene via Datatracker wrote: Reviewer: Bruno Decraene 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-rtgwg-bgp-pic-12.txt Reviewer: Bruno Decraene Review Date: 2021-01-29 IETF LC End Date: not started. (in WG LC) Intended Status: Informational Summary: I have some minor concerns about this document that I think should be resolved before publication. Comments: Draft is clear and very readable although the subject is a bit complex and usually not well known from IETF readers/documents as the described behavior is local to a node and largely implementation specific. The terminology and some text are very implementation dependent. As such, the document may not age well and may not map directly to other implementations. May be the abstract / introduction could clarify that the document is partly an architecture and partly a description of one specific implementation. #Ahmed. I added a sentence at the end of the introduction It is noteworthy to mention that this document describes FIB architecture that can be implemented in both hardware and/or software. ================= Minor: ================= References: LDP and SR-MPLS are used interchangeably in the text. There doesn't seem to be a reason for the former to be a normative reference while the latter an informative reference. #Ahmed: I made the SR also normative ---- IdNits has the following complaint: "The document has examples using IPv4 documentation addresses according to RFC6890, but does not use any IPv6 documentation addresses. Maybe there should be IPv6 examples, too?" It looks a priori easy to fix. e.g. replacing the second IPv4 route (e.g. 65000: 203.0.113.0/24) by an IPv6 one. #Ahmed: I added a separate example at the end of the section to avoid any confusion ----- Introduction I feel that the introduction could introduce both PIC core and PIC edge, rather than just "PIC" #Ahmed: IMHO the objective of an introduction to present a very high level description of the idea. The distinction BGP-PIC Core vs BGP-PIC edge requires details explanation without which it will cause more confusion than clarification. In fact the reason I put BGP-PIC core vs BGP-PIC edge in section 6 is that IMHO a lot has to be explained before these two terms make sense ----- §1.1 The terminology section would benefit from adding the terms "PIC Core" and "PIC Edge" (e.g. using the first sentences of sections 6.1 & 6.2) #Ahmed: Again the previous response applies. ------ §1.1 "IGP prefix: A prefix P/m (of any AFI/SAFI) that is learnt via an Interior Gateway Protocol, such as OSPF and ISIS, has a path for. " A priori incorrect grammar. You could remove either ", has a path for" or "is learnt via" #Ahmed: removed the phrase "has a path for." ----- "o Egress PE, "ePE": A BGP speaker that learns about a prefix through an eBGP peer and chooses that eBGP as the next-hop for that prefix." May be OLD: that eBGP NEW: that eBGP peer #Ahmed: Done ------ §1.1 "Adjacency" definition could be moved before the definition of "Primary path" since the definition of the latter use the former. #Ahmed: Done ------- §1.1 "Forwarding chain" is used in some definition but not defined beforehand. #Ahmed: I added an item to describe it in the Terminology section ------- §2 "It is not uncommon to see multiple destinations are reachable via the same list of next-hops." A priori incorrect grammar. OLD: are reachable could be "that are reachable" or "reachable" #Ahmed: corrected ------- 2.1.2 Another option to learn multiple BGP NH/path is simply to receive IBGP paths from multiple BGP RR selection a different path as best.(and hence reflecting a different path). #Ahmed: Thanks a lot. I added the sentence to the second paragraph in section 2.1.2 ------- §2.2 "2 ECMP paths with IGP-NH1 and IGP-NH2 reachable via the interfaces I1 and I2, respectively." I don't precisely understand the sentence. "Respectively" means that I1 (resp. I2) is used to reach IGP-N1 (resp. IGP-N2). Soe where are the 2 ECMP paths? Note that given the data in Figure 2, my understanding is that there are only 2 interfaces (I1 and I2) used by both IGP prefixes. Hence ", respectively" to be removed. #Ahmed: May be the confusion comes from using "ECMP" I modified the text to say "2 equal cost paths" It seems that either "respectively" is to be removed, or that 2 interfaces are missing (2ECMP*2 destinations) For better illustration, Figure 1 could be extended by adding the connected interfaces and neighbors of iPE. Also the routing table could mention those interfaces OLD: 192.0.2.1/32 via Core, Label: IGP-L11 via Core, Label: IGP-L12 192.0.2.2/32 via Core, Label: IGP-L21 via Core, Label: IGP-L22 NEW: 192.0.2.1/32 via I1, Label: IGP-L11 via I2, Label: IGP-L12 192.0.2.2/32 via I1, Label: IGP-L21 via I2, Label: IGP-L22 #Ahmed: I modified the example as you suggested (thanks a lot) All of the above to be corrected based on the response to the first comments. (are there 2 or 4 interfaces?) ------- § Figure 2 The draft uses NH1 for both BGP-NH1 and IGP-NH1 and they represent different things. Some parts of Figure sometimes uses "NH1" alone (in OutLabel-List) which is ambiguous. Also :s/BGP NH1/BGP-NH1 :s/IGP NH1/IGP-NH1 #Ahmed: corrected as suggested ------- §2.2 "For example, if the interface "I2" goes down, only the shared IGP pathlist needs to be updated," It seems to me that the IGP OutLabel-List would also need to be updated. #Ahmed: No. If we modify the shared IGP pathlist by removing the path that uses I2, then only the first path will be used for forwarding and hence the second label in the OutLabel list will never be used. Of course IGP will later update the prefix to remove the failed path together with the second label. ------ §2.2 Figure 2 (and may be section 2.2) does not refer to local protection (IGP FRR, eBGP FRR). I'd guess that some of the Paths are marked as backup. If so, I would suggest to add this. #Ahmed: This section is intended to give a brief illustration of the idea of BGP-PIC. We are already giving detail example for active/backup in section 6.2.1 and 6.2.2. So IMHO there is no need for another example here ------- §6.2.1 "IGP running on the iBGP peers instructs FIB to remove the IP" Instead of "iBGP peers" I think that you have introduced the term "iPE" just for the purpose. (alternative "iBGP routers" as "iBGP peers" raise the question of "peers of who?" which introduce complexity if one wants to accommodate for Route Reflectors) #Ahmed: corrected as suggested (thanks a lot) ------- §3.2 In figure 3, " VPN-L11 +---------+ (Label-leaf)---+---->|Unlabeled| | +---------+ | | VPN-L21 | | | (swap) | | +---------+ | | BGP Pathlist | +------------+ Connected route | | CE-NH |------>(to the CE) | |path-index=0| | +------------+ | | VPN-NH2 | VPN-IP1 -----+------------------>| (backup) |------>IGP Leaf " It's not obvious whether the long vertical line should be read from Up to down of from Down to up, as there is no arrow on it. My understanding is that it should be read up to down. In which case, adding an arrow would help. e.g. VPN-L11 (Label-leaf)---+----> | V | #Ahmed: Corrected as suggested ------- §5.1 "a sample scenario" Did you mean :s/sample/simple ? #Ahmed: I meant sample. But I modified it to "example" to avoid the confusion ------- §5.2 "the VPN prefix VPN-IP1 and VPN2-IP2." The figure uses "VPN-IP2" (VPN- vs VPN2-). May be :s/prefix/prefixes #Ahmed: corrected "VPN2-" to "VPN-", and replaced prefix with prefixes ------- §5.2 " o Both the routers ASBR21 and ASBR22 of Domain 2 advertise the same label LASBR21 and LASBR22 to the egress PEs ePE1 and ePE2, respectively, to the routers ASBR11 and ASBR22 of Domain 1." may be "advertise the same label LASBR21 and LASBR22 for the egress PEs ePE1 and ePE2" (:s/to/for) #Ahmed: Corrected ------- §5.2 OLD: 65000: 203.0.113.0/24 via ePE1 (192.0.2.2), VPN Label: VPN-L22 via ePE2 (192.0.2.3), VPN Label: VPN-L32 NEW: 65000: 203.0.113.0/24 via ePE2 (192.0.2.2), VPN Label: VPN-L22 via ePE3 (192.0.2.3), VPN Label: VPN-L32 (:s/PE1/PE2 :s/PE2/PE3 ) #Ahmed: Corrected (thanks for catching) ------- §6.2.1 "hence the label VPN-L12 will be pushed" In figure 2, I believed you used the name "VPN-L21" (L21 vs L12) #Ahmed: Corrected (thanks for catching) ------- §6.2.2 "Hence traffic will be forwarded used the backup path towards ePE2." :s/used/using #Ahmed: corrected ------- §6.2.2 " FIB manager on the edge router detecting the link failure applies the following steps" May be you could add a reference to Figure 3 which precisely describes this case. (otherwise the reader keeps using Figure 2 which is less appropriate for this case. #Ahmed: added Figure 3 as suggested ------- §6.2.2 Possibly the two cases (NH self and not NHS could be described in two sub- sections. (When reading "In the second case", the reader may have lost the context). In which case, the last paragraph "Let's try to apply the case of next-hop self" could be grouped with the "First cast"/sub section. But up to you. This is an editorial choice. #Ahmed: thanks for the comment. I would prefer to leave it as it is ================= Nits: ================= Reference [3]. Somehow, the title and list of authors does not match the ones in the referenced RFC. https://tools.ietf.org/html/rfc8277 #Ahmed: Corrected ------ §1.1 In Pathlist OLD: . "). NEW: . #Ahmed: Corrected ------ §7.1 "This section provides details for each failure and now the hierarchical and shared FIB structure proposed in this document allows recovery that does not depend on number of BGP prefixes." I would assume :s/now/how #Ahmed: Corrected ------ :s/ABSRs /ASBRs #Ahmed: Corrected _________________________________________________________________________________________________________________________ Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration, Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci. This message and its attachments may contain confidential or privileged information that may be protected by law; they should not be distributed, used or copied without authorisation. If you have received this email in error, please notify the sender and delete this message and its attachments. As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified. Thank you.
_______________________________________________ rtgwg mailing list [email protected] https://www.ietf.org/mailman/listinfo/rtgwg
