Re: Rtgdir last call review of draft-ietf-rtgwg-bgp-pic-12

[Ahmed Bashandy <[e-mail]>]

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