Re: [Bier] Rtgdir last call review of draft-ietf-bier-te-arch-10

[Toerless Eckert <[e-mail]>]

Thanks, Yingzhen

Detailed replies for your comments below inline.
It should resolve all your concerns.
These have been integrated into draft rev -12
which the authors feel is ready for RFC editor.

Full diff from -11 to -12:

http://tools.ietf.org//rfcdiff?url1=https://www.ietf.org/archive/id/draft-ietf-bier-te-arch-11.txt&url2=https://www.ietf.org/archive/id/draft-ietf-bier-te-arch-12.txt

Diff with just the deltas for comments by Robert Sparks, Yingzhen Qu and Martin Duke.

http://tools.ietf.org//rfcdiff?url1=https://raw.githubusercontent.com/toerless/bier-te-arch/32d75563d07b8f7559bc2a88e80f113e228ad21a/draft-ietf-bier-te-arch.txt&url2=https://raw.githubusercontent.com/toerless/bier-te-arch/fbb26ac4d2cc5dd841e36a00801f58ad5c7eae11/draft-ietf-bier-te-arch.txt

Thanks again for your review.

Toerless


On Fri, Aug 20, 2021 at 10:29:35PM -0700, Yingzhen Qu via Datatracker wrote:
> Reviewer: Yingzhen Qu
> Review result: Has Nits
> 
> I have been selected as the Routing Directorate reviewer for this draft. The
> Routing Directorate seeks to review all routing or routing-related drafts as
> they pass through IETF last call and IESG review, and sometimes on special
> request. The purpose of the review is to provide assistance to the Routing ADs.
> For more information about the Routing Directorate, please see
> http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir
> <http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir>
> 
> Although these comments are primarily for the use of the Routing ADs, it would
> be helpful if you could consider them along with any other IETF Last Call
> comments that you receive, and strive to resolve them through discussion or by
> updating the draft.
> 
> Document: draft-ietf-bier-te-arch-10
> Reviewer: Yingzhen Qu
> Review Date: Aug 20th, 2021
> Intended Status: Standards Track
> 
> Summary:
> 
> This document has some issues/nits that should be at least considered prior to
> publication.
> 
> Comments:
> 
> Typically when BIER-TE controller calculates BitStrings, the result “overlay”
> topology has to be trees, no circles. Then ring topology becomes a special
> case. This should be explained/stressed.

Changed the first sentences of abstract and intro to:

<t>BIER-TE introduces a new semantic for "bit positions" (BP). They indicate adjacencies
of the network topology, as opposed to (non-TE) BIER in which BPs indicate
 "Bit-Forwarding Egress Routers" (BFER).  A BIER-TE packets BitString therefore indicates the
edges of the (loop-free) tree that the packet is forwarded across by BIER-TE.
> 
> Comments inline:
> 
> [Line numbers from idnits]
> 
> >From IDNITS:
>   == The document seems to lack the recommended RFC 2119 boilerplate, even if
>      it appears to use RFC 2119 keywords -- however, there's a paragraph with
>      a matching beginning. Boilerplate error?
> 
>      RFC 8174, paragraph 11:
>         The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
>         "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY",
>         and "OPTIONAL" in this document are to be interpreted as
>         described in BCP 14 [RFC2119] [RFC8174] when, and only when, they
>         appear in all capitals, as shown here.
> 
>      ... text found in draft:
>         The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
>         "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY",
>         and "OPTIONAL" in this document are to be interpreted as
>         described in BCP 14 [RFC2119], [RFC8174] when, and only when,
> .....................................^
>         they appear in all capitals, as shown here.
> 
> The extra “,” should be removed.

Fixed.

> 142        BIER-TE introduces a new semantic for bit positions (BP) that
> 143        indicate adjacencies, as opposed to BIER in which BPs indicate Bit-
> 144        Forwarding Egress Routers (BFER).  With BIER-TE, the BIFT of each BFR
> 145        is only populated with BP that are adjacent to the BFR in the BIER-TE
> 146        Topology.  Other BPs are empty in the BIFT.  The BFR replicate and
> 147        forwards BIER packets to adjacent BPs that are set in the packet.
> 148        BPs are normally also cleared upon forwarding to avoid duplicates and
> 149        loops.  This is detailed further below.
> 
> [nits]: BIFT, BFR should be expanded the first time in this document.

Fixed through other reviewers feedback already.

> [nits]: detailed further below, may reference a section number.

Rather deleted the sentence. Kinda obvious after the first paragraph of intro.

> 359        BIER-TE is designed so that is forwarding plane is a simple extension
> 
> [nits]: s/so that is/so that its

Fixed through other reviewers already.

> 376            3.  The supportable encapsulations, [RFC8296] or other (future)
> 377                encapsulations.
> 
> [minor]: I don’t think you can say it works with “future” encapsulations.

Changed to:

