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

["Neeraj Malhotra (nmalhotr)" <[e-mail]>]

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 feature is working properly? What fields should one
monitor? Any changes in the YANG model?
[NM]: added.

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

[NM]: added.

### 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 that
instead of asking IANA to make the allocation again!

[NM]: fixed.

## Nits:

* Expand the abbreviation on first use
    * CE (also in abstract)
    * PE (also in abstract)
    * LAG (also in abstract)
    * IRB
    * BUM
    * HRW
    * DP
[NM]: done.

* 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 longer
just a proposal.
[NM]: done.
* 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.
````
[NM]: Both per-[ES] RT-1 and per-[ES, EVI] RT-1 routes are required for aliasing procedure specified in RFC 7432.

* Section 7 should ideally be a subsection of Section 6 as it is related to the
DF election

[NM]: done.