Re: [Rift] AD review https://datatracker.ietf.org/doc/draft-ietf-rift-rift/ (v-18) (review up to Section 4.3)

[Jordan Head <[e-mail]>]

Thanks, Jim,

Replies inline as jhead>>

Keep them coming.



Juniper Business Use Only
From: James Guichard <[e-mail]>
Date: Thursday, September 21, 2023 at 7:09 AM
To: [e-mail] <[e-mail]>
Cc: [e-mail] <[e-mail]>, James Guichard <[e-mail]>
Subject: AD review https://datatracker.ietf.org/doc/draft-ietf-rift-rift/ (v-18) (review up to Section 4.3)
[External Email. Be cautious of content]

Dear authors,

Please find review comments (up to but not including section 4.3). I have used IDNITs format so that you can see the line numbers for easier review.

Section: Introduction:
202           *  optional fully automated construction of fat tree topologies based
203              on detection of links without any configuration (Section 4.2.7)
204              while allowing for conventional configuration methods and
205              arbitrary mix of both types of nodes as well,

Jim> I do not understand the last part of the above paragraph. Do you mean a mix of ZTP and non-ZTP nodes?

jhead>> Yes.

Section 4: RIFT: Routing in Fat Trees
767           a protocol optimized for Routing in Fat Trees (RIFT) that in most

Jim> No need to expand on RIFT as already did so earlier in the text.

jhead>> Ack.

771           unusual combination, it does quite naturally exhibit the desirable
772           properties desired.

Jim> Replace 'desirable properties desired' with 'desired properties'.

jhead>> Ack.

Section 4.1:  Overview
808           To account for the "northern" and the "southern" information split
809           the link state database is partitioned accordingly into "north
810           representation" and "south representation" TIEs.  In simplest terms

Jim> While TIE is in the terminology section it is probably appropriate to expand on it here on the first use.

jhead>> Ack.

811           the North TIEs contain a link state topology description of lower
812           levels and and South TIEs carry simply node description of the level

Jim> Remove duplicate 'and' in the above sentence.

jhead> Ack.

Section 4.1.3:  Fallen Leaf Problem
1258        In large fabrics or fabrics built from switches with low radix, the
1259        ToF ends often being partitioned in planes which makes the occurrence

Jim> "the ToF ends often being partitioned..." does not parse. I would suggest "the ToF may often become partitioned across planes.." or something like that.

jhead>> Sure, I’ll clarify the text.

Jim> You use both "Top-of-Fabric" and "ToF" terminology. Can you pick one and use that throughout the document? I would suggest ToF as that is defined
in section 3.1.

jhead>> Agreed. I’ll look through the document and converge on ToF.

1264        A "Fallen Leaf" is a leaf that can be reached by only a subset, but
1265        not all, of Top-of-Fabric nodes due to missing connectivity.  If R is

Jim> I would remove "but not all" as by definition a subset means "not all" of the ToF nodes can reach the leaf node.

jhead>> Ack.

Section 4.1.5:  Addressing the Fallen Leaves Problem

Jim> General comment: I think the document would be much easier to follow if sections 4.1.2 - 4.1.5 were moved into an appendix with a pointer to
that appendix in section 4.1.1 (for those readers interested in multiplane). For those readers not familiar with complex datacenter design it is
going to be very confusing to read and in some sense detracts from the specification which is the next section.

jhead>> I will concede that this topic quite dense, but that’s just the nature of multiplane fabrics. However, this is a protocol specification that at baseline is for those who plan to implement the protocol in its entirety. The Reader’s Digest section exists to help readers navigate the document depending upon their level and area of interest. For those that are interested in multiplane fabrics, moving the section to the appendix will not shield them from the complexity, but I do think it will make the document feel disjointed. For those who are not interested, be it an implementor or an operator, the Reader’s Digest will steer them around the topic.

Section 4.2:  Specification

1483        Any attempt to transition from a state towards another on reception
1484        of an event where no action is specified must be considered an

Jim> Should 'must' be 'MUST' in the above sentence? it seems that you want to mandate this action so therefore MUST seems more appropriate.

jhead>> Agreed, I’ll adjust it. The sentence following that one indicates as such.

1485        unrecoverable error, i.e. the protocol MUST reset all adjacencies,
1486        discard all the state and may not start again.

Jim> What does 'may not start again' actually mean? I assume you do not want to mandate that the router should not restart RIFT but perhaps change 'may not' to 'SHOULD NOT' or even 'RECOMMENDED' not to start again.

jhead>> Essentially we were trying not to over specify and ultimately restrict how one might implement this behavior (e.g. backoff mechanisms, etc.) After looking at it, simply removing “and may not start again.” accomplishes that.

1493        The machines can use conceptually "timers" for different situations.

