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 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. > * 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! > > ## 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 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 - ```` > 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 >
_______________________________________________ BESS mailing list BESS@ietf.org https://www.ietf.org/mailman/listinfo/bess
