Subject: [Rift] AD Review of draft-ietf-rift-rift-16 (4.2.3.3.1)

From: Alvaro Retana <[email-masked]>
Date: Wed, 15 Feb 2023 05:00:02 -0800

Hi!

This message contains §4.2.3.3.1 and a couple of "detours" -- I
pointed those out with separators; for example I dug introduce
you_are_sending_too_quickly and the comments below indicate that this
way:

===== <you_are_sending_too_quickly> =====
...
===== </you_are_sending_too_quickly> =====

Alvaro.

2448	4.2.3.3.1.  Normative Flooding Procedures

2450	   On reception of a TIE with an undefined level value in the packet
2451	   header the node MAY issue a warning and indiscriminately discard the
2452	   packet.  Such packets can be useful however to establish e.g. via
2453	   `instance_name`, `name` and `originator` elements in `LIEPacket`
2454	   whether the cabling of the node fulfills expectations, even before
2455	   ZTP procedures determine levels across the topology.

[] This paragraph seems out of place.

[major] "MAY issue a warning and indiscriminately discard the packet"

Given that this is an option, what are the considerations to do so?
Is the warning due to the "TIE with an undefined level" or to the fact
that the packer was discarded?  If the former, can the actions be
performed independently of each other; for example, drop the packet
but not log a warning?

[minor] "indiscriminately discard...packets can be useful"

I'm guessing that "indiscriminately" was used to emphasize the action.
If the packet can se useful, but not flooded (since we're in this
section), maybe the correct action is to ignore (not to discard).  ??

[minor] "useful however to establish e.g. via `instance_name`, `name`
and `originator` elements in `LIEPacket` whether the cabling of the
node fulfills expectations"

I'm guessing this is not explained anywhere and that it is probably an
operational issue to be covered in the applicability document, right?

2457	   This section specifies the precise, normative flooding mechanism and
2458	   can be omitted unless the reader is pursuing an implementation of the
2459	   protocol or looks for a deep understanding of underlying information
2460	   distribution mechanism.

[] This seems to be a better first-paragraph for the section.

2462	   Flooding Procedures are described in terms of a flooding state of an
2463	   adjacency and resulting operations on it driven by packet arrivals.
2464	   The FSM itself has basically just a single state and is not well
2465	   suited to represent the behavior.  An implementation MUST either
2466	   implement the given procedures in a verbatim manner or behave on the
2467	   wire in the same way as the provided normative procedures of this
2468	   paragraph.

[nit] s/a flooding state/the flooding state

[minor] "The FSM itself has basically just a single state and is not
well suited to represent the behavior."

There's no TIE FSM, right?  Then why even mention it?

[major] "MUST either implement the given procedures in a verbatim
manner or behave on the wire in the same way as the provided normative
procedures of this paragraph."

Normative requirements with options are not great.  s/MUST/is expected to

Also, I don't know what the difference is between the options.

2470	   RIFT does not specify any kind of flood rate limiting since such
2471	   specifications always assume particular points in available
2472	   technology speeds and feeds and those points are shifting at faster
2473	   and faster rate (speed of light holding for the moment).

