Re: [bess] Warren Kumari's No Objection on draft-ietf-bess-evpn-etree-13: (with COMMENT)

[Warren Kumari <[e-mail]>]

Awesome, thank you.

W

On Sun, Oct 29, 2017 at 2:42 PM, Ali Sajassi (sajassi)
<[e-mail]> wrote:
>
> Hi Warren,
>
> Please see my replies inline marked with ³Ali>"
>
> On 9/3/17, 3:56 PM, "Warren Kumari" <[e-mail]> wrote:
>
>>Warren Kumari has entered the following ballot position for
>>draft-ietf-bess-evpn-etree-13: No Objection
>>
>>When responding, please keep the subject line intact and reply to all
>>email addresses included in the To and CC lines. (Feel free to cut this
>>introductory paragraph, however.)
>>
>>
>>Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
>>for more information about IESG DISCUSS and COMMENT positions.
>>
>>
>>The document, along with other ballot positions, can be found here:
>>https://datatracker.ietf.org/doc/draft-ietf-bess-evpn-etree/
>>
>>
>>
>>----------------------------------------------------------------------
>>COMMENT:
>>----------------------------------------------------------------------
>>
>>[ I wrote this review back on Aug 8th, but the balloting wasn't open then
>>- I
>>believe that there has been a new revision since, and so some of these may
>>already have been addressed. ]
>>
>>I'm balloting No Objection, but I do have significant reservations;
>>because of the number of nits the document is quite hard to read - a
>>number of
>>these required re-reading the sentence multiple times, guessing at what
>>the
>>sentence is trying to says, etc. Many of these are really obvious (e.g
>>see nit
>>#14 below) and it makes me concerned that the document hasn't been
>>sufficiently
>>reviewed.
>>
>>In addition, the RFC Editor would have caught these, but it doesn't seem
>>reasonable to rely on them to make documents readable, nor to waste
>>multiple
>>reviewers time addressing what could easily have been caught with a
>>simple read
>>though.
>>
>>Questions / Notes:
>>1: The document has 6 authors (one listed as an author) and a contributors
>>section -- is there a substantive difference between those above the fold
>>and
>>those below it?
>
> Ali> Merged ³Contributors² section into the ³Acknowledgements² section.
>>
>>2: This document uses a number of acronyms without expanding them on
>>first use.
>
> Ali> All acronyms should have been explained (or expanded) before their
> use. But, if there are any, please let me know.
>>
>>3: Section 8.1 Considerations for PMSI Tunnel Types
>>"The status of 0x7F may only be
>>   changed through Standards Action [RFC5226]." - what makes 0x7F
>>special? What
>>   is it reserved for?
>
> Ali> I updated that section in rev14.
>>
>>4: The shepherd writeup says:
>>Q: (16) Will publication of this document change the status of any
>>existing RFCs?
>>A: No change of the status of existing RFCs.
>>
>>I believe that this is incorrect, especially when compared with the Q/A
>>#17.
>
> Ali> This draft described enhancements/extension to procedures in RFC7432
> and RFC7623 when E-TREE service is needed.
>>
>>5: The IANA considerations section seems to be incorrect / transition
>>isn't
>>really described. The current registry says: 0xFB-0xFE Reserved for
>>Experimental Use
>>
>>This document changes that to be:
>>0x7B-0x7E      Reserved for Experimental Use
>
> Ali> The wording in the IANA section wasn¹t quite accurate. The draft
> doesn¹t change the already-defined values for experimental use but rather
> it expands it. I have updated the text to reflect that.
>>
>>While it "only" a change to an experimental range, by their very nature
>>you
>>don't know if anyone is using the current range. I think that the document
>>needs to be much more clear about the fact that it is changing this, and
>>also
>>what the implications are for people who are already using e.g: 0xFB -
>>from
>>what I can see, it could break existing deployments.
>
> Ali> The existing range for Experimental (0xFB-0xFE) doesn¹t change but
> rather it got expanded. It should be now clear in rev14.
>>
>>Nits:
>>
>>1: Section 2.1 Scenario 1: Leaf OR Root site(s) per PE
>>O: In such scenario, using tailored BGP Route Target (RT) import/export
>>   policies among the PEs belonging to the same EVI, can be used to
>>   restrict the communications among Leaf PEs.
>>P: In such scenario, tailored BGP Route Target (RT) import/export
>>   policies among the PEs belonging to the same EVI can be used to
>>   restrict the communications among Leaf PEs.
>>C: Redundant 'using' when coupled with 'can be used'
>
> Ali> Rev13 already has updated text based on this comment.
>>
>>2: Section  2.2 Scenario 2: Leaf OR Root site(s) per AC
>>O: Approach (A) would require the same data plane enhancements as
>>   approach (B) if MAC-VRF and bridge tables used per VLAN, are to
>>   remain consistent with [RFC7432] (section 6).
>>C: This doesn't really parse -- I cannot tell if it is just an extraneous
>>comma
>>(after VLAN) or if it is that subject for "used per VLAN" is unclear.
>
> Ali> Done.
>>
>>3:
>>O: Given that both approaches (A) and (B) would require exact same data-
>>   plane enhancements,
>>P: Given that both approaches (A) and (B) would require the same data-
>>   plane enhancements,
>>C: grammar
>
> Ali> Done.
>>
>>4:
>>O: It should be noted that if one wants to use RT constrain
>>   in order to avoid MAC advertisements
>>P: It should be noted that if one wants to use RT constraints
>>   in order to avoid MAC advertisements
>>C: Assuming "constraints"; if not, needs more rewording.
>
> Ali> Done.
>>
>>5:
>>O: For this scenario, if for a given EVI, significant number of PEs have
>>   both Leaf and Root sites attached, even though they may start as
>>   Root-only or Leaf-only PEs, then a single RT per EVI should be used.
>>P: If, for a given EVI, a significant number of PEs have both Leaf and
>>Root
>>sites attached (even though they may start as Root-only or Leaf-only
>>PEs), then
>>a single RT per EVI should be used. C: Probably cleaner would just be to
>>drop
>>the "For this scenario"; I don't think that the reader would take this a
>>generalization, but your views may differ.
>
> Ali> Done
>>
>>6: 2.3 Scenario 3: Leaf OR Root site(s) per MAC
>>I think that this may be better titled as "2.3 Scenario 3: Leaf OR Root
>>site(s)
>>per MAC address" -- without the 'address' it wasn't clear for the first
>>bit if
>>you actually intended MAC or if this was a typo for AC. Purely a nit.
>
> Ali> Rev13 already has the updated text
>>
>>7:
>>O: This scenario is not covered in both [RFC7387] and [MEF6.1];
>>P: This scenario is not covered in either [RFC7387] or [MEF6.1];
>>C: At least I'm assuming you meant either!
>
> Ali> Rev13 already has the updated text
>
>>
>>8: Section 3.1 Known Unicast Traffic
>>O: Since in EVPN, MAC learning is performed in control plane via
>>P: Since in EVPN, MAC learning is performed in the control plane via
>>C: Or perhaps "by the control plane"
>
> Ali> Done.
>>
>>9:
>>O:  thus providing very efficient filtering and avoiding sending known
>>unicast
>>   traffic over MPLS/IP core to be filtered
>>P:  thus providing very efficient filtering and avoiding sending known
>>unicast
>>   traffic over the MPLS/IP core to be filtered
>
> Ali> Done.
>>
>>10:
>>O: This new Extended Community MUST be advertised with MAC/IP
>>   Advertisement route.
>>P: This new Extended Community MUST be advertised with MAC/IP
>>   Advertisement routes.
>>C: s/route/routes/ (or similar)
>
> Ali> Done.
>>
>>11: Section 3.2 BUM Traffic
>>O: This specification does not provide support for filtering BUM
>>   (Broadcast, Unknown, and Multicast) traffic on the ingress PE because
>>   it is not possible to perform filtering of BUM traffic on the ingress
>>   PE, as is the case with known unicast described above, due to the
>>   multi-destination nature of BUM traffic.
>>P: This specification does not provide support for filtering BUM
>>(Broadcast,
>>Unknown, and Multicast) traffic on the ingress PE; due to the
>>multi-destination
>>nature of BUM traffic, is is not possible to perform filtering of the
>>same on
>>the ingress PE. C: Parenthesis make it a bit easier to read, but the whole
>>sentence should be rewritten; "This specification doesn't do X because it
>>is
>>not possible to do X due to Y" sounds odd, even more so being a run on
>>sentence.
>
> Ali> Done.
>>
>>12:
>>O: Other mechanisms for identifying root or leaf (e.g., on a per MAC
>>address
>>basis) is beyond the scope of this document.
>
>>P: Other mechanisms for identifying root or leaf (e.g., on a per MAC
>>address
>   basis) are beyond the scope of this document. C: Plural alignment.
>
> Ali > It has already been changed to "Other mechanisms for identifying
> Root or Leaf sites such as the use of source MAC address of the receiving
> frame are optional."
>>
>>13: Section 3.2.1 BUM traffic originated from a single-homed site on a
>>leaf AC
>>O: along with an Ethernet A-D per ES route
>>C: A-D is used without expansion. I'm assuming Auto-Discovery, but this
>>is not
>>helpful to readers.
>
> Ali> Done.
>>
>>14: 3.2.3 BUM traffic originated from a multi-homed site on a leaf AC
>>O: In such scenarios,  If a multicast
>>P: In such scenarios, if a multicast
>>C: Things like this (there are multiple) make the reader wonder how well
>>this
>>was reviewed. This has been in the document since -03 (Oct 2015), so it
>>isn't
>>simply a "rush to squeeze it in" event.
>
> Ali> Done.
>>
>>15: Section 3.3.1 E-Tree with MAC Learning
>>O: For the scenario described in section 2.1 (or possibly section 2.2),
>>these
>>routes are imported ... P: For the scenario described in section 2.1 (and
>>possibly the scenario in section 2.2), these routes are imported ... C:
>>The
>>original sounds a lot like you are not clear which section you are
>>referring to.
>
> Ali> Cannot find it. Must have been rectified.
>>
>>16:
>>O: To support multicast/broadcast from Leaf to Root sites, ingress
>>   replication should be sufficient for most scenarios where there are
>>   only a few Roots (typically two). Therefore, in a typical scenario, a
>>   root PE needs to support both ...
>>C: Throughout the document you are using a mix of 'Root' vs 'root',
>>'Leaf' vs
>>'leaf' -- there doesn't seem to be much consistency.
>
> Ali > Done. ³Leaf² and ³Root² are now used consistently.
>>
>>17: Section 4 Operation for PBB-EVPN
>>O: In PBB-EVPN, the PE advertises a Root/Leaf indication along with each
>>   B-MAC Advertisement route, to indicate whether the associated B-MAC
>>   address corresponds to a Root or a Leaf site.
>>C: Please fix the commas - I think you just need to delete the second (or
>>perhaps put the "along with each" bit in parens) - the current text is
>>confusing.
>
> Ali> Done.
>>
>>18:
>>O: In such multi-homing scenarios where an Ethernet Segment has
>>   both Root and Leaf ACs, ...
>>P: In multi-homing scenarios where an Ethernet Segment has
>>   both Root and Leaf ACs, ...
>
> Ali> Done.
>>
>>19:
>>O: it is assumed that While different ACs (VLANs) on the same ES
>>C: See #14.
>
> Ali> Done.
>>
>>20: Section 4.1 Known Unicast Traffic
>>O: On the ingress PE, the C-MAC destination address lookup yields, ...
>>C: C-MAC is used without expansion - please insert reference (probably to
>>RFC
>>7623)
>
> Ali> Done.
>>
>>21: Section 4.3 E-Tree without MAC Learning
>>O: In scenarios where the traffic of interest is only Multicast and/or
>>   broadcast, the ...
>>P: In scenarios where the traffic of interest is only multicast and/or
>>   broadcast, the ...
>>C: Please choose a single capitalization for Broadcast and Multicast and
>>stick
>>to it -- there are around 5 Multicast and 6 multicast.
>
> Ali> Done. The other instances of ³Multicast² is for tile/name - e.g.,
> Inclusive Multicast route
>>
>>22: Section 5.2 PMSI Tunnel Attribute
>>O: This document uses all other remaining fields per
>>   existing definition.
>>   P: This document does not redefine any other terms.
>>C: This still ready poorly -- I think according to existing definitions
>>(also,
>>plural matching).
>
> Ali> Changed it to: "All other fields remain as defined in [RFC6514]."
>>
>>23:
>>O: When receiver ingress-replication label is needed, the high-order bit
>>   of the tunnel type field (Composite Tunnel bit) is
>>C: I think "When a receiver ingress-replication label is needed" or "When
>>receiver ingress-replication labels are needed" - whatever the case, I
>>think
>>you are missing a word or using the wrong one.
>
> Ali> Changed it to ³When receiver ingress-replication labels are needed"
>
>>
>>24:
>>O: When this Composite Tunnel bit is set, the "tunnel identifier" field
>>   would begin with a three-octet label,
>>P: When this Composite Tunnel bit is set, the "tunnel identifier" field
>>   begins with a three-octet label,
>
> Ali> It is already done for Rev13.
>>
>>25:
>>O: PEs that don¹t understand the
>>   new meaning of the high-order bit would treat the tunnel type
>>C: As before, delete "would"
>
> Ali> Done.
>>
>>26: Section 7  Security Considerations
>>O: Furthermore, this document provides additional
>>   security check by allowing sites
>>P: Furthermore, this document provides an additional
>>   security check by allowing sites
>>C: Or "checks"...
>>
> Ali> Done.
>>
>



-- 
I don't think the execution is relevant when it was obviously a bad
idea in the first place.
This is like putting rabid weasels in your pants, and later expressing
regret at having chosen those particular rabid weasels and that pair
of pants.
   ---maf