Re: RtgDir review: draft-ietf-rtgwg-bgp-pic-00

["Ahmed Bashandy (bashandy)" <[e-mail]>]

Hi,

Thanks a lot for the detailed review.

I submitted version 01.  I CCed Jeff and Rob Shakir, who volunteered to 
be a shepherd (Thanks a lot):):)

I have restructured the document to address your comment. Besides I went 
over all the "Minor issues" as well as "nits" and corrected them, except 
1-2 comments which I explained why I did not make the modifications.

Version 01 is modified and now it contains an "overview" section. Hence 
all sections have been shifted by one. So for example, the comment about 
section 2.3.3 now applies to section 3.2

Please see my reply inline  starting with "#Ahmed"

Thanks

Ahmed


On 4/20/2016 6:53 AM, [e-mail] wrote:
>
> 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-00
> Reviewer: Bruno Decraene
> IETF LC End Date: “QA review” pre WG LC
> Intended Status: Informational
>
> *Summary:*
>
> I have some minor concerns about this document that I think should be 
> resolved before publication.
>
> *Comments:*
>
> - Document is interesting. Document is relatively clear but sometime 
> it feels like there is a little room for some reformulation/edition to 
> improve fluidity. In particular, the learning curve is a bit steep at 
> the beginning of the doc as most of the concepts are introduced in 3 
> pages (pages 4-6) in the form of a list of terminology and a pseudo 
> code. I would find useful to have an overview section just after the 
> introduction, with a high level view of the solution with a limited 
> number of new terms.
>
> - The text feels like authoritative, while probably many terms are 
> implementation specific. A priori, I would not expect all 
> implementation of BGP PIC to use the same terms, and possibly not the 
> same data structure. May be the text could be generalized to cover 
> multiple implementations; or modified to describe a generalized 
> concept (i.e. data-structure designed to share as much data as 
> possible between elements, at the cost of additional indirections); or 
> the document could state that it describes a specific implementation 
> with implementations specific terminology, data structure, and 
> specifics. Or a combination of both (e.g. adding a section being both 
> generalized and describing the concept, and then the existing sections 
> after stating that they are specific to one implementation).
>
#Ahmed: I have restructured the document to make much more general. 
Please take a look. All feedback is most welcomed.
>
> *Minor Issues:*
>
> - I find figure 2 very useful to understand the data-structure. I 
> would move it sooner in the doc, somewhere before §2.2. (with its 
> subsequent text below) e.g. a new §2.2 "FIB data-structure"
>
> It would need to be generalized i.e. example non-specific. I could 
> think of:
>
> IP Leaf:      Pathlist:       IP Leaf:                Pathlist:
>
> --------      --------- -------                 --------
>
> BGP NLRI ---> BGP NH1   ----> IGP IP1 (BGP NH1)  ---> IGP NH1, I1  
> ---> Adjacency1
>
> BGP NHi   --...                         IGP NHi, Ii  --..
>
> |                                    |
>
> |                                         |
>
> |                                         |
>
> v                                         v
>
>           OutLabel Array:                           OutLabel Array:
>
> --------------                            --------------
>
>           L (NLRI, NH1)                             L (IP1, NH1)
>
> L (NLRI, NHi)                             L (IP1, NHi)
>
> - Figure 1 could be enhanced with IGP-NH1, IGP-NH2, I1 and I2.
>
#Ahmed: Corrected

> - Example 3 does not use the same naming convention than examples 1 
> and 2, this make it harder to follow for a priori no reason. e.g. VPN 
> labels are named VPN-L11 in examples 1 and 2, but are named 
> VPN-PE21(P1) in exmaple 3; transport labels are named LDP-L12 in 
> exmaples 1 and 2, but LASBR11(PE22) and L11 in figure 3.
>
#Ahmed: I renamed the VPN labels to conform to the convention used in 
Examples 1 and 2. Also All IGP labels were renamed to IGP-Lij to make 
them applicable to both LDP and Segment routing.
For ASBR labels, we need to differentiate between ASBR labels and IGP 
labels. Hence the labels advertised by ASBRs are named differently.

