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

[Dhruv Dhody <[e-mail]>]

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) <
[e-mail]> 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 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.
>
>
>