Re: [Rift] AD Review of draft-ietf-rift-rift-12 (Part 1)

[Alvaro Retana <[e-mail]>]

On May 10, 2021 at 5:08:45 PM, Tony Przygienda wrote:


Tony/authors:

Hi!

After clearing some of my queue, I'm back on this document...still
will be interleaving with others.  :-(


Alvaro.


...
> > Part 1 includes the introductory text through the end of §4.1 (Overview).
> >
> > While I appreciate the overview of the topologies and the protocol, I
> > think that this extended introductory material is at times too
> > long/wordy and complex (for the average reader, sometimes being forced
> > to make assumptions or jump ahead), but also incomplete. To be more
> > specific:
>
> reader's guide upfront should help that, i.e. guide readers depending on
> what they need to correct section, tell them what needs reading

It does.  Thanks!

I'm appending at the bottom some comments on the new section and other
new text, based on -15.


> > - The description of the general topology (§4.1.2) gets in to significant
> > detail and complexity, including the way in which the concepts are depicted
> > in the figures. (ASCII art is not the best, you might want to take
> > advantage of using SVI.) I noticed that none of the figures/sections in
> > §4.1.2 are referenced elsewhere (but the simple Figures 2 and 3 are), which
> > makes me think about the value of the in-depth treatment if it will not be
> > explicitly considered later on.
>
> Jordan is preparing very nice SVGs. We will retain the ASCII form (it's
> actually easier on the mind _once_ one understands it is the feeling
> by most authors) while the SVG will allow an "intuitive first understanding".

That's fine.

However, there are some differences between the figures.  I realize
that some of the details won't be able to fit in the ASCII version --
but please try to keep them as close as possible.

The most important thing is for the descriptions to apply to whichever
version of the document is being used.

- Figure 1: the SVG shows 32-bit addresses (A.A.A.A/32), but the ASCII
only has room for A/32.  It is not obvious that A/32 and A.A.A.A/32
are the same.

The text describing the figure says this: "...the notation A/32 is
used to indicate a loopback route to node A and 0/0 is the usual
notation for a default route."  But A/32 is not in the SVG...  Bottom
line: be mindful in checking that the text and the figures match.  In
this case it looks like the easy fix is to use A/32 in the SVG.


- Figure 2: There are some subtle differences like the indication of
directionality: for example, the ASCII explicitly shows an arrow ("^")
to indicate that P111/2 is going up, but the SVG doesn't (it uses a
partial line from the node sending the information).  The reader's
digest mentions that the indication of direction are of "paramount
importance", so this seems like a good thing to be consistent about.

The SVG has an "All TIEs" indication on the side, covering the whole
topology.  But the ASCII seems to indicate that "All TIEs" go from
Spin112 to ToF 22 only.


- Figure 3: the figures are completely different!

- Figure 7: the nodes are not tagged in the ASCII version.

- Figure 8: the nodes have a different tag.

- Figure 13: the figures are completely different!


> We will check figure references and make sure they are all refered to
> after we have the negative section reordered (example front) and SVG done.

I think that Figures 16 and 28 are still not referenced anywhere.



...
> > 273 2. Introduction
...
> > [nit] s/Fat-Tree/Fat Tree/g To be consistent with the terminology section.
>
> done, this was the only instance

[] Not considering the references, I still count 5 instances.



...
> > [minor] s/NOT/not/g This is not one of the rfc2119 keywords, so it
> > should not be capitalized. I know you're doing it for emphasis, but
> > it will be eventually changed -- so let's take care of it now.
>
> replaced whole document non-normative NOT with *not* and AND with *and*

Ok.

There are still one "NOT" (in the Node TIE definition) and one "AND"
(in 4.2.2.1) left.

...




*****
What follows are comments on -15 covering the same sections (through
the end of §4.1 (Overview)).
*****


[Line numbers from idnits.]

...
166	1.  Introduction
...
199	   The protocol does further provide

[nit] Add a ":".


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

[] "traditional" carries some non-inclusive connotations.  Consider an
alternative, suggestions:

      - classic
      - classical
      - common
      - conventional
      - customary
      - fixed
      - habitual
      - historic
      - long-established
      - popular
      - prescribed
      - regular
      - rooted
      - time-honored
      - universal
      - widely used
      - widespread