<t>The supportable encapsulations including <xref target="RFC8296"/>.</t>

> 388            1.  In BIER, bits in the BitString of a BIER packet header
> 389                indicate a BFER and bits in the BIFT indicate the BIER
> 390                control plane calculated next-hop toward that BFER.  In BIER-
> 391                TE, bits in the BitString of a BIER packet header indicate an
> 392                adjacency in the BIER-TE topology, and only the BFRs that are
> 393                upstream of this adjacency have this bit populated with the
> 394                adjacency in their BIFT.
> 
> [nits]: The English language in this paragraph needs to be improved, especially
> whether it’s singular or plural. For example, bits in the BitString of a BIER
> packet header indicate BFERs.

Already changed for prior reviewer, but changed second part of paragraph
now to singular example:

     <t>In BIER, bits in the BitString of a BIER packet header indicate a BFER
        and bits in the BIFT indicate the BIER control plane calculated next-hop
        toward that BFER. In BIER-TE, a bit in the BitString of a BIER packet
        header indicates an adjacency in the BIER-TE topology, and only the
        BFR that is the upstream of that adjacency has its BP populated with
        the adjacency in its BIFT.</t>

    <t>In BIER, the implied reference option for the core part of the BIER layer
       control plane are the BIER extensions for distributed routing protocols.
       This includes ISIS/OSPF extensions for BIER, <xref target="RFC8401"/>
       and <xref target="RFC8444"/>.</t>

> 
> 430            4.  BIER-TE forwarding does not use the BFR-id field of the BIER
> 431                packet header.
> 
> [minor]: what’s BFR-id field? Do you mean BFIR-id?

Yes, fixed. also fixed BFIR-id in one or two other places.

> 443            2.  BIER-TE deployments will have to assign BFR-ids to BFR and
> 444                insert them into the BFR-id field of BIER packet headers as
> 
> [minor]: the BFR-id field?

Actually was changed for another reviewer to "to BFRs and..."

> 512        1.  During initial provisioning of the network and/or during
> 513            modifications of its topology and/or services: protocols and/or
> 514            procedures to establish BIER-TE BIFTs:
> 
> [nits]: two “:” in a row.

replaced first ":" with ", the".

> 523                BIER-TE headers on BFIR.  Alternatively, bfir-id in BIER
> 
> [nits]: s/bfir-id/BFIR-id as in RFC8296.

