Hi Tony, Thanks for your thorough review and comments. I have posted the -09 version that I believe/hope have address your comments/concerns. https://datatracker.ietf.org/doc/html/draft-ietf-bess-mvpn-evpn-aggregation-label-09 Please see zzh> below for some details. Juniper Business Use Only -----Original Message----- From: Tony Przygienda via Datatracker <noreply@xxxxxxxx> Sent: Tuesday, November 1, 2022 4:02 PM To: rtg-dir@xxxxxxxx Cc: bess@xxxxxxxx; draft-ietf-bess-mvpn-evpn-aggregation-label.all@xxxxxxxx; last-call@xxxxxxxx Subject: Rtgdir last call review of draft-ietf-bess-mvpn-evpn-aggregation-label-08 [External Email. Be cautious of content] Reviewer: Tony Przygienda Review result: Has Issues As first, technically sound except point 18. Rest of the commentes provided are all for easier readability/clarity for a reader that may not be super instrinsically familiar with the world of overlay multicast tunnels underlying VPN technologies first. I am quite versant in it but even then, the complexity of the field made me stumble on some assertions given without explanation. Then, good amount of omitted words etc necessary to read as coherent English sentences. Ultimately, some of the transitions in terms of causality chains do not connect and can leave the reader stranded which I'm pointing out in specific comments. Numbered: 1. document only distincts between p2mp and mp2mp rather than using the PMSI terminology with inclusisve/selective [RFC6513]. the S/I is not relevant but it would help readability if that would be explained. Especially since the S/I starts to be introduced in 2.2.2.1 suddenly. Zzh> The difference between P2MP and MP2MP is actually not important and not the focus of the document. The document starts with P2MP because it is more common; it does talk about MP2MP's applicability with some specifics afterwards. Zzh> Use of per-VPN/BD/ES DCB labels does not work for tunnel segmentation, and entire section 2.2.2 is about how we mitigate that. Section 2.2.2.1 explains the need for per-PMSI labels using selective tunnels example, and section 2.2.2.2 extends it to inclusive tunnels. Zzh> For an average reader, selective/inclusive tunnels are easier to understand than S/I-PMSI terms so the sections are based on "tunnels" instead of PMSIs, but now I have added additional glossaries and text to tie the two together. 2. Terminology -- provide references for BD/BUM/PMSI/IMET/PTA in terms of RFCs defining them properly. Currently the section is just acronym expansion really. -- provide definition of "aggregate tunnel" in the terminology section rather than later in the document for consistency -- add definitons for "context space", "upstream assigned label" and "ESI Label" here as well rather than later in the document. This may lead to more conscise and readable introduction section -- add definition for DCB -- add def for SRGB with according SRGB --- I suggest to add upstream assigned (and downstream) labels to definitions as well since it's so central to the document. Acronym expansion + RFC ref is fine AFAIS but at least the reader can peddle back and know in one shot where all the stuff comes from or read things upfront to have an inkling. The drip-drip technique uesed in the document is ok'ish since it makes an illusion of gradual introduction into the solution space on first read but makes it hard to find things later, have in one easy shot a quick recall "what was that all about". IME good glossary goes a very long way when attempting a 2nd read or trying to do a fast page-in later. -- given 2.2.2.1 I suggest to add PMSI + S/I + PTA defintions + IMET + RBR. And maybe minimum definition (or where to find the terminology precisely) for the whole (C-*,C-*) machinery involved in context lookups you pull out in 2.2.2.3 Zzh> I added more terms. Zzh> I assume most people now just read the electronic copy instead of paper copy, so the drip-drip technique should not only work well for gradual introduction but also not make it hard to find things later? 😊 3. "but each PE would know not to allocate labels from that block for other purposes" is difficult to read. Rewrite from negative. Zzh> I removed it entirely. Sometimes less is more 😊 4. "1000 is for VPN 0, 1001 for VPN 1 ...". Write it clearer, maybe "label 1000 maps to VPN 0, 1001 to VPN1 and so forth" Zzh> Fixed as suggested. 5. " network. (Though if tunnel segmentation [RFC6514] is used, each segmentation region could have its own DCB. This will be explained in more detail later.)". This separate sentence in () is funny, make is a composite sentence or part of previous sentence in () zzh> I removed it. It's more accurate w/o it. 6. " However, that is not necessarily as the labels used by PEs for the purposes defined in this document will only rise to the top of the label stack when traffic arrives the PEs. " does not parse as English. this is not necessarily _true_ ? arrives _from_ the PEs (which it always does or you want to emphasize that traffic can "arrive from P" ?) ? zzh> I hope it becomes clear if I replace "necessarily" with "necessary" - it is a typo 😊 7. " That way they do not have to aside the same DCB used for the purposes in this document. " Again, not English. "they do not have to _set_ aside" ? Zzh> Right. Fixed. 8. "if it is difficult". If it is _too_ difficult? Zzh> Fixed. 9. Here you will loose many readers "We also augment the signaling so that it is possible to indicate that a particular label is from an identified context label space that is different than the ingress PE's own context label space. " If you need to differentiate between a "ingress PE context label space" and another type of context label space you need to introduce this terminology, maybe "GCLS" for global context label space and "PCLS" or some such thing. Otherwise the reading of the "context label space" depends too much on the context (pun intented ;-) And then segmentation points seem to have yet another "context label space" which may need zzh> I changed all "context label space" to "context-specific label space" to be exactly matching RFC5331 and more importantly changed relevant text to make it clear. Please see the attached diff file. 10. " Notice that, the VPN/BD/ES-identifying labels from the DCB or from those few context label spaces are very similar to VNIs in VXLAN. Allocating a label from the DCB or from those a few context label spaces and communicating them to all PEs should not be different from allocating VNIs, and should be feasible in today's networks since controllers are used more and more widely. " I would loose the whole thing. It's an analogy across yet another technology and imperfect one on top of that given we have SRC/DST in VXLAN on top and "bias" and all kind of ugly hacks (eeeh, enforced features ;-) Zzh> Well my impression is that EVPN-VXLAN is used more widely than EVPN-MPLS 😊 11. " MP2MP tunnels present the same problem that can be solved the same way." What same problem? what same way? Does that relate to vxlan analogy or the same problem as P2MP? So why a section start here? Zzh> Same problem as in "2.1. Problem Description" and same solution as in "2.2. Proposed Solution". A new section is used because of the following additional requirement for MP2MP: It is REQUIRED by this document that the PE Distinguisher labels allocated by a particular node come from the same label space that the node uses to allocate its VPN-identifying labels. Zzh> I clarified in the text. 12. add to glossary section "PE Distinguisher Labels" with RFC reference Zzh> It's a term that is difficult to define concisely in the glossary section. I would still prefer to reference it the first time it is used. 13. " though it does not address the original scaling issue, because there would be one million labels allocated from those a few context label spaces in the original example " - from those a few context label spaces - does not seem to parse Zzh> The previous paragraph says: In this case, it is not practical to have a central authority assign domain-wide unique labels to individual S-PMSI routes. To address this problem, all PEs can be assigned disjoint label blocks in those few context label spaces, and each will allocate labels for segmented S-PMSI independently from its assigned label block that is different from any other PE's. For example, PE1 allocates from label block [101~200], PE2 allocates from label block [201~300], and so on. Zzh> Basically, instead of each ingress PE upstream-assigns from its own label space, we'll just use one or a few context label spaces and divide that into different label blocks, each block assigned to one PE. Your quoted text "a few context label spaces" refer to those label spaces. The reason to use those a few context label spaces instead of per-PE label spaces is to make it easier for the egress PEs - instead of managing many context label spaces, only a few needs to be managed. 14. " to allocate its own label from the label block allocated to itself " drop the -own- and define what is -label block allocated to itself-. Is that the "segmentation point context label space"? zzh> I removed the "own". The "label block" is the one described in previous section as the following: In this case, it is not practical to have a central authority assign domain-wide unique labels to individual S-PMSI routes. To address this problem, all PEs can be assigned disjoint label blocks in those few context label spaces, and each will allocate labels for segmented S-PMSI independently from its assigned label block that is different from any other PE's. For example, PE1 allocates from label block [101~200], PE2 allocates from label block [201~300], and so on. Zzh> I added a reference to it. 15. " [note - future revision may extend the applicability of this document to Ingress Replication as well] " to be expanded or removed zzh> Removed 😊 16. " when a segmentation point re-advertise a PMSI " re-advertise_s_ zzh> Fixed. 17. " Note that, when a segmentation point re-advertise a PMSI route to the next segment, it does not need to re-advertise a new label unless the upstream or downstream segment uses Ingress Replication. " I couldn't imagine why. Can you provide a one liner explanation. Zzh> I can't provide a *one liner* explanation 😊 Here is a longer explanation: Zzh> The label is used to demultiplex traffic belonging to different PMSIs but carried on the same intra-region tunnel. Because it is allocated from a context label space known to every PE and segmentation points, and the context label space is identified by a DCB label, all segmentation points can demultiplex based on the same label so there is no need to re-advertise a new label. Zzh> For comparison, when ingress replication is used, a PE or a segmentation point advertise a local downstream-assigned label towards the upstream via Leaf A-D routes, and the PMSI route does not carry a demultiplexing label. 18. I am missing capability indication in BGP somehow to make a PE indicate it even understands this new signalling type. What happens in mixed scenarios etc. Zzh> Mixed scenario is not supported. I added the following at the beginning of "procedures" section: When these procedures are used, all PE routers and segmentation points MUST support the procedures. It is outside the scope of this document how that is ensured. Zzh> Thanks! Zzh> Jeffrey -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
