Re: [RTG-DIR] Rtgdir last call review of draft-ietf-rtgwg-bgp-pic-12

[[e-mail]]

Thanks Ahmed.
Looks good to me.
--Bruno

From: Ahmed Bashandy [mailto:[e-mail]]
Sent: Saturday, August 21, 2021 2:34 AM
To: DECRAENE Bruno INNOV/NET <[e-mail]>; [e-mail]; Prodosh Mohapatra <[e-mail]>; [e-mail]
Cc: [e-mail]; [e-mail]; [e-mail]
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.