Subject: Re: [Last-Call] Rtgdir last call review of draft-ietf-rtgwg-bgp-pic-12

From: Ahmed Bashandy <[email-masked]>
Date: Sat, 6 Feb 2021 11:11:15 -0800

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