Last Call Review of draft-ietf-bess-mvpn-evpn-aggregation-label-08
review-ietf-bess-mvpn-evpn-aggregation-label-08-rtgdir-lc-przygienda-2022-11-01-00

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.

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

3. "but each PE would
   know not to allocate labels from that block for other purposes" is difficult
   to read. Rewrite from negative.

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"

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 ()

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" ?) ?

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" ?

8. "if it is difficult". If it is _too_ difficult?
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

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 ;-)

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?

12.
add to glossary section "PE Distinguisher Labels" with RFC reference
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

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"?

15.
"
[note -
   future revision may extend the applicability of this document to
   Ingress Replication as well]
"

to be expanded or removed

16.
" when a segmentation point re-advertise a PMSI "
re-advertise_s_

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.

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.