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 -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