...
208	   *  automatic pruning and load balancing of topology flooding
209	      exchanges over a sufficient subset of links which resolves the
210	      traditional problem of link-state protocol struggling with densely
211	      meshed graphs due to high volume of flooding traffic
212	      (Section 4.2.3.9),

[major] "...which resolves the traditional problem of link-state
protocol struggling"   Comparing with other protocols -- and the
problem is not described anyway.

Suggestion>
   *  automatic pruning and load balancing of topology flooding
      exchanges over a sufficient subset of links (Section 4.2.3.9),



214	   *  automatic aggregation (Section 4.2.3.8) and consequently automatic
215	      disaggregation (Section 4.2.5) of prefixes on link and node
216	      failures to prevent black-holing and suboptimal routing,

[] "black-holing" and other variations ("blackhole") are used in
several parts.  Please consider using more inclusive terminology.
Perhaps something like "dropped" or "lost traffic".

https://www.ietf.org/about/groups/iesg/statements/on-inclusive-language/


...
223	   *  re-balancing of traffic towards the spines based on bandwidth
224	      available (Section 4.3.7.1) and finally

[nit] s/ and finally/, and finally


...
272	1.1.  Requirements Language

274	   The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
275	   "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and
276	   "OPTIONAL" in this document are to be interpreted as described in BCP
277	   14 RFC 2119 [RFC2119] RFC 8174 [RFC8174] when, and only when, they
278	   appear in all capitals, as shown here.

[major] The boilerplate us not exactly what rfc8174 says, so idnits is
complaining.

      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 difference seems to be in how the documents are referenced:  s/BCP
14 RFC 2119 [RFC2119] RFC 8174 [RFC8174]/BCP 14 [RFC2119] [RFC8174]


280	2.  A Reader's Digest

[minor] Given that readers may come to this section before any other
part of the document, please expand all acronyms on first mention.


...
288	   The indications to direction (i.e. "top", "bottom", etc.) referenced
289	   in the Section 1 are of paramount importance.  RIFT requires a
290	   topology with a sense top and bottom in order to properly achieve a
291	   sorted topology.  Clos, Fat-Tree, and other similarly structured
292	   networks are conducive to such requirements.  RIFT does allow for
293	   further relaxation of these constraints, they will be mentioned later
294	   in this section.

[nit] s/indications to direction/indications of direction


[nit] s/sense top/sense of top


296	   Operators and implementors alike must understand if multi-plane IP
297	   fabrics are of interest or not.  Section 3.2 illustrates an example
298	   of both single-plane in Figure 2 and multi-plane fabric in Figure 3.
299	   Multi-plane fabrics require understanding of additional RIFT concepts
300	   (e.g.  negative disaggregation in Section 4.2.5.2) that are otherwise
301	   unnecessary in context of strictly single-plane fabrics.  Overview
302	   (Section 4.1) and Section 4.1.2 aim to provide enough context to
303	   determine if multi-plane fabrics are of interest to the reader.  The
304	   Fallen Leaf part (Section 4.1.3), and additionally Section 4.1.4 and
305	   Section 4.1.5 describe further considerations that are specific to
306	   multi-plane fabrics.

[nit] s/Overview/The Overview


308	   The fundamental protocol concepts are described starting in the
309	   specification part (Section 4.2), but some sub-sections are not quite
310	   as relevant unless dealing with implementation of the protocol.  The
311	   protocol transport (Section 4.2.1) is of particular importance for
312	   two reasons.  First, it introduces RIFT's packet formats in the form
313	   of a normative Thrift model given in Appendix B.3.  Second, the
314	   Thrift model component is a prelude to understanding the RIFT's
315	   inherent security features as defined in the security segment
316	   (Section 7).  The normative schema defining the Thrift model can be
317	   found in both Appendix B.2 and Appendix B.3.  Furthermore, while a
318	   detailed understanding of Thrift and the models are not required
319	   unless implementing RIFT, they may provide additional useful
320	   information for other readers.

