Thanks a lot for the detailed review I will address the comments shortly 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. ================= 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. ---- 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. ----- Introduction I feel that the introduction could introduce both PIC core and PIC edge, rather than just "PIC" ----- §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) ------ §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" ----- "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 ------ §1.1 "Adjacency" definition could be moved before the definition of "Primary path" since the definition of the latter use the former. ------- §1.1 "Forwarding chain" is used in some definition but not defined beforehand. ------- §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" ------- 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). ------- §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. 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 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 ------- §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. ------ §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. ------- §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) ------- §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 | ------- §5.1 "a sample scenario" Did you mean :s/sample/simple ? ------- §5.2 "the VPN prefix VPN-IP1 and VPN2-IP2." The figure uses "VPN-IP2" (VPN- vs VPN2-). May be :s/prefix/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) ------- §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 ) ------- §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) ------- §6.2.2 "Hence traffic will be forwarded used the backup path towards ePE2." :s/used/using ------- §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. ------- §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. ================= 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 ------ §1.1 In Pathlist OLD: . "). NEW: . ------ §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 ------ :s/ABSRs /ASBRs
-- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
