Re: [bess] Rtgdir early review of draft-ietf-bess-evpn-unequal-lb-18

2023-12-07 Thread Neeraj Malhotra (nmalhotr)

Hi,

Thanks Dhruv for the pointer (did not notice earlier that this was already 
allocated). Fixed in rev21.

Thanks,
Neeraj

From: Dhruv Dhody 
Date: Tuesday, December 5, 2023 at 8:12 PM
To: Neeraj Malhotra (nmalhotr) 
Cc: rtg-...@ietf.org , bess@ietf.org , 
draft-ietf-bess-evpn-unequal-lb@ietf.org 

Subject: Re: Rtgdir early review of draft-ietf-bess-evpn-unequal-lb-18
Hi Neeraj,

On Wed, Dec 6, 2023 at 2:19 AM Neeraj Malhotra (nmalhotr) 
mailto:nmalh...@cisco.com>> wrote:

Hi Dhruv,

rev20 incorporates all of the additional points below.

Regarding,

“* In cases where allocations are already made under FCFS, please state that
instead of asking IANA to make the allocation again!”

I am not aware of any allocations that have already been made. Have updated the 
text in this section (now section 10) to call out all requested allocations as 
“suggested” values.

Please do let me know in case you see anything else missing.


Dhruv: See 
https://www.iana.org/assignments/bgp-extended-communities/bgp-extended-communities.xhtml#evpn
0x10
EVPN Link Bandwidth Extended Community
[draft-ietf-bess-evpn-unequal-lb-15<https://www.iana.org/go/draft-ietf-bess-evpn-unequal-lb-15>]
2022-03-11

Thus, this text needs to change --
A new EVPN Link Bandwidth extended community is defined to signal local ES link 
bandwidth to ingress PEs. This extended community is defined of type 0x06 
(EVPN). IANA is requested to assign a suggested sub-type value of 0x10 for the 
EVPN Link bandwidth extended community, of type 0x06 (EVPN). EVPN Link 
Bandwidth extended community is defined as transitive.¶


It would also be a good idea to clearly identify the registry here "EVPN 
Extended Community Sub-Types".

Thanks!
Dhruv


Thanks,
Neeraj

From: Dhruv Dhody mailto:d...@dhruvdhody.com>>
Date: Monday, December 4, 2023 at 11:36 PM
To: Neeraj Malhotra (nmalhotr) mailto:nmalh...@cisco.com>>
Cc: rtg-...@ietf.org<mailto:rtg-...@ietf.org> 
mailto:rtg-...@ietf.org>>, 
bess@ietf.org<mailto:bess@ietf.org> mailto:bess@ietf.org>>, 
draft-ietf-bess-evpn-unequal-lb@ietf.org<mailto:draft-ietf-bess-evpn-unequal-lb@ietf.org>
 
mailto:draft-ietf-bess-evpn-unequal-lb....@ietf.org>>
Subject: Re: Rtgdir early review of draft-ietf-bess-evpn-unequal-lb-18
Thanks Neeraj! Thanks for taking my comments into consideration!

Looking at -19 some additional points!

- No reference should be added in the abstract
- Note to the IESG in the abstract, can be moved to the shepherd report and 
provided the assigned shepherd agrees with your justification.
- s/advertisong/advertising/
- I am worried about the use of "operators SHOULD" in Section 8 i.e. we are 
using SHOULD for how operators need to behave instead of how the 
implementations ought to handle these operational issues.
- This is missed:
### Section 14

* In cases where allocations are already made under FCFS, please state that
instead of asking IANA to make the allocation again!

Thanks!
Dhruv


On Tue, Dec 5, 2023 at 8:08 AM Neeraj Malhotra (nmalhotr) 
mailto:nmalh...@cisco.com>> wrote:

Hi Dhruv,

Many thanks for a very detailed review and comments. I have just published 
version 19 that significantly revises the document to incorporate all of your 
comments as well as comments from Genart early review. Please see additional 
clarifications inline below. Please do let me know in case you see anything 
else outstanding.

Thanks,
Neeraj



## 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)
[NM]: added justification for 6 authors.

* Please follow the RFC style guide. For instance, the Introduction should 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.
[NM]: done

* Use of some words in all capital letters -  OR, ALL, NOT. This should be
avoided so as not to confuse with RFC2119 keywords which have special meaning
when in capital.
[NM]: done

* The documents should add references to relevant RFCs when talking about
existing EVPN features.
* IRB
*
[NM]: done

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

[NM]: done

* Please add references for the terms where possible. Definitions such

Re: [bess] Rtgdir early review of draft-ietf-bess-evpn-unequal-lb-18

2023-12-05 Thread Dhruv Dhody
Hi Neeraj,

On Wed, Dec 6, 2023 at 2:19 AM Neeraj Malhotra (nmalhotr) <
nmalh...@cisco.com> wrote:

>
>
> Hi Dhruv,
>
>
>
> rev20 incorporates all of the additional points below.
>
>
>
> Regarding,
>
>
>
> “* In cases where allocations are already made under FCFS, please state
> that
> instead of asking IANA to make the allocation again!”
>
>
>
> I am not aware of any allocations that have already been made. Have
> updated the text in this section (now section 10) to call out all requested
> allocations as “suggested” values.
>
>
>
> Please do let me know in case you see anything else missing.
>
>
>

Dhruv: See
https://www.iana.org/assignments/bgp-extended-communities/bgp-extended-communities.xhtml#evpn

0x10 EVPN Link Bandwidth Extended Community [
draft-ietf-bess-evpn-unequal-lb-15
<https://www.iana.org/go/draft-ietf-bess-evpn-unequal-lb-15>] 2022-03-11

Thus, this text needs to change --

A new EVPN Link Bandwidth extended community is defined to signal local ES
> link bandwidth to ingress PEs. This extended community is defined of type
> 0x06 (EVPN). IANA is requested to assign a suggested sub-type value of 0x10
> for the EVPN Link bandwidth extended community, of type 0x06 (EVPN). EVPN
> Link Bandwidth extended community is defined as transitive.¶



It would also be a good idea to clearly identify the registry here "EVPN
Extended Community Sub-Types".

Thanks!
Dhruv



> Thanks,
>
> Neeraj
>
>
>
> *From: *Dhruv Dhody 
> *Date: *Monday, December 4, 2023 at 11:36 PM
> *To: *Neeraj Malhotra (nmalhotr) 
> *Cc: *rtg-...@ietf.org , bess@ietf.org ,
> draft-ietf-bess-evpn-unequal-lb@ietf.org <
> draft-ietf-bess-evpn-unequal-lb@ietf.org>
> *Subject: *Re: Rtgdir early review of draft-ietf-bess-evpn-unequal-lb-18
>
> Thanks Neeraj! Thanks for taking my comments into consideration!
>
> Looking at -19 some additional points!
>
> - No reference should be added in the abstract
> - Note to the IESG in the abstract, can be moved to the shepherd report
> and provided the assigned shepherd agrees with your justification.
> - s/advertisong/advertising/
> - I am worried about the use of "operators SHOULD" in Section 8 i.e. we
> are using SHOULD for how operators need to behave instead of how the
> implementations ought to handle these operational issues.
> - This is missed:
> ### Section 14
>
> * In cases where allocations are already made under FCFS, please state that
> instead of asking IANA to make the allocation again!
>
> Thanks!
> Dhruv
>
>
>
>
>
> On Tue, Dec 5, 2023 at 8:08 AM Neeraj Malhotra (nmalhotr) <
> nmalh...@cisco.com> wrote:
>
>
>
> Hi Dhruv,
>
>
>
> Many thanks for a very detailed review and comments. I have just published
> version 19 that significantly revises the document to incorporate all of
> your comments as well as comments from Genart early review. Please see
> additional clarifications inline below. Please do let me know in case you
> see anything else outstanding.
>
>
>
> Thanks,
>
> Neeraj
>
>
>
>
>
> ## 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)
>
> [NM]: added justification for 6 authors.
>
> * Please follow the RFC style guide. For instance, the Introduction should
> 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.
>
> [NM]: done
>
>
> * Use of some words in all capital letters -  OR, ALL, NOT. This should be
> avoided so as not to confuse with RFC2119 keywords which have special
> meaning
> when in capital.
>
> [NM]: done
>
> * The documents should add references to relevant RFCs when talking about
> existing EVPN features.
> * IRB
> *
>
> [NM]: done
>
>
> ### 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.
> 
>
> [NM]: done
>
>

Re: [bess] Rtgdir early review of draft-ietf-bess-evpn-unequal-lb-18

2023-12-05 Thread Neeraj Malhotra (nmalhotr)

Hi Dhruv,

rev20 incorporates all of the additional points below.

Regarding,

“* In cases where allocations are already made under FCFS, please state that
instead of asking IANA to make the allocation again!”

I am not aware of any allocations that have already been made. Have updated the 
text in this section (now section 10) to call out all requested allocations as 
“suggested” values.

Please do let me know in case you see anything else missing.

Thanks,
Neeraj

From: Dhruv Dhody 
Date: Monday, December 4, 2023 at 11:36 PM
To: Neeraj Malhotra (nmalhotr) 
Cc: rtg-...@ietf.org , bess@ietf.org , 
draft-ietf-bess-evpn-unequal-lb@ietf.org 

Subject: Re: Rtgdir early review of draft-ietf-bess-evpn-unequal-lb-18
Thanks Neeraj! Thanks for taking my comments into consideration!

Looking at -19 some additional points!

- No reference should be added in the abstract
- Note to the IESG in the abstract, can be moved to the shepherd report and 
provided the assigned shepherd agrees with your justification.
- s/advertisong/advertising/
- I am worried about the use of "operators SHOULD" in Section 8 i.e. we are 
using SHOULD for how operators need to behave instead of how the 
implementations ought to handle these operational issues.
- This is missed:
### Section 14

* In cases where allocations are already made under FCFS, please state that
instead of asking IANA to make the allocation again!

Thanks!
Dhruv


On Tue, Dec 5, 2023 at 8:08 AM Neeraj Malhotra (nmalhotr) 
mailto:nmalh...@cisco.com>> wrote:

Hi Dhruv,

Many thanks for a very detailed review and comments. I have just published 
version 19 that significantly revises the document to incorporate all of your 
comments as well as comments from Genart early review. Please see additional 
clarifications inline below. Please do let me know in case you see anything 
else outstanding.

Thanks,
Neeraj



## 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)
[NM]: added justification for 6 authors.

* Please follow the RFC style guide. For instance, the Introduction should 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.
[NM]: done

* Use of some words in all capital letters -  OR, ALL, NOT. This should be
avoided so as not to confuse with RFC2119 keywords which have special meaning
when in capital.
[NM]: done

* The documents should add references to relevant RFCs when talking about
existing EVPN features.
* IRB
*
[NM]: done

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

[NM]: done

* 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 needs
one. Also, can the local PE and Ingress PE be different?
[NM]: done. Made the terminology consistent to use “Ingress/Egress PE” and 
removed “Local/Remote PE” terminology.

### Section 4

* Why SHOULD and not MUST in -

Implementations SHOULD support the default units of Mbps

[NM]: done. Corrected to MUST.

* IMHO section 4.2 is better suited in the appendix
[NM]: done

### Section 5

* Section 5.1; Why SHOULD and not MUST?
[NM]: done. Corrected to 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.

[NM]: done

* 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. *
[NM]: done

Section 5.2; How is the weighted path list computed when the normalized weight
is in fractions i.e. L(1, 10) = 2500 Mbps and thus W(1, 10) = 2.5? I am
guessing you assume it is an integer (same as BW Increment) but it is not
stated.
[NM]: The method in this section is only an example. Weighted pathlist 
computation is a local implementation choice. That said, divide by HCF method 
in this example will always result in integer weights, although the computed 
weight values using this example method may not always to be reasonably 
programmed in HW. I have added another paragraph to explicitly clarify this as 
well as that this is an implementation choice.

### Section 6

* The update 

Re: [bess] Rtgdir early review of draft-ietf-bess-evpn-unequal-lb-18

2023-12-04 Thread Dhruv Dhody
Thanks Neeraj! Thanks for taking my comments into consideration!

Looking at -19 some additional points!

- No reference should be added in the abstract
- Note to the IESG in the abstract, can be moved to the shepherd report and
provided the assigned shepherd agrees with your justification.
- s/advertisong/advertising/
- I am worried about the use of "operators SHOULD" in Section 8 i.e. we are
using SHOULD for how operators need to behave instead of how the
implementations ought to handle these operational issues.
- This is missed:
### Section 14

* In cases where allocations are already made under FCFS, please state that
instead of asking IANA to make the allocation again!

Thanks!
Dhruv


On Tue, Dec 5, 2023 at 8:08 AM Neeraj Malhotra (nmalhotr) <
nmalh...@cisco.com> wrote:

>
>
> Hi Dhruv,
>
>
>
> Many thanks for a very detailed review and comments. I have just published
> version 19 that significantly revises the document to incorporate all of
> your comments as well as comments from Genart early review. Please see
> additional clarifications inline below. Please do let me know in case you
> see anything else outstanding.
>
>
>
> Thanks,
>
> Neeraj
>
>
>
>
>
> ## 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)
>
> [NM]: added justification for 6 authors.
>
> * Please follow the RFC style guide. For instance, the Introduction should
> 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.
>
> [NM]: done
>
>
> * Use of some words in all capital letters -  OR, ALL, NOT. This should be
> avoided so as not to confuse with RFC2119 keywords which have special
> meaning
> when in capital.
>
> [NM]: done
>
> * The documents should add references to relevant RFCs when talking about
> existing EVPN features.
> * IRB
> *
>
> [NM]: done
>
>
> ### 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.
> 
>
> [NM]: done
>
>
> * 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 needs
> one. Also, can the local PE and Ingress PE be different?
>
> [NM]: done. Made the terminology consistent to use “Ingress/Egress PE” and
> removed “Local/Remote PE” terminology.
>
> ### Section 4
>
> * Why SHOULD and not MUST in -
> 
> Implementations SHOULD support the default units of Mbps
> 
>
> [NM]: done. Corrected to MUST.
>
>
> * IMHO section 4.2 is better suited in the appendix
>
> [NM]: done
>
> ### Section 5
>
> * Section 5.1; Why SHOULD and not MUST?
>
> [NM]: done. Corrected to 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.
> 
>
> [NM]: done
>
>
> * 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.
> *
>
> [NM]: done
>
>
> Section 5.2; How is the weighted path list computed when the normalized
> weight
> is in fractions i.e. L(1, 10) = 2500 Mbps and thus W(1, 10) = 2.5? I am
> guessing you assume it is an integer (same as BW Increment) but it is not
> stated.
>
> [NM]: The method in this section is only an example. Weighted pathlist
> computation is a local implementation choice. That said, divide by HCF
> method in this example will always result in integer weights, although the
> computed weight values using this example method may not always to be
> reasonably programmed in HW. I have added another paragraph to explicitly
> clarify this as well as that this is an implementation choice.
>
>
> ### Section 6
>
> * The update procedure listed here "updates" other specifications. I
> wonder if
> this should be captured in metadata, abstract etc.
>
> [NM]: Added additional text to abstract.
>
> * Section 6.3.1,
> * Change L(min) to Lmin t to not be conffused by L(i)
>
> [NM]: done.
>
>
> * I am unsure of what you mean by "with PE(1) = 10, PE(2) = 10, PE(3)
> = 20"
> which later changes to "with PE(1) = 10, PE(2) = 10, PE(3) = 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.
>
> [NM]: fixed.
>
>
> * Inconsistency between MUST and 

Re: [bess] Rtgdir early review of draft-ietf-bess-evpn-unequal-lb-18

2023-12-04 Thread Neeraj Malhotra (nmalhotr)

Hi Dhruv,

Many thanks for a very detailed review and comments. I have just published 
version 19 that significantly revises the document to incorporate all of your 
comments as well as comments from Genart early review. Please see additional 
clarifications inline below. Please do let me know in case you see anything 
else outstanding.

Thanks,
Neeraj



## 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)
[NM]: added justification for 6 authors.

* Please follow the RFC style guide. For instance, the Introduction should 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.
[NM]: done

* Use of some words in all capital letters -  OR, ALL, NOT. This should be
avoided so as not to confuse with RFC2119 keywords which have special meaning
when in capital.
[NM]: done

* The documents should add references to relevant RFCs when talking about
existing EVPN features.
* IRB
*
[NM]: done

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

[NM]: done

* 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 needs
one. Also, can the local PE and Ingress PE be different?
[NM]: done. Made the terminology consistent to use “Ingress/Egress PE” and 
removed “Local/Remote PE” terminology.

### Section 4

* Why SHOULD and not MUST in -

Implementations SHOULD support the default units of Mbps

[NM]: done. Corrected to MUST.

* IMHO section 4.2 is better suited in the appendix
[NM]: done

### Section 5

* Section 5.1; Why SHOULD and not MUST?
[NM]: done. Corrected to 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.

[NM]: done

* 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. *
[NM]: done

Section 5.2; How is the weighted path list computed when the normalized weight
is in fractions i.e. L(1, 10) = 2500 Mbps and thus W(1, 10) = 2.5? I am
guessing you assume it is an integer (same as BW Increment) but it is not
stated.

[NM]: The method in this section is only an example. Weighted pathlist 
computation is a local implementation choice. That said, divide by HCF method 
in this example will always result in integer weights, although the computed 
weight values using this example method may not always to be reasonably 
programmed in HW. I have added another paragraph to explicitly clarify this as 
well as that this is an implementation choice.

### Section 6

* The update procedure listed here "updates" other specifications. I wonder if
this should be captured in metadata, abstract etc.
[NM]: Added additional text to abstract.

* Section 6.3.1,
* Change L(min) to Lmin t to not be conffused by L(i)
[NM]: done.

* I am unsure of what you mean by "with PE(1) = 10, PE(2) = 10, PE(3) = 20"
which later changes to "with PE(1) = 10, PE(2) = 10, PE(3) = 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.
[NM]: fixed.

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

[NM]: fixed.

* 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!
[NM]: This section updates bullet f in [EVPN-PREF-DF]. I have updated the text 
to clearly state that.

### Section 9

* What should be the value-units in this case to be interpreted as relative
weight?
[NM]: 0x01. Added text to state that (this is now section 7.2 in rev19).

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

Re: [bess] Rtgdir early review of draft-ietf-bess-evpn-unequal-lb-18

2023-11-20 Thread Neeraj Malhotra
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 AM Dhruv Dhody via Datatracker <
nore...@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 “early” 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 “early” review of a draft before it is submitted for publication to the
> IESG. The early review can be performed at any time during the draft’s
> 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
> 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 should
> 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 be
> 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 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
> 
>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) = 2500 Mbps and thus W(1, 10) = 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) = 10, PE(2) = 10, PE(3)
> = 20"
> which later changes to "with PE(1) = 10, PE(2) = 10, PE(3) = 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#section-4.1
> ; Note that there is already a bullet f there!
>
> ### Section 9
>
> * What should be the