Jim> Machines? I assume that you mean FSMs? if so please change the above.

jhead>> Yes, will clarify.

Section 4.2.1:  Transport

1505        All RIFT packet structures and their contents are defined in the
1506        Thrift [thrift] models in Appendix B.  The packet structure itself is

Jim> I assume that Appendix B is normative. Please add a note to say so here.

jhead>> Will do.

Section 4.2.2:  Link (Neighbor) Discovery (LIE Exchange)

1541        MUST be sent with an IPv4 Time to Live (TTL) or an IPv6 Hop Limit
1542        (HL) of either 1 or 255 to prevent RIFT information reaching beyond a

Jim> 1 or 255? Some guidance here is necessary. RFC5082 for example (GTSM) uses 255 although personally I am not sold on that. The main point are 'options' necessary (if not just pick 1 or 255) and if not, appropriate text should be added, or have something in the security considerations section.

jhead>> We’ve had several discussions on this in the past, ultimately due to the lack of strong consensus between using 1 or 255 in various protocol implementations, so we allowed for both. Per Alvaro’s recommendation, we’ve had discussions with the Applicability draft authors to add some of this to their draft, I’ll work on ensuring we get consensus on what is included in that document.

I’ll look at adding something to cover this in the Security Considerations section as well.

1551        An implementation may listen and send LIEs on IPv4 and/or IPv6

Jim> Should the above 'may' be 'MAY'?

jhead>> A normative MAY won’t do the trick here or we will end up with interesting interpretations, I’ll rework the phrasing to something that appropriately gets the point across.

1668        change any of its local addresses or stop IPv4 forwarding, it has to
1669        tear down and rebuild the adjacency.  It also has to remove any state

Jim> Change 'it has to' to 'MUST' in the above sentences.

jhead>> Ack.

1692        (such as LIE is considered a "minimally valid" LIE).  Observe that

Jim> Change 'as' to 'a' above.

jhead>> Ack.

Section 4.2.2.1:  LIE Finite State Machine
1884        *  CLEANUP: The FSM *conceptually* holds a 'current neighbor'
1885           variable that contains information received in the remote node's
1886           LIE that is processed against LIE validation rules.  In the event

Jim> A pointer to where LIE validation rules are specified would be helpful here.

jhead>> The LIE validation rules are just a few lines up at the very end of the previous sub-section, so a reader should have context as they read this section. Let me see if I can find a way to put them in their own sub-section.

1892           1.  reflecting the neighbor as described in ValidReflection and
1894           2.  setting the necessary `not_a_ztp_offer` variable if level was
1895               derived from the last known neighbor on this interface and
1897           3.  setting `you_are_flood_repeater` to computed value

Jim> I found the above confusing without the text indicating that 'ValidReflection' is part of the FSM whereas 'not_a_ztp_offer' and 'you_are_flood_repeater' are variables of the LIEPacket. Could you make this clear in the text somehow perhaps simply by inserting 'LIEPacket' in front of these variables in the text above?

jhead>> Here’s the current text as is.

   *  SEND_LIE: create and send a new LIE packet

      1.  reflecting the neighbor as described in ValidReflection and

      2.  setting the necessary `not_a_ztp_offer` variable if level was
          derived from the last known neighbor on this interface and

      3.  setting `you_are_flood_repeater` to computed value

jhead>> I think the fact that we say “create and send a new LIE packet” implies we’re already talking about a LIEPacket, however I do see what you mean. What about something like this where are a bit more explicit in what each thing is:

   *  SEND_LIE: create and send a new LIE packet

      1.  reflecting the `neighbor` element as described in ValidReflection and

      2.  setting the necessary `not_a_ztp_offer` variable if level was
          derived from the last known neighbor on this interface and

      3.  setting the `you_are_flood_repeater` variable to the computed value

2357        For Node TIEs to carry more adjacencies than fit into an MTU-sized
2358        packet, the element `neighbors` may contain a different set of
2359        neighbors in each TIE.  Those disjoint sets of neighbors MUST be
2360        joined during corresponding computation.  Nevertheless, in case
2361        across multiple Node TIEs

2363        1.  `capabilities` do not match *or*

2365        2.  `flags` values do not match *or*

2367        3.  same neighbor repeats in multiple TIEs with different values

Jim> I cannot make sense of the above - the text says "in case <blah> do not match etc.." but does not say what is the result of this (?)

jhead>> Will rephrase.

2492        adjustement of flooding speeds the encoded packets provide hints to

Jim> typo 'adjustement' -> 'adjustment'

jhead>> Ack.

2815        TIEs from its remote peer.  Such a publisher model can be implemented
2816        in many ways, either in a single thread of execution of in parallel
2817        threads.

Jim> The last sentence does not parse please correct.

jhead>> Sure, will rephrase.

Thanks!

Jim