From neeraj.ietf@gmail.com Mon Nov 20 12:56:28 2023 Return-Path: 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 10CFCC15C28F; Mon, 20 Nov 2023 12:56:28 -0800 (PST) X-Virus-Scanned: amavisd-new at amsl.com X-Spam-Flag: NO X-Spam-Score: -2.104 X-Spam-Level: X-Spam-Status: No, score=-2.104 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, HTML_MESSAGE=0.001, RCVD_IN_ZEN_BLOCKED_OPENDNS=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01, URIBL_BLOCKED=0.001, URIBL_DBL_BLOCKED_OPENDNS=0.001, URIBL_ZEN_BLOCKED_OPENDNS=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 ([50.223.129.194]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id U8OE_wg2aRYS; Mon, 20 Nov 2023 12:56:24 -0800 (PST) Received: from mail-qt1-x82f.google.com (mail-qt1-x82f.google.com [IPv6:2607:f8b0:4864:20::82f]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 391D7C1522AF; Mon, 20 Nov 2023 12:56:24 -0800 (PST) Received: by mail-qt1-x82f.google.com with SMTP id d75a77b69052e-41cbf31da84so27778921cf.0; Mon, 20 Nov 2023 12:56:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1700513783; x=1701118583; darn=ietf.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=1qhc0rh9x4wbqKimHPFRJ6EJ6AsSaI9lsTVFGErWCyU=; b=gpKpmt0EcFfgnX2QVCzqwLT99pxQ2akW4hidS9DaUAeduVD8UYN8I1tbsLprmq9mlK TC+QW6Pksl+JlJKGJ19CNwfGaqP0Ptw0sUwQMXoOHwfjimfeTcVuJIWiBtg/ASRh92v6 hfkDx8wnRmcO/CfLLCc80QkmC9fJYMxq7raeBYEGm15FPtIEU38qIFu2rgS6kLc7lD2U 8dfxBQJM40mCeg9Tv/+WIoYjbJMh4TL53GOoHnoEoiMhOC6n/wEMOFHM4Qcjv8ckz14p wGDhKyhl1rgMKMgtGcLdQnaFrtj1gYVExPQ4/zNef4qBsJ1gYU0xD4KijgKHHf5ZSLE2 jBFA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700513783; x=1701118583; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=1qhc0rh9x4wbqKimHPFRJ6EJ6AsSaI9lsTVFGErWCyU=; b=b4a9n+hsS0eGwNsW/NhcbrTTfl2SmfMx2oQ0jR1Y/8fSZp3d+OiVQvb96qBAFoua4Y 13hge9HeOxQzsXP1kYe2jVEzKnwkPjqFtDiy6eXQYXEkVgkTsvE/UblKNOiMVerNVxGX mDbNhs6D7kT5SIWifFu6vWxh0sXQk53hV7WhlaxCQBs0PjfMHfmYMz3Tf3sk58mHJNHd xsBa2/WAQKU7JR/QdPHi3pLRfoZ6Hc6Tr7WnbWPw3xJ5A2mbcGVU+PA2o6UoyhvyY83H wQmYo1k5dkVWYQkAV4DjuwVvd6/xkB67/t8qssvnkPRw2zLcAQCjUHP78QOVuUmz5k4t FUmQ== X-Gm-Message-State: AOJu0YxJasHuz1HIHz60NAps9skHEKu6rYWMUb0O6I2hWSF/gSIX8xa0 jJEcqorVNDMjNliaU5MBLsiqyJD/weKqnxZlAo/MKGIuFucc3Q== X-Google-Smtp-Source: AGHT+IFIiybtrYtD+0xKh5cto6v9de88cU7Hq1nRp8wXnf8VXte1E2gkeTBkgOzlX8JHBbjwsJBPqeGtDWSh7TdfFNk= X-Received: by 2002:a05:622a:1386:b0:421:a1ee:a330 with SMTP id o6-20020a05622a138600b00421a1eea330mr12939566qtk.12.1700513782968; Mon, 20 Nov 2023 12:56:22 -0800 (PST) MIME-Version: 1.0 References: <170013249509.30988.13683270341413520261@ietfa.amsl.com> In-Reply-To: <170013249509.30988.13683270341413520261@ietfa.amsl.com> From: Neeraj Malhotra Date: Mon, 20 Nov 2023 12:56:11 -0800 Message-ID: To: Dhruv Dhody Cc: rtg-dir@ietf.org, bess@ietf.org, draft-ietf-bess-evpn-unequal-lb.all@ietf.org Content-Type: multipart/alternative; boundary="000000000000f83c12060a9bb77c" Archived-At: Subject: Re: [RTG-DIR] [bess] Rtgdir early review of draft-ietf-bess-evpn-unequal-lb-18 X-BeenThere: rtg-dir@ietf.org X-Mailman-Version: 2.1.39 Precedence: list List-Id: Routing Area Directorate List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 20 Nov 2023 20:56:28 -0000 --000000000000f83c12060a9bb77c Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Dhruv, Many thanks for the review. Will update the draft and respond by next week. Thanks, Neeraj On Thu, Nov 16, 2023 at 3:01=E2=80=AFAM Dhruv Dhody via Datatracker < noreply@ietf.org> wrote: > Reviewer: Dhruv Dhody > Review result: Has Issues > > # RTGDIR review of draft-ietf-bess-evpn-unequal-lb-18 > > Hello, > > I have been selected to do a routing directorate =E2=80=9Cearly=E2=80=9D = review of this > draft. > https://datatracker.ietf.org/doc/draft-ietf-bess-evpn-unequal-lb/ > > The routing directorate will, on request from the working group chair, > perform > an =E2=80=9Cearly=E2=80=9D review of a draft before it is submitted for p= ublication to the > IESG. The early review can be performed at any time during the draft=E2= =80=99s > lifetime > as a working group document. The purpose of the early review depends on t= he > stage that the document has reached. As this document is > post-working-group-last-call, my focus for the review was to determine > whether > the document is ready to be published. > > For more information about the Routing Directorate, please see > https://wiki.ietf.org/en/group/rtg/RtgDir > > Document: draft-ietf-bess-evpn-unequal-lb-18 > Reviewer: Dhruv Dhody > Review Date: 16 Nov 2023 > Intended Status: Standards Track > > ## Summary: > > The motivation for the draft is clear and described well. However, I have > significant concerns about this document. It needs more work before being > submitted to the IESG. > > ## Comments: > > ### General > > * Request the shepherd to make sure that there is a valid justification > for 6 > authors. Better yet just make it 5 authors (you have 3 authors from the > same > affiliation and one author marked as editor) > > * Please follow the RFC style guide. For instance, the Introduction shoul= d > be > the first section as per - > https://www.rfc-editor.org/rfc/rfc7322.html#section-4.8.1. The best would > be to > have a new Introduction section that briefly introduces the concept and > change > section 2 to "Motivation" or something like that. > > * Use of some words in all capital letters - OR, ALL, NOT. This should b= e > avoided so as not to confuse with RFC2119 keywords which have special > meaning > when in capital. > > * The documents should add references to relevant RFCs when talking about > existing EVPN features. > * IRB > * > > ### Section 1 > > * Please update the Requirements Language template to - > ```` > The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", > "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and > "OPTIONAL" in this document are to be interpreted as described in > BCP 14 [RFC2119] [RFC8174] when, and only when, they appear in all > capitals, as shown here. > ```` > * Please add references for the terms where possible. Definitions such as > "Egress PE" and "Ingress PE" refer to RT-1, RT-2, and RT-5 especially nee= ds > one. Also, can the local PE and Ingress PE be different? > > ### Section 4 > > * Why SHOULD and not MUST in - > ```` > Implementations SHOULD support the default units of Mbps > ```` > * IMHO section 4.2 is better suited in the appendix > > ### Section 5 > > * Section 5.1; Why SHOULD and not MUST? > * Section 5.1; Consider adding the reasoning behind > ```` > EVPN link bandwidth extended community SHOULD NOT be attached to per- > EVI RT-1 or to EVPN RT-2. > ```` > * Section 5.2; If the extended commuity is silently ignored, how would an > operator learn about it? At least a requirement for a log should be added= . > * > Section 5.2; How is the weighted path list computed when the normalized > weight > is in fractions i.e. L(1, 10) =3D 2500 Mbps and thus W(1, 10) =3D 2.5? I = am > guessing you assume it is an integer (same as BW Increment) but it is not > stated. > > ### Section 6 > > * The update procedure listed here "updates" other specifications. I > wonder if > this should be captured in metadata, abstract etc. > > * Section 6.3.1, > * Change L(min) to Lmin t to not be conffused by L(i) > * I am unsure of what you mean by "with PE(1) =3D 10, PE(2) =3D 10, P= E(3) > =3D 20" > which later changes to "with PE(1) =3D 10, PE(2) =3D 10, PE(3) =3D 10= " * > Other > documents do not use the word affinity, it was difficult for me to > verify > the affinity formula and I left that for the WG to verify for > correctness. > * Inconsistency between MUST and MAY - > ```` > Depending on the chosen HRW hash function, the affinity function MUST = be > extended to include bandwidth increment in the computation. > > affinity function specified in [EVPN-PER-MCAST-FLOW-DF] MAY be > extended as follows to incorporate bandwidth increment j: > > affinity or random function specified in [RFC8584] MAY be extended as > follows to incorporate bandwidth increment j: > ```` > > * Section 6.4, asks to add a new bullet (f) in > > https://www.ietf.org/archive/id/draft-ietf-bess-evpn-pref-df-13.html#sect= ion-4.1 > ; Note that there is already a bullet f there! > > ### Section 9 > > * What should be the value-units in this case to be interpreted as relati= ve > weight? > > ### Section 12 > > * Isn't there operation issues with the correct settings of "value-units" > for > Generalized weight? How does an operator learn about the inconsistency? H= ow > does the operator know this feature is working properly? What fields > should one > monitor? Any changes in the YANG model? > > ### Section 13 > > * Even if your claim that there are no new security concerns could be > true, it > needs to be justified and the relevant security of base EVPN needs to be > referenced. You may also highlight some security concerns most relevant t= o > this > extension. > > ### Section 14 > > * Please don't squat on codepoint and allocate them yourself. > * Best to use TBAx > * Or at the very least say that they are suggested values > * In cases where allocations are already made under FCFS, please state th= at > instead of asking IANA to make the allocation again! > > ## Nits: > > * Expand the abbreviation on first use > * CE (also in abstract) > * PE (also in abstract) > * LAG (also in abstract) > * IRB > * BUM > * HRW > * DP > > * s/detailed in RFC 7432/detailed in [RFC7432]/ > * s/all egress PEs, ALL remote traffic/all egress PEs, all remote traffic= / > * There are various instances where you use"proposed", this should be > changed > to "specified" as we are moving towards RFC publication and it is no long= er > just a proposal. * Isnt "per-[ES, EVI] RT-1" enough? Why does it say > "per-ES > RT-1 and per-[ES, EVI] RT-1" in - ```` > In an unlikely scenario where an EVPN > implementation does not support EVPN aliasing procedures, MAC > forwarding path-list at the ingress PE is computed based on per-ES > RT-1 and RT-2 routes received from egress PEs, instead of per-ES RT-1 > and per-[ES, EVI] RT-1 from egress PEs. > ```` > * Section 7 should ideally be a subsection of Section 6 as it is related > to the > DF election > > Thanks! > Dhruv > > --- > > *In case of bad formatting, refer to this message at - > https://notes.ietf.org/draft-ietf-bess-evpn-unequal-lb-18?view* > > > _______________________________________________ > BESS mailing list > BESS@ietf.org > https://www.ietf.org/mailman/listinfo/bess > --000000000000f83c12060a9bb77c Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

Hi Dhruv,

Many thanks fo= r the review. Will update the draft and respond by next week.
Thanks,
Neeraj

On Thu, Nov 16, 2023 at 3:01=E2= =80=AFAM Dhruv Dhody via Datatracker <noreply@ietf.org> wrote:
Reviewer: Dhruv= Dhody
Review result: Has Issues

# RTGDIR review of draft-ietf-bess-evpn-unequal-lb-18

Hello,

I have been selected to do a routing directorate =E2=80=9Cearly=E2=80=9D re= view of this draft.
https://datatracker.ietf.org/doc/dr= aft-ietf-bess-evpn-unequal-lb/

The routing directorate will, on request from the working group chair, perf= orm
an =E2=80=9Cearly=E2=80=9D review of a draft before it is submitted for pub= lication to the
IESG. The early review can be performed at any time during the draft=E2=80= =99s lifetime
as a working group document. The purpose of the early review depends on the=
stage that the document has reached. As this document is
post-working-group-last-call, my focus for the review was to determine whet= her
the document is ready to be published.

For more information about the Routing Directorate, please see
https://wiki.ietf.org/en/group/rtg/RtgDir

Document: draft-ietf-bess-evpn-unequal-lb-18
Reviewer: Dhruv Dhody
Review Date: 16 Nov 2023
Intended Status: Standards Track

## Summary:

The motivation for the draft is clear and described well. However, I have significant concerns about this document. It needs more work before being submitted to the IESG.

## Comments:

### General

* Request the shepherd to make sure that there is a valid justification for= 6
authors. Better yet just make it 5 authors (you have 3 authors from the sam= e
affiliation and one author marked as editor)

* Please follow the RFC style guide. For instance, the Introduction should = be
the first section as per -
https://www.rfc-editor.org/rfc/rfc7322.ht= ml#section-4.8.1. The best would be to
have a new Introduction section that briefly introduces the concept and cha= nge
section 2 to "Motivation" or something like that.

* Use of some words in all capital letters -=C2=A0 OR, ALL, NOT. This shoul= d be
avoided so as not to confuse with RFC2119 keywords which have special meani= ng
when in capital.

* The documents should add references to relevant RFCs when talking about existing EVPN features.
=C2=A0 =C2=A0 * IRB
=C2=A0 =C2=A0 *

### Section 1

* Please update the Requirements Language template to -
````
=C2=A0 =C2=A0The key words "MUST", "MUST NOT", "RE= QUIRED", "SHALL", "SHALL NOT",
=C2=A0 =C2=A0"SHOULD", "SHOULD NOT", "RECOMMENDED&= quot;, "NOT RECOMMENDED", "MAY", and
=C2=A0 =C2=A0"OPTIONAL" in this document are to be interpreted as= described in
=C2=A0 =C2=A0BCP 14 [RFC2119] [RFC8174] when, and only when, they appear in= all
=C2=A0 =C2=A0capitals, as shown here.
````
* Please add references for the terms where possible. Definitions such as "Egress PE" and "Ingress PE" refer to RT-1, RT-2, and R= T-5 especially needs
one. Also, can the local PE and Ingress PE be different?

### Section 4

* Why SHOULD and not MUST in -
````
Implementations SHOULD support the default units of Mbps
````
* IMHO section 4.2 is better suited in the appendix

### Section 5

* Section 5.1; Why SHOULD and not MUST?
* Section 5.1; Consider adding the reasoning behind
````
=C2=A0 =C2=A0EVPN link bandwidth extended community SHOULD NOT be attached = to per-
=C2=A0 =C2=A0EVI RT-1 or to EVPN RT-2.
````
* Section 5.2; If the extended commuity is silently ignored, how would an operator learn about it? At least a requirement for a log should be added. = *
Section 5.2; How is the weighted path list computed when the normalized wei= ght
is in fractions i.e. L(1, 10) =3D 2500 Mbps and thus W(1, 10) =3D 2.5? I am=
guessing you assume it is an integer (same as BW Increment) but it is not stated.

### Section 6

* The update procedure listed here "updates" other specifications= . I wonder if
this should be captured in metadata, abstract etc.

* Section 6.3.1,
=C2=A0 =C2=A0 * Change L(min) to Lmin t to not be conffused by L(i)
=C2=A0 =C2=A0 * I am unsure of what you mean by "with PE(1) =3D 10, PE= (2) =3D 10, PE(3) =3D 20"
=C2=A0 =C2=A0 which later changes to "with PE(1) =3D 10, PE(2) =3D 10,= PE(3) =3D 10" * Other
=C2=A0 =C2=A0 documents do not use the word affinity, it was difficult for = me to verify
=C2=A0 =C2=A0 the affinity formula and I left that for the WG to verify for= correctness.
=C2=A0 =C2=A0 * Inconsistency between MUST and MAY -
````
=C2=A0 =C2=A0Depending on the chosen HRW hash function, the affinity functi= on MUST be
=C2=A0 =C2=A0extended to include bandwidth increment in the computation.
=C2=A0 =C2=A0affinity function specified in [EVPN-PER-MCAST-FLOW-DF] MAY be=
=C2=A0 =C2=A0extended as follows to incorporate bandwidth increment j:

=C2=A0 =C2=A0affinity or random function specified in [RFC8584] MAY be exte= nded as
=C2=A0 =C2=A0follows to incorporate bandwidth increment j:
````

* Section 6.4, asks to add a new bullet (f) in
https://www.ietf.org= /archive/id/draft-ietf-bess-evpn-pref-df-13.html#section-4.1
; Note that there is already a bullet f there!

### Section 9

* What should be the value-units in this case to be interpreted as relative=
weight?

### Section 12

* Isn't there operation issues with the correct settings of "value= -units" for
Generalized weight? How does an operator learn about the inconsistency? How=
does the operator know this feature is working properly? What fields should= one
monitor? Any changes in the YANG model?

### Section 13

* Even if your claim that there are no new security concerns could be true,= it
needs to be justified and the relevant security of base EVPN needs to be referenced. You may also highlight some security concerns most relevant to = this
extension.

### Section 14

* Please don't squat on codepoint and allocate them yourself.
=C2=A0 =C2=A0 * Best to use TBAx
=C2=A0 =C2=A0 * Or at the very least say that they are suggested values
* In cases where allocations are already made under FCFS, please state that=
instead of asking IANA to make the allocation again!

## Nits:

* Expand the abbreviation on first use
=C2=A0 =C2=A0 * CE (also in abstract)
=C2=A0 =C2=A0 * PE (also in abstract)
=C2=A0 =C2=A0 * LAG (also in abstract)
=C2=A0 =C2=A0 * IRB
=C2=A0 =C2=A0 * BUM
=C2=A0 =C2=A0 * HRW
=C2=A0 =C2=A0 * DP

* s/detailed in RFC 7432/detailed in [RFC7432]/
* s/all egress PEs, ALL remote traffic/all egress PEs, all remote traffic/<= br> * There are various instances where you use"proposed", this shoul= d be changed
to "specified" as we are moving towards RFC publication and it is= no longer
just a proposal. * Isnt "per-[ES, EVI] RT-1" enough? Why does it = say "per-ES
RT-1 and per-[ES, EVI] RT-1" in - ````
=C2=A0 =C2=A0In an unlikely scenario where an EVPN
=C2=A0 =C2=A0implementation does not support EVPN aliasing procedures, MAC<= br> =C2=A0 =C2=A0forwarding path-list at the ingress PE is computed based on pe= r-ES
=C2=A0 =C2=A0RT-1 and RT-2 routes received from egress PEs, instead of per-= ES RT-1
=C2=A0 =C2=A0and per-[ES, EVI] RT-1 from egress PEs.
````
* Section 7 should ideally be a subsection of Section 6 as it is related to= the
DF election

Thanks!
Dhruv

---

*In case of bad formatting, refer to this message at -
https://notes.ietf.org/draft-ietf-bes= s-evpn-unequal-lb-18?view*


_______________________________________________
BESS mailing list
BESS@ietf.org
https://www.ietf.org/mailman/listinfo/bess
--000000000000f83c12060a9bb77c--