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

[James Guichard <[e-mail]>]

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?

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.

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

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

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.

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.

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.
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.

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.

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.

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.

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.

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

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

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.

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.

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

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

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.

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

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

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.

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?

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

2492        adjustement of flooding speeds the encoded packets provide hints to

Jim> typo 'adjustement' -> 'adjustment'

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.

Thanks!

Jim