[minor] "RIFT's inherent security features as defined in the security
segment (Section 7)"    §7 makes sense, but the model, mechanisms,
etc. are specified in §4.4.  Should there be a pointed to that too?


[nit] s/understanding...are not required/understanding...is not required


322	   If implementing RIFT to support multi-plane topologies Section 4.2
323	   should be reviewed in its entirety in conjunction with previously
324	   mentioned Thrift schemas.  Sections not relevant to single-plane
325	   implementations will be noted later in the section.  Special
326	   attention should be paid to the LIE definitions part (Section 4.2.2)
327	   as it not only outlines basic neighbor discovery and adjacency
328	   formation, but also provides necessary context for RIFT's ZTP
329	   (Section 4.2.7) and mis-cabling detection capabilities that allow it
330	   to automatically detect and build the underlay topology with a
331	   negligible configuration.  These specific capabilities are detailed
332	   in Section 4.2.7.

[nit] s/with previously mentioned/with the previously mentioned


[nit] s/with a negligible configuration/with negligible configuration


334	   For other readers, the following sections provide a more detailed
335	   understanding of the fundamental properties and highlight some
336	   additional benefits of RIFT such as link state packet formats, highly
337	   efficient flooding, synchronization, loop-free path computation and
338	   link-state database maintenance - Section 4.2.3, Section 4.2.3.2,
339	   Section 4.2.3.3, Section 4.2.3.4, Section 4.2.3.6, Section 4.2.3.7,
340	   Section 4.2.3.8, Section 4.2.4, Section 4.2.4.1, Section 4.2.4.2,
341	   Section 4.2.4.3, Section 4.2.4.4.  RIFT's unique ability to perform
342	   weighted unequal-cost load balancing of traffic across all available
343	   links is outlined in Section 4.3.7 with an accompanying example.

[] Let's stay away from "marketing"-like adjectives: highly, unique, etc..


...
356	   Section 5 contains a set of comprehensive examples that continue to
357	   highlight just how efficiently RIFT handles failures by containing
358	   impact to only the required set of nodes.  It should also help cement
359	   some of RIFT's core concepts in the reader's mind.

[] s/continue to highlight just how efficiently RIFT handles failures
by containing impact/show how RIFT contains the impact of failures


...
367	   More information related to RIFT can be found in the "RIFT
368	   Applicability" [APPLICABILITY] document, which discusses alternate
369	   topologies upon which RIFT may be deployed, use cases where it is
370	   applicable, and presents operational considerations that complement
371	   this document.

[major] As we discussed, putting complementary information in that
other document is fine, but it should be a Normative reference.


...
375	3.1.  Terminology
...
401	   Level:
402	      Clos and Fat Tree networks are topologically partially ordered
403	      graphs and 'level' denotes the set of nodes at the same height in
404	      such a network, where the bottom level (leaf) is the level with
405	      lowest value.  A node has links to nodes one level down and/or one
406	      level up.  Under some circumstances, a node may have links to
407	      nodes at the same level and a leaf may have links to nodes
408	      multiple levels higher.  RIFT counts levels from top-of-fabric
409	      (ToF) numerically down.  Level 0 always implies a leaf in RIFT but
410	      a leaf does not have to be level 0.  Level in RIFT can be
411	      configured or automatically derive its level via ZTP as explained
412	      in Section 4.2.7.  As final footnote: Clos terminology uses often
413	      the concept of "stage" but due to the folded nature of the Fat
414	      Tree it is not used from this point on to prevent
415	      misunderstandings.

[nit] s/Level in RIFT can be configured or automatically derive its
level via ZTP/Level in RIFT can be configured or automatically derived
via ZTP


[nit] s/As final footnote/As a final footnote


...
456	   Top-of-fabric Plane or Partition:
457	      In large fabrics top-of-fabric switches may not have enough ports
458	      to aggregate all switches south of them and with that, the ToF is
459	      'split' into multiple independent planes.  Section 4.1.2 explains
460	      the concept in more detail.  A plane is subset of ToF nodes that
461	      see each other through south reflection or E-W links.

[nit] s/is subset/is a subset


...
635	   System ID:
636	      Each RIFT node identifies itself by a valid, network wide unique
637	      number when trying to build adjacencies or describing its
638	      topology.  RIFT System IDs can be auto-derived or configured.