Fixed to BFR-id (see above. (see above ;-)

> 527            4.  Install/update the BIFTs into the BFRs and optionally BFR-id
> 528                into BFIR.
> 
> [minor]: “optionally BFR-id into BFIR” duplicates with point 3 above.

True, but point 3 is address allocation with motivation why. Point 4 is then
that address installation. The duplication makes the step better independent from
each other. I think..

> 530        2.  During operations of the network: Protocols and/or procedures to
> 531            support creation/change/removal of overlay flows on BFIR:
> 
> [nits]: two “:” in a row.

replaced first ":" with ",".

> 553           BFR in step 1, such YANG/Netconf/RestConf.
> 
> [nits]: s/ Netconf/Restconf /NETCONF/RESTCONF, also you may consider adding
> informative references YANG (RFC7950), NETCONF [RFC6241] and RESTCONF [RFC8040]

Fixed.

> 578        extending a Link-State-Protocol (LSP) based IGP into the BIER-TE
> 
> [minor]: I’d suggest remove “LSP” abbreviation here. LSP means Link-State PDU
> in IS-IS.

Fixed.

> 600        models such as Netconf/RestConf/Yang/PCEP.  Vendor-specific CLI on
> 
> [nits]: s /Netconf/RestConf/Yang/ NETCONF/RESTCONF/YANG. They are used in
> multiple times in the draft, please fix all of them.

Yepp.

> 1016            AdjacentBits = Packet->BitString &= ~AdjacentBits[SI];
> 1017            Packet->BitString &= AdjacentBits[SI];
> 
> [major]: I’m confused about these two lines of code. Can you please explain what
> it is trying to achieve? Packet->BitString got masked twice?

Fixed already though Bens review. The "~" was on the wrong line.
-12 also has more explanatory text and comments.

> 1047    4.5.  Basic BIER-TE Forwarding Example
> 
> 1049       [RFC Editor: remove this section.]
> 
> [minor]: an example that matches the pseudocode would be helpful. Whether this
> example needs to be removed is up to the author.

Hmm.. Like exercising every instance of the forwarding code once...
Yeah, that would have been a good idea earlier, but a bit too late
now given how all reviews are done, and so i wouldn't have help getting
bugs in it fixed. Alvaro didn't mind keeping this existing example, but no
more chime ins, so i think i will keep it marked as to be removed,
because except for a bit more complexity, it doesn't offer anything
fundamental new over the first example in the doc.
> 
> 1127       is in the BitString and this is an adjacency towards BFR3.  BFIR2
> 1128       therefore clears p2 in the BitString and sends a copy towards BFR2.
> 
> [major]: line 1128 should be “a copy towards BFR3” in case the example is kept.

Yepp, already fixed from other review.

> 1142       Further processing of the packet in BFR4, BFR5 and BFER2 accordingly.
> 
> [major]: BFR4 sees BitStrings of p5, p10, p11 and p12, should it send the packet
> To BFER2 directly because of p10? I’m not sure whether p10 is adjacent to BFR4
> in the figure 7.

No, they are not. Two rows of space in the ASCII art. If they where, then
the BP allocations would be one of the more difficult LAN cases.

> 1180       topologies with fewer bit positions (4.1, 4.3, 4.4, 4.5, 4.6, 4.7,
> 1181       4.8).
> 
> [major]: the section numbers in “()” are not correct.

List already removed by suggestion of other reviewer.

> 1209       A leaf BFERs is one where incoming BIER-TE packets never need to be
> 
> [nits]: s/BFERs/BFER

Fixed.
> 
> 1241                BFR1
> 1242                 |p1
> 1243          LAN1-+-+---+-----+
> 1244              p3|  p4|   p2|
> 1245              BFR3 BFR4  BFR7
> 
> 1247                              Figure 11: LAN Example
> 
> [nits]: Please center this figure

Hah. indented picture randomnly. Not sure if there is an actual XML center function.

> 1266       bit position on the hub's BIFT is set up with a list of
> 1267       forward_connected() adjacencies, one for each Spoke.
> 
> [comments]: my understanding is that this optimization only works when the hub
> needs to forward a received packet to all spokes except the spoke sending the
> packet. Correct?

Kinda.
You could not do this BP optimization if you want to directly send packets
from one spoke via the hub to all other spokes AND you can not deal with
the sending spoke getting back its own packet. The text does not
elaborate on this spoke-sending case. We are suppressing such "send-back-to-sender"
case in IP multicast Bidir-PIM, but that resulted in the most demanding
forwarding hardware, so i didn't want to add this optimization to BIER-TE
to make it easier deployable. There are other solutions, such as discarding
the packet on the BFER after its reflected back and/or using flow
overlay reflectors, e.g.: spokes send to the reflector and the reflector
is the hub. This is what one would recommend to do in IP multicast SSM too.
But too much solution detail for this doc.

> 1326    5.1.7.  Equal Cost MultiPath (ECMP)
> 
> [minor]: How a hash function is chosen is up implementations. This section talks
> about “polarization” and how it can be avoided, personally I think it’s not
> relevant.

The reason to explain polarization here is to to explain the desire 
for the ECMP adjacency to have a seed parameter, so that controllers
can seed the ECMP adjacencies in these type of multi-stage topologies
differently to avoid polarization. I did run into these polarization
issues in IP multicast in these topologies in the past and the common
ECMP algos from vendors in IP multicast did/do not have such seed
parameters, thats why i felt this was the appropriate and simple
solution and relevant.

> 1475       segments: (1) BFR2 via link L1, (2) BFR2 via link L2, or (3) via
> 1476       BFR3.
> 
> [nits]: I think it should be: (3) BFR3 L0.

It was correct, but its good to see how it wasn't obvious to the
reader what the purpose was. I added a second link into BFR3 labelled the two links
as L3 and L4, and added explanation that in this case its meant to not matter whether
the traffic is to be passed into BFR3 via L3 or L4. Aka: BFR3 is the example
with the Node-Segment L0 address, whereas BFR2 is the example with the two
Adjacency SIDs L1, L2 - if one where to use SR terminology, which i rather want to
avoid to keep things simple.
> 
> 1494       bit positions can be re-used across multiple BFR to minimize the
> 
> [nit] s/bit/Bit

I am never sure if a term like "bit positions" that is actually a
name intended NOT to be capitalized should be capitalized at sentence start
or not, but i did it for now. But you wouldn't capitalize e.g.: 'e'
at the beginning of a sentence (https://simple.wikipedia.org/wiki/E_(mathematical_constant)).

> [nit] s/BFR/BFRs

Fixed (several places).
> 
> 1492    5.1.9.  Reuse of bit positions (without DNC)
> 
> [comments]: About the reuse of bit positions in this section, my understand is
> that this needs to be calculated carefully to meet condition (A) or (B). In
> case of new Multicast overflows added or topology changes, there are risks
> neither of these two condition holds any more, so the BP has to be reassigned
> hence BitStrings at BFIR and BIFTs.

Yes.  I'd probably say "carefully designed by humans" or
 "brute force calculated by controllers".

In either case, any address planning is prudently done for a predictable
topology growth scenario and not just for the current state of the topology,
or else you run into renumbering pains (like many enterprises did in IP
and iP multicast when they didn't know/observe this rule upfront).

Thanks a lot for the review!

Toerless