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

Ahmed Bashandy <abashandy.ietf@gmail.com> Sat, 06 February 2021 19:11 UTC

Return-Path: <abashandy.ietf@gmail.com>
X-Original-To: rtg-dir@ietfa.amsl.com
Delivered-To: rtg-dir@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 7E8BD3A36FE; Sat, 6 Feb 2021 11:11:20 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.099
X-Spam-Level:
X-Spam-Status: No, score=-2.099 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, NICE_REPLY_A=-0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id peV7yWfPD91Q; Sat, 6 Feb 2021 11:11:18 -0800 (PST)
Received: from mail-pj1-x1033.google.com (mail-pj1-x1033.google.com [IPv6:2607:f8b0:4864:20::1033]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 3D8843A3698; Sat, 6 Feb 2021 11:11:18 -0800 (PST)
Received: by mail-pj1-x1033.google.com with SMTP id q72so5403405pjq.2; Sat, 06 Feb 2021 11:11:18 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding:content-language; bh=R3rx3x1mOzK43CvzMTJwvdd64/xIvkhkdgSQ/2Uu5tI=; b=oCAanF3fNu1pq5xouHZAw501v1hdXAHn3zM6kelX/fXIX/1P6Y53JoVUs36SlHrJef 9KUYXfibtJW4TTQ+O71XaUN9/SN2illLXawFc9bWL3KTPBGeVqj20QjOIaIpwaGKEmdN W53ekjmL4RIbE8+Pk6R9mVkfe6eI3nffGAfAaealkxLN6sMCgpU8xbV+73Ia1PsMFRNj ilyipfW6o5d3hqgBOo/jwuDqcHGTZj+yn3naVaLoOHLmAXU+TIPKzjXbVGzhTpYOsZD7 JHakGJ8jPtIfAfZ8Vit8WYT/WPU+/f34ONcc2QD4UatE/9b6vSsjItnFXMqePNIfwpBG FYDA==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding :content-language; bh=R3rx3x1mOzK43CvzMTJwvdd64/xIvkhkdgSQ/2Uu5tI=; b=lMEzwEZ8BvQjYgvg0ilmRKGZCr9GYX1rSYsqEnCwUnck8cc9R0YYmIjN3+BuvgNlz4 6HsBJlR6QpwqXtWvWUapS0zQdk1oorBMlk+W+NZQem7DQCBLCT7ClMO1TPKM0bT2RkHX juABlZIbavaeQEMqN5p3By5jDg82TPAw5JTDHuT1+NbmdsqxBssaSldvaNEkUf0GiRcD Qq2OGT/e0usP6YQ/dc2Sbs31j8JmJyuPoAbWVL5nzTLNKptaS87S4rrxGjuCx7e5AGYy uU0oyvnyP4aVwqG5SvI0PeUp9be4SeDjBwsdq+qXmWqxjjt5MZwPCyBivMdCvX6UUwna SyTw==
X-Gm-Message-State: AOAM531l6JZYM1CYRKD2yMMUSGOY7EluW1hdsOMJAAaoyodw8bIAaZfH TDzPVGQ8+7+xZ4qmorw2nMdK80yUu60=
X-Google-Smtp-Source: ABdhPJx60GqNSdaVDEBdwqyX0R/pVcIPBJ4Q5+bN7pgoIqnrOWtOjvRgHi+G/YyuYOFG8Jej+XcVmw==
X-Received: by 2002:a17:90b:1a89:: with SMTP id ng9mr9555575pjb.201.1612638677234; Sat, 06 Feb 2021 11:11:17 -0800 (PST)
Received: from [192.168.50.246] (c-73-189-164-225.hsd1.ca.comcast.net. [73.189.164.225]) by smtp.gmail.com with ESMTPSA id k128sm14016476pfd.137.2021.02.06.11.11.15 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 06 Feb 2021 11:11:16 -0800 (PST)
To: Bruno Decraene <bruno.decraene@orange.com>, rtg-dir@ietf.org
Cc: draft-ietf-rtgwg-bgp-pic.all@ietf.org, last-call@ietf.org, rtgwg@ietf.org
References: <161194636226.19136.6263568097212297078@ietfa.amsl.com>
From: Ahmed Bashandy <abashandy.ietf@gmail.com>
Message-ID: <55a1c403-8420-1bcc-847a-f0988b3529fa@gmail.com>
Date: Sat, 06 Feb 2021 11:11:15 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.7.0
MIME-Version: 1.0
In-Reply-To: <161194636226.19136.6263568097212297078@ietfa.amsl.com>
Content-Type: text/plain; charset="utf-8"; format="flowed"
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Archived-At: <https://mailarchive.ietf.org/arch/msg/rtg-dir/cByb3QHfjoVZj3xFIcRElxssNuU>
Subject: Re: [RTG-DIR] Rtgdir last call review of draft-ietf-rtgwg-bgp-pic-12
X-BeenThere: rtg-dir@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Routing Area Directorate <rtg-dir.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/rtg-dir>, <mailto:rtg-dir-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/rtg-dir/>
List-Post: <mailto:rtg-dir@ietf.org>
List-Help: <mailto:rtg-dir-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/rtg-dir>, <mailto:rtg-dir-request@ietf.org?subject=subscribe>
X-List-Received-Date: Sat, 06 Feb 2021 19:11:21 -0000

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