[] Judgement about other technology. :-(

Suggestion> RIFT does not specify any kind of flood rate limiting.
Instead, the packets provide hints...

2475	   To help with adjustement of flooding speeds the encoded packets
2476	   provide hints to react accordingly to losses or overruns via
2477	   `you_are_sending_too_quickly` in `LIEPacket` and `Packet Number` in
2478	   security envelope described in Section 4.4.3.  Flooding of all
2479	   corresponding topology exchange elements SHOULD be performed at
2480	   highest feasible rate whereas the rate of transmission MUST be
2481	   throttled by reacting to packet elements and adequate features of the
2482	   system such as e.g. queue lengths or congestion indications in the
2483	   protocol packets.

[nit] s/in `LIEPacket`/in the `LIEPacket`

[nit] s/in security envelope/in the security envelope

===== <you_are_sending_too_quickly> =====

[] I went looking for `you_are_sending_too_quickly`:

2952	4.2.3.5.  'Flood Only Node TIEs' Bit

2954	   RIFT includes an optional ECN (Explicit Congestion Notification)
2955	   mechanism to prevent "flooding inrush" on restart or bring-up with
2956	   many southbound neighbors.  A node MAY set on its LIEs the
2957	   corresponding `you_are_sending_too_quickly` flag to indicate to the
2958	   neighbor that it should temporarily flood node TIEs only to it and
2959	   slow down the flooding of any other TIEs.  It SHOULD only set it in
2960	   the southbound direction.  The receiving node SHOULD accommodate the
2961	   request to lessen the flooding load on the affected node if south of
2962	   the sender and SHOULD ignore the indication if northbound.

[nit] s/flood node TIEs only/only flood Node TIEs

[major] "It SHOULD only set it in the southbound direction."

The "SHOULD only" construct is always confusing.

s/
   A node MAY set on its LIEs the corresponding `you_are_sending_too_quickly`
   flag to indicate to the neighbor that it should temporarily flood node TIEs
   only to it and slow down the flooding of any other TIEs.  It SHOULD only
   set it in the southbound direction.
/
   A node MAY set the `you_are_sending_too_quickly` flag in its LIEs to
   indicate to southbound neighbors to temporarily only flood Node TIEs
   to it and slow down the flooding of any other TIEs.

[major] "The receiving node ...SHOULD ignore the indication if northbound."

s/northbound/north of the sender

When is it ok to not ignore the indication if north of the sender?
Why is this action recommended and not required?  The schema says that
it is "Ignored when received from southbound neighbor.", which sounds
to me more like a "MUST".

2964	   Obviously this mechanism is most useful in the southbound direction.
2965	   The distribution of node TIEs guarantees correct behavior of
2966	   algorithms like disaggregation or default route origination.
2967	   Furthermore though, the use of this bit presents an inherent trade-
2968	   off between processing load and convergence speed since suppressing
2969	   flooding of northbound prefixes from neighbors permanently will lead
2970	   to traffic loss.

[minor] "Obviously this mechanism is most useful in the southbound direction."

It may not be obvious to everyone...

Is there any utility in the other direction?  If
`you_are_sending_too_quickly` is ignored anyway, it seems like this
sentence is not needed.

===== </you_are_sending_too_quickly> =====

===== <Packet Number> =====

[] I also went looking for `Packet Number`.

Where's the Security Envelope in the schema?

>From §4.4.3:

5275	   Packet Number:
5276	      16 bits.  An optional, per adjacency, per packet type
5277	      monotonically increasing number rolling over using sequence number
5278	      arithmetic defined in Appendix A.  A node SHOULD correctly set the
5279	      number on subsequent packets or otherwise MUST set the value to
5280	      `undefined_packet_number` as provided in the schema.  This number
5281	      can be used to detect losses and misordering in flooding for
5282	      either operational purposes or in implementation to adjust
5283	      flooding behavior to current link or buffer quality.  This number
5284	      MUST NOT be used to discard or validate the correctness of
5285	      packets.  Packet numbers are incremented on each interface and
5286	      within that for each type of packet independently.  This allows to
5287	      parallelize packet generation and processing for different types
5288	      within an implementation if so desired.

[nit] s/using sequence number arithmetic/using the sequence number arithmetic

[major] "SHOULD correctly set the number on subsequent packets"

This is not a good Normative statement -- among other things, by
recommending it you're opening the door for it to be ok for the number
to not be "correctly set", not to mention the definition of "correct",
etc.

Given the references to Appendix A, it is Normative (right?), so
perhaps the right approach is to simply drop the sentence.

Suggestion>
s/
   An optional, per adjacency, per packet type monotonically increasing
   number rolling over using sequence number arithmetic defined in
   Appendix A.  A node SHOULD correctly set the number on subsequent
   packets or otherwise MUST set the value to `undefined_packet_number`
   as provided in the schema.
/
   An optional, per adjacency, per packet type number set using the
   sequence number arithmetic defined in Appendix A.  If the arithmetic
   in Appendix A is not used the node MUST set the value to
   `undefined_packet_number`.

===== </Packet Number> =====

===== <Appendix A> =====

[] I also looked at Appendix A.

6968	Appendix A.  Sequence Number Binary Arithmetic

6970	   The only reasonably reference to a cleaner than [RFC1982] sequence
6971	   number solution is given in [Wikipedia].  It basically converts the
6972	   problem into two complement's arithmetic.  Assuming a straight two
6973	   complement's subtractions on the bit-width of the sequence number the
6974	   corresponding >: and =: relations are defined as:

[] I'm not going to pretend to understand the details, but it seems
that you're saying that RFC1982 is not great and that what is on
Wikipedia is better -- Wikipedia claims the same thing.  Besides not
comparing what RIFT uses with other technology...

The references to Appendix A are Normative (as in, Appendix A is how
RIFT does it), right?  But the reference to RFC1982 is listed as
Normative while the one to Wikipedia is Informative.  Which one should
the implementation follow?  The options I see are: RFC1982, Wikipedia,
or Appendix A (the specification is self-contained there).

I would be very happy with the answer being Appendix A as Wikipedia is
not really a stable reference and you already said that RFC1982 is not
a good solution.  This would mean eliminating the two references and
relying on the text in the Appendix.

===== </Appendix A> =====

2485	   A node SHOULD NOT send out any topology information elements if the
2486	   adjacency is not in a "ThreeWay" state.  No further tightening of
2487	   this rule as to e.g. sequence is possible due to possible link
2488	   buffering and re-ordering of LIEs and TIEs/TIDEs/TIREs in a real
2489	   implementation for e.g.  performance purposes.

[major] "SHOULD NOT send...if the adjacency is not in a "ThreeWay" state"

Is it ok to ever send TIEs if not in ThreeWay?  Why is this behavior
recommended and not required?

§4.2.2.1 defines ThreeWay as:

   ThreeWay: this state signifies that ThreeWay valid LIEs from a
   neighbor are received. On achieving this state the link can be
   advertised in `neighbors` element in `NodeTIEElement`.

I read that to mean that the TIE describing the local node MUST NOT be
sent unless in ThreeWay.  But that is different from forwarding other
TIEs.

I saw that the next sentence says that "No further tightening of this
rule as to e.g. sequence is possible..." but what you wrote is not
clear (maybe missing a couple of commas) and it doesn't seem to be
related to origination vs flooding.

2491	   A node MUST drop any received TIEs/TIDEs/TIREs unless it is in
2492	   ThreeWay state.

[] This sentence sort of answers some of my questions above.

2494	   TIDEs and TIREs MUST NOT be re-flooded the way TIEs of other nodes
2495	   MUST be always generated by the node itself and cross only to the
2496	   neighboring node.

[] Suggestion>
   TIEs generated by other nodes MUST be re-flooded.  TIDEs and TIREs
   MUST NOT be re-flooded.