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

["Ali Sajassi (sajassi)" <[e-mail]>]

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