[major] What is a "valid" System ID?  I found the answer in §4.2.2,
but given that we don't want to make this section an index, perhaps
simplify the definition here and leave the validity for later:

Suggestion>
   Each RIFT node identifies itself by a network-wide unique number...


...
719	4.  RIFT: Routing in Fat Trees

721	   Remainder of this documents presents the detailed specification of a
722	   protocol optimized for Routing in Fat Trees (RIFT) that in most
723	   abstract terms has many properties of a modified link-state protocol
724	   when distributing information northbound and a distance vector
725	   protocol when distributing information southbound.  While this is an
726	   unusual combination, it does quite naturally exhibit the desirable
727	   properties desired.

[nit] s/Remainder of this documents/The remainder of this document


...
1361	4.1.5.  Addressing the Fallen Leaves Problem
...
1369	   When used for the operation of disaggregation, a positive South TIE
1370	   contained in `positive_disaggregation_prefixes`, as usual, indicates
1371	   reachability to a prefix of given length and all addresses subsumed
1372	   by it.  In contrast, a negative route advertisement contained in
1373	   `negative_disaggregation_prefixes` indicates that the origin cannot
1374	   route to the advertised prefix.

[major] This paragraph was updated to include the "contained in
`positive_disaggregation_prefixes`" and "contained in
`negative_disaggregation_prefixes`" phrases.  This is the first time
these elements (?) are mentioned, so I went looking for a
description...and found this in the schema:

/** TIE carrying prefixes */
struct PrefixTIEElement {
    /** Prefixes with the associated attributes. */
    1: required map<common.IPPrefixType, PrefixAttributes> prefixes;
} (python.immutable = "")

...

/** Single element in a TIE. */
union TIEElement {
    /** Used in case of enum common.TIETypeType.NodeTIEType. */
    1: optional NodeTIEElement     node;
    /** Used in case of enum common.TIETypeType.PrefixTIEType. */
    2: optional PrefixTIEElement          prefixes;
    /** Positive prefixes (always southbound). */
    3: optional PrefixTIEElement   positive_disaggregation_prefixes;
    /** Transitive, negative prefixes (always southbound) */
    5: optional PrefixTIEElement   negative_disaggregation_prefixes;
    /** Externally reimported prefixes. */
    6: optional PrefixTIEElement          external_prefixes;
    /** Positive external disaggregated prefixes (always southbound). */
    7: optional PrefixTIEElement
            positive_external_disaggregation_prefixes;
    /** Key-Value store elements. */
    9: optional KeyValueTIEElement keyvalues;
} (python.immutable = "")


Both elements point at PrefixTIEElement, as does "prefixes".  I also
looked at PrefixAttributes (from PrefixTIEElement), but couldn't
figure out where the distinction between positive and negative is
made.

I also found Figure 18 (Adding Routes from South TIE Positive and
Negative Prefixes), which I realize is an example -- but I also
couldn't figure out how positive and negative prefixes are
advertised/treated differently.

How/where is the distinction between them (and with "prefixes") made?
This question is probably answered somewhere in the document, and
there might be some confusion in my reading of the schema.

After all this, what is the value of adding the references to
`positive_disaggregation_prefixes` and
`negative_disaggregation_prefixes` at this point in the text?


...
6676	11.1.  Normative References
...
6759	   [VFR]      Erlebach et al., T., "Cuts and Disjoint Paths in the
6760	              Valley-Free Path Model of Internet BGP Routing", Springer
6761	              Berlin Heidelberg Combinatorial and Algorithmic Aspects of
6762	              Networking, 2005.

[major] This reference should be Informative.



6764	11.2.  Informative References

6766	   [APPLICABILITY]
6767	              Wei, Y., Zhang, Z., Afanasiev, D., Thubert, P., and T.
6768	              Przygienda, "RIFT Applicability", Work in Progress,
6769	              Internet-Draft, draft-ietf-rift-applicability-10, 16
6770	              December 2021, <https://datatracker.ietf.org/doc/html/
6771	              draft-ietf-rift-applicability-10>.

[major] This reference should be Normative.


[EoR P1-15]