Subject: Re: [bess] RtgDir Last Call review: draft-ietf-bess-evpn-optimized-ir-09

From: "Rabadan, Jorge (Nokia - US/Mountain View)" <[email-masked]>
Date: Mon, 8 Nov 2021 15:16:16 +0000

Hi Julien,

Thanks very much for the review. Great points.

We took all your suggestions and incorporate them in version 10. Please see the resolutions in-line below with [jorge].
Thanks!
Jorge

-----Original Message-----
From: [email-masked] <[email-masked]>
Date: Thursday, October 14, 2021 at 12:39 PM
To: [email-masked] <[email-masked]>
Cc: [email-masked] <[email-masked]>, [email-masked] <[email-masked]>, [email-masked] <[email-masked]>, [email-masked] <[email-masked]>
Subject: RtgDir Last Call review: draft-ietf-bess-evpn-optimized-ir-09
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. 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-bess-evpn-optimized-ir-09
Reviewer: Julien Meuric
Review Date: 2021-10-12
IETF LC End Date: 2021-09-07
Intended Status: Standards Track

*Summary:*
I think the document is almost ready for publication, but a few
clarification improvements should be considered before.

*Comments:*
First of all, I must say that I'm not an expert on NVO3 nor data
centers. As a result, reading the document implied an extensive use of
the terminology section at the beginning. Once through with this step,
the document appears to be well written and specifications are clear and
well organized. I suggest a few clarifications below.

*Major Issues:*
No major issues found.

*Minor Issues:*
- The terminology section doesn't gather all the terms/acronyms used
along the I-D. Some are expanded on use (not necessarily the 1st one),
some aren't. Among the missing ones I spotted: RD, TS, PMSI, Leaf A-D,
VSID, ES, ESI, DF, NDF...
[jorge] added. Thanks.

- Section 4 includes 2 naked figures. Each of them deserves a title
(beware of the multiple references to the former Figure 1 when renumbering).
[jorge] done. Thanks.

- The PTA's description mentions the T, BM, U and L flags: it would be
valuable to include them on the figure, all the more as their position
is already defined or even pre-existing (L).
[jorge] added, thanks.

- Section 8 tackles a particular case which may arise with "some
software-based or low-end AR-REPLICATOR nodes" which do "not support
more than one IP address". One could wonder if that kind of nodes would
be appropriate to be used as AR-REPLICATORs. Anyway, assuming that
situation can be faced, what is the expected behavior in case of common
Ethernet Segments, i.e. if a BD falls within the scope of both section 8
and section 9? (If nonsense, a sentence might be welcome.)
[jorge] excellent point. I modified section 9.2 and added bullet ‘d’, as follows:

  a.  For BUM traffic received on a local AR-REPLICATOR's AC, "Local-
       Bias" procedures as in [RFC8365] SHOULD be followed.

   b.  For BUM traffic received on an AR-REPLICATOR overlay tunnel with
       AR-IP as the IP DA, "Local-Bias" SHOULD also be followed.  That
       is, traffic received with AR-IP as IP DA will be treated as
       though it had been received on a local AC that is part of the ES
       and will be forwarded to all local ES, irrespective of their DF
       or NDF state.

   c.  BUM traffic received on an AR-REPLICATOR overlay tunnel with IR-
       IP as the IP DA, will follow regular [RFC8365] "Local-Bias" rules
       and will not be forwarded to local ESes that are shared with the
       AR-LEAF or AR-REPLICATOR originating the traffic.

   d.  In cases where the AR-REPLICATOR supports a single IP address,

       the IR-IP and the AR-IP are the same IP address, as discussed in

       Section 8.  The received BUM traffic will be treated as in 'b'

       above if the received VNI is the AR-VNI, and as in 'c' if the VNI

       is the IR-VNI.

- The document describes several procedures in several steps, including
a kind of functional description and a node-centric behavior, which are
complementary and provide some useful rephrasing. However, they
sometimes feel like overlapping a bit (e.g., in section 5.1, item d. and
a few paragraphs further about "BM packet on an overlay tunnel"), or
don't necessarily use the same level of RFC 2119 terminology. Careful
alignment/limited duplication would be welcome there.
[jorge] ok, I aligned the normative language in 5.1, 5.2, 6.1 and 6.2. Thanks.

- Section 5.2 states that "the AR-LEAF can locally select which
AR-REPLICATOR it sends the BM traffic to". It seems that nothing
precludes an on-the-fly change, e.g. for administrative or policy
reasons, but an explicit sentence could make it clearer. A similar
comment applies to Section 6.2.
[jorge] fair point. Added this in both sections:
“An AR-LEAF MAY change the AR-REPLICATOR(s) selection dynamically, due to an administrative or policy configuration change.”

- In section 7, "a node MAY decide to honor the PFL flag". Two
subsequent comments:
  * "a node MAY honor" is enough to describe the expectation,
  * what happens if the node doesn't honor? Is it "free to silently
ignore" the flag?
[jorge] ok, I changed it to:

The ability to signal and process these PFL flags SHOULD be an

   administrative choice.  If a node is configured to process the PFL

   flags, upon receiving a non-zero PFL flag the node will remove the

   sender from the corresponding flood-list.  A node not following this

   document or not configured for this optimization will ignore any of

   the received PFL bits.

*Nits:*

Abstract:
- EVPN isn't expanded at 1st use and isn't marked as well-know by the
RFC Editor (though I agree it could;
https://www.rfc-editor.org/materials/abbrev.expansion.txt).
[jorge] added

- s/PIM (Protocol Independent Multicast) based trees/PIM (Protocol
Independent Multicast)-based trees/
[jorge] fixed

Section 1:
- s/Tenant Systems (TS) reducing/Tenant Systems (TS), reducing/
[jorge] fixed

Section 2:
- BD's expansion should come earlier since it's forward referred to by
some previous items in the list.
[jorge] fixed

- The phrases "ingress traffic" and "ingress packets" are used: wouldn't
"incoming traffic" and "incoming packets" be more appropriate?
[jorge] fixed

Section 3:
- s/3. Solution requirements/3. Solution Requirements/
[jorge] fixed

Section 4:
- s/Attributes for optimized-IR/Attributes for Optimized-IR/
- s/the PE's loopback address/a loopback address of the PE/
- s/different than the IR-IP/different from the IR-IP/
- s/its use SHOULD/their use SHOULD/
[jorge] fixed

Section 5:
- OLD
   Unknown unicast SHALL follow
   the same path as known unicast traffic in order to avoid packet
   reordering for unicast applications and simplify the control and data
   plane procedures.
   Note that known unicast forwarding is not impacted by this solution.
  NEW
   Note that known unicast forwarding is not impacted by this solution,
   i.e. unknown unicast SHALL follow the same path as known unicast
   traffic.
[jorge] done

- s/5.1. Non-selective AR-REPLICATOR procedures/5.1. Non-selective
AR-REPLICATOR Procedures/
- s/ingress BM/incoming BM/
- s/to either AR-REPLICATOR or AR-LEAF/to AR-REPLICATOR or AR-LEAF/
- s/it is RECOMMENDED to use this flag for/this flag is useful for/
["SHOULD set" in the previous sentence is fine, RFC 2119 language to
suggest a use case isn't appropriate]
- s/send all the BM packets to that AR-REPLICATOR/send all the BM
packets targeted to that AR-REPLICATOR/
- s/overlay tunnel, will forward/overlay tunnel, it will forward/
- s/in section Section 5.1./in Section 5.1./
- s/5.3. RNVE procedures/5.3. RNVE Procedures/
[jorge] fixed

Section 6:
- s/the AR-LEAF that requested/the AR-LEAFs that requested/
- s/6.1. Selective AR-REPLICATOR procedures/6.1. Selective AR-REPLICATOR
Procedures/
- s/The Replicator-AR route MUST include/The AR-REPLICATOR MUST include/
- s/AR-LEAF-set (excluding the overlay tunnel to the source
AR-LEAF)./AR-LEAF-set, excluding the overlay tunnel to the source AR-LEAF./
- s/speed up the convergence/speed up the traffic recovery/  [or
"mitigate the traffic impact"]
[jorge] fixed, except for the “Replicator-AR routes” phrase, which is correct.

Section 8:
- s/8. AR Procedures for single-IP AR-REPLICATORS/8. AR Procedures for
Single-IP AR-REPLICATORS/
[jorge] fixed

Section 10:
- s/existance/existence/
- s/this document provides a fall back/this document specifies a fall back/
[jorge] fixed

Regards,

Julien