Hi Warren, Please see my replies inline marked with ³Ali>"
On 9/3/17, 3:56 PM, "Warren Kumari" <[email protected]> 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. > _______________________________________________ BESS mailing list [email protected] https://www.ietf.org/mailman/listinfo/bess