> - §2.3.3
>
> "The local labels of the next hops".
>
>  - All labels are locally assigned. So what do you mean by "local"
>
> - "next-hop" sometimes refers to IGP/connected next-hop (a priori the 
> case here) and sometimes to BGP next-hop. I find it hard to follow. I 
> rather use a different name (e.g; connected next-hop vs BGP next-hop)
>
#Ahmed: Corrected.
>
> - §3
>
> "the hashing at the BGP level yields path 0 while the hashing at the 
> IGP level yields path 1. In that case, the packet will be sent out of 
> interface I1 with the label stack "LDP-L12,VPN-L21".
>
> Does not seem to match my understanding. For "LDP-L12,VPN-L21" I would 
> assume BGP used path index 1 and IGP used path index 0.
>
#Ahmed: Corrected.
>
> IMHO:
>
> OLD: "Hence ASBR22 swaps "LASBR22(PE22)" with the LDP/SR label of 
> PE22, pushes the label of the next-hop towards PE22 in domain 2, and 
> sends the packet along the shortest path towards PE22."
>
> NEW: "Hence ASBR22 swaps "LASBR22(PE22)" with the LDP/SR label for 
> PE22 advertised by the next-hop towards PE22 in domain 2, and sends 
> the packet along the shortest path towards PE22."
>
> (in all cases "swaps" then "pushes" would increase the label stack by 
> 1, which is not the case.)
>
#Ahmed: Corrected
>
> §4.1
>
> "the useable paths in the loadinfo"
>
> loadinfo is a proprietary FIB datastructure which has not been 
> introduced/defined. You need to either remove that term (if possible) 
> or define it somewhere.
>
#Ahmed: Corrected. "loadinfo" is replaced with "pathlist", which is the 
intended term
>
> "Hence traffic restoration occurs within the time frame of IGP 
> convergence,"
>
> agree.
>
> ..."and, for local link failure, within the timeframe of local 
> detection. Thus it is possible to achieve sub-50 msec convergence as 
> described in [10] for local link failure"
>
> IMO, this is restricted to specific cases. e.g. external (eBGP) link 
> failure, ECMP case, possibly IP FRR.  So possibly
>
> OLD: for local link failure, within the timeframe of local detection. 
> Thus it is possible to achieve sub-50 msec convergence as described in 
> [10] for local link failure
>
> NEW: for local link failure, assuming a backup path has been 
> precomputed, within the timeframe of local detection (e.g. 50ms). 
> Example of solutions precomputing a backup path are IP FRR [LFA], 
> [RLFA], [MRT], [TI-LFA] or eBGP path having a backup path [10].
>
#Ahmed: Corrected
>
> §4
>
> I would find useful to indicate, for each type of failure, the number 
> of data-structure that need to be updated.
>
#Ahmed: In general it is not always possible to know the number of 
updated data structures due to a topology change because it depends on 
the topology itself. For example, in Section 5.1 (The section about 
BGP-PIC core), it is not possible to outline the datastructures that are 
modified for a remote link failure. However for local link failure, we 
added a statement in second paragraph of Section 5.1 indicating that 
only pathlists with paths using the local link need to be updated. We 
also indicated in the 3rd paragraph that datastructures are modified 
starting from the IGP leaves without walking back to BGP pathlists or 
data structures
>
> ---
>
> §4.2.2
>
> "To avoid loops, ePE2 MUST treat any core facing path as a backup
>
>       path, otherwise ePE2 may redirect traffic arriving from the core
>
>       back to ePE1 causing a loop."
>
> Looks a bit under-described to me. Could you please elaborate a bit? 
> In particular:
>
> - if 2 PE (PE1, PE2) are connected in U to 2 P (P1, P2)     
> (P1-PE1-PE2-P2), PE1 being nominal and PE2 only used in backup, in the 
> nominal situation, if the core network sends the trafic to PE1 via PE2 
> (used as a P/transit), how does PE2 know that it must send this 
> traffic to PE1? (rather than CE2)
>
> - this behavior looks like an additional specific feature. How doew 
> ePE1 knows that ePE2 have this feature?
>
> ---
>
#Ahmed: The statement is removed. As mentioned in Section 6.1 how to 
avoid loops in case of CE node failure is a different topic and needs to 
be addressed separately
>
> §4.3
>
> "  Hence if the platform supports the "unflattened" forwarding chain,
>
>    then a single pathlist needs to be updated while if the platform
>
>    supports a shallower forwarding chain, then two pathlists need to be
>
>    updated."
>
> IINM "single"  and "two" pathlist applies to the specific example. In 
> this last sentence/summary, I'd prefer a more general statement. A 
> priori, without digging too much in this most complex use case, it 
> seems like :s/single/o(1)  :s/two/o(PE) . The former looks close 
> (single vs o(1)) but IMHO there is a significant difference between 2 
> and o(PE) (i.e. 100s)
>
#Ahmed: Agreed. The last paragraph is generalized and modified to 
indicate possible outcomes of flattenning forwarding chains
>
> ---
>
> §5.1
>
> Good paragraph. It's quite clear that the convergence time does not 
> depend on the number of BGP prefixes, which is good. For the benefit 
> of the reader, it would be even more interesting if, for each type of 
> failure, the text could indicate on what it depends. e.g.  o(1), 
> o(connected interfaces), o(PE), o(PEnominal*PEbackup)....
>
#Ahmed: Each subsection indicates the convergence time dependency. For 
example, in Section 6.1.1, the last paragraph says that the dependence 
is on the IGP convergence time only
>
> --
>
> §7
>
> "No additional security risk is introduced by using the mechanisms 
> proposed in this document"
>
> In general, with such a sentence, it's difficult to evaluate whether 
> the authors have very quickly evaluated the risk or if this evaluation 
> has been performed in details. So in general, some more text detailing 
> which aspects have been evaluated is interesting for the reader (yet 
> painful for the authors).
>
> As the document describe an internal box behavior, this is difficult 
> to evaluate and discuss. But from a bad experience, I fear that there 
> may be an impact. Indeed, with such structure, the FIB 
> structure/memory is typically different between BGP prefixes and IGP 
> prefixes. In general, the implementation is designed to support the 
> "right" numbers of both. But assuming an accident or an attack, the 
> numbers may not be "right". e.g. one upon a time, someone has 
> redistributed the BGP table into the IGP. In this case, the total 
> number of IP prefixes in the FIB is exactly the same. But as the data 
> structure used in the FIB was different between BGP and IGP prefixes, 
> the FIB ran out of memory and the line card crashed (well actually 
> only the IP FIB, so IS-IS hello packet were still correctly sent and 
> forwarded. As a result, traffic was permanently black holed)
>
#Ahmed: I added a statement indicating that the behavior is internal to 
the router.
On another front, although the scenario that you mentioned is related to 
reliability rather than security, it indicates that BGP-PIC actually 
improves the reliability of FIB because it greatly reduces the use of 
hardware and software memory
>
> ---
>
> § 9
>
> OLD: that allows achieving prefix independent convergence
>
> NEW: that allows achieving BGP prefixes independent convergence
>
> (it's still depend on the number of IGP prefixes and/or BGP pathlist)
>
#Ahmed: Corrected
>
> *Nits:*
>
> Abstract
>
> "via more than one path."
>
> In this 1rst sentence, it's not clear what path really means. (e.g cf 
> the terminology section where you have more than one). I guess that 
> you mean "BGP path". (as there are also typically multiple IGP path to 
> reach each BGP Next Hop)
>
#Ahmed: modified to next-hop to indicate BGP next-hops
>
> "The objective is achieved through organizing the forwarding chains"
>
> "chain" does not self self explicit to me. what about :s/chains/data 
> structure"
>
#Ahmed: Agreed, changed to "data structures"
>
> "complete transparency"
>
> what do you mean? transparency to what / from who?
>
#Ahmed: Removed the word "transparency
>
> §1
>
> OLD: to allow for more than one path for a given prefix
>
> NEW: to allow for BGP to advertise more than one path for a given prefix
>
#Ahmed: Modified as suggested
>
> OLD: Another more common and widely deployed scenario is L3VPN with 
> multi-homed VPN sites
>
> NEW: Another more common and widely deployed scenario is L3VPN with 
> multi-homed VPN sites with unique Route Distinguisher.
>
#Ahmed: Modified as suggested
>
> ---
>
> §1.2
>
> "Pathlist: It is an array of paths"
>
> "OutLabel-Array: The OutLabel-Array is a list of one or more outgoing 
> labels "
>
> So a list is defined as an array and the array is defined as a list :-).
>
> What about using the same term, e.g. a list?
>
#Ahmed: Corrected. Both are now pathlist and OutLabel-List:)
>
> --
>
> The OutLabel-Array is a list of one or more
>
>       outgoing labels and/or label actions where each label or label
>
>       action has 1-to-1 correspondence to a path in the pathlist. It
>
>       is possible that the number of entries in the OutLabel-array is
>
>       different from the number of paths in the pathlist and the ith
>
>       Outlabel-Array entry is associated with the path whose path-
>
>       index is "i".
>
> - I don't see how one can have a 1-to-1 correspondance if the number 
> of elements is not the same.
>
#Ahmed Corrected.
>
> - Last sentence could be splitted in 2.
>
> -- 
>
> Since the term ingres PE is defined, you could also detine the term 
> egress PE. Possibly in the same sentence.
>
> OLD: "Ingress PE, "iPE": It is a BGP speaker that learns about a
>
>       prefix through another IBGP peer and chooses that IBGP peer as
>
>       the next-hop for the prefix
>
> NEW:      "Ingress PE, "iPE": It is a BGP speaker that learns about a
>
>       prefix through a IBGP peer and chooses an egress PE as the 
> next-hop for the prefix.
>
#Ahmed: Modified as suggested
>
> As a side note, the previous definition assume that there were no 
> Route Relfector (the iBGP peer is the BGP Next Hop)
>
#Ahmed: Correct. I did not want to unnecessarily complicate the 
discussion with information
>
> -- 
>
> §2.3
>
> Figure 1 represents a VPN network with 3 PE and a CE. In this context, 
> "VPN-P1" sounds a bit like a P router. What about :s/VPN-P1/VPN-IP1 ? 
> Same comment for IGP-P1.
>
#Ahmed: Agreed. All modified as suggested
>
> --
>
> §2.3.2
>
> OLD: ePE2 constructs the forwarding chain depicted in Figure 1
>
> NEW: ePE2 constructs the forwarding chain depicted in Figure 3
>
#Ahmed: Corrected
>
> OLD: VPL-L11
>
> NEW: VPN-L11
>
#Ahmed: Corrected
>
> §2.3.3
>
> OLD: can reach ASBR1
>
> NEW: can reach ASBR11
>
#Ahmed: Corrected
>
> OLD: The label for advertised by ASBR11 to iPE
>
> NEW: The label advertised by ASBR11 to iPE
>
> OLD: The labels for advertised by ASBR12 to iPE
>
> NEW: The labels advertised by ASBR12 to iPE
>
#Ahmed: Corrected
>
> OLD: The labels for advertised to iPE by ASBR11 using BGP-LU
>
> NEW: The labels advertised  by ASBR11 to iPE using BGP-LU
>
#Ahmed: Corrected
>
> ---
>
> §3
>
> OLD: Let's applying the above forwarding steps to the example 
> described in Figure 1 Section 2.3.1.
>
> OLD: Let's applying the above forwarding steps to the example 
> described in Figure 2 Section 2.3.1.
>
#Ahmed: Corrected
>
> (somewhat guesssing. But in all cases, there is no figure 1 in section 
> 2.3.1)
>
> ---
>
> §4.1
>
> IMO
>
> OLD: As soon as the IGP convergence is effective for the BGP nhop 
> entry, the new forwarding state is immediately available to all 
> dependent BGP prefixes.
>
> NEW: As soon as the IGP convergence is effective for a BGP next-hop 
> entry, the new forwarding state is immediately available to all 
> dependent BGP prefixes.
>
> more generally
>
> :s/nhop/next-hop
>
#Ahmed: Replaced nhop with next-hop in the entire document
>
> ---
>
> §4.3
>
> :s/PE222/PE22
>
#Ahmed: Corrected
>
> Best regards,
>
> Bruno
>
> **
>
> _________________________________________________________________________________________________________________________
>
> 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.