Subject: [Rift] AD Review of draft-ietf-rift-rift-12 (Part 3)

From: Alvaro Retana <[email-masked]>
Date: Tue, 25 May 2021 17:49:49 -0400

Dear authors:

I just finished reading through the LIE FSM (§4.2.2.1) -- this is part
3 of my review.

As I read through the FSM I found a lot of instances where terminology
is used inconsistently: different spelling, inconsistent
capitalization, the same/or very similar names for different things,
etc.  These are basic/minor issues, but they make the text hard to
follow.  These are clearly the types of issues that should be
identified and addressed by the WG!

I will wait for -13 to continue my review.  Please address the
consistency issues (which I fear may also be present in the rest of
the document) by then.

Thanks!

Alvaro.

[Line numbers from idnits.]

...
1507	4.2.2.1.  LIE FSM

1509	   This section specifies the precise, normative LIE FSM and can be
1510	   omitted unless the reader is pursuing an implementation of the
1511	   protocol.

[] I guess that the last part will be superseded by the reader guide:
"...and can be omitted..."

[major] Structure of the FSM.

Only one State is mentioned outside of the "Actions" list below -- and
none are described.  Some of the state names may be obvious
("TwoWay"), but others are not ("MultipleNeighborsWait").  Please
provide a short (one line) description.

Also, only one Event is listed outside of the "Events" list below.
Please describe all of them in one place.

The FSM figure is long -- and may interrupt the flow if some text is
before it and some after.  It looks like most of it is after it in
this section.  Please add a Figure number/description and refer to it
before it is presented.

It seems to me that a logical order might be to first describe the
"well known procedures", then the events, and finally the actions
(including the State descriptions).  [**I am reviewing this section in
that order** -- after looking at the initial paragraphs before the FSM
figure.]

I would love to see a description similar to rfc4271/§8, but I can
live with other formats.

The "Event [Action]" terminology from the diagram should be explained.

The characterization of some of the terms (SendLie is a good example
below) is confusing to me: I think of events as things that happen
(LieRcvd, a timer expires, etc.), and an action as the result of that
trigger (SendLie, move to a different State, etc.).  The
Events/Actions are not all treated in the same way.  If your
understanding is different, please explain.

...
1656	   Events
...
1690	   o  HoldtimeExpired: adjacency hold down expired

[major] s/hold down/holddown time
That is what is used in the schema: "holddown time".

...but also holdtime (not holddown) is used.

The diagram of the FSM includes several HoldTimerExpired.

Also, the diagram shows this:

      TimerTick [PUSH SendLIE,
                 IF HoldTimer expired
                  PUSH HoldTimerExpired]

...but HoldTimer is not defined anywhere.  Back to the schema:

/** default LIE FSM holddown time */
const TimeIntervalInSecType   default_lie_holdtime  = 3
...
 struct LIEPacket {
...
    /** Required holdtime of the adjacency, i.e. how much time
        MUST expire without LIE for the adjacency to drop. */
    12: required common.TimeIntervalInSecType
             holdtime = common.default_lie_holdtime;

The terminology is not consistent.

Also, the schema contains the normative requirement (MUST).  That
should be placed (if anywhere) in this section.

...
1694	   o  MultipleNeighborsDone: cooldown for multiple neighbors expired

[major] Inconsistent terminology...

"cooldown" doesn't show up anywhere.

The FSM diagram includes:

    TimerTick [IF MulNeighTimer expired
               PUSH MultipleNeighborsDone]

...but MulNeighTimer is not defined.

The Actions list (below) does include "multiple neighbors timer as 4 *
DEFAULT_LIE_HOLDTIME", which takes us back to the holdtimer above (see
HoldtimeExpired) as it seems that you're referring to the same timer,
except that it is called DEFAULT_LIE_HOLDTIME here and
default_lie_holdtime in the schema...and "Default LIE Holdtime" in
Table 6.

Note also that this multiple neighbors timer is (in the diagram)
started with still another name: "MultipleNeighbors
[StartMulNeighTimer]".  I appreciate that in some of these cases the
intent may be to save space by abbreviating, but the result is
inconsistency.

1696	   o  SendLie: send a LIE out

[minor] Is this an Event, an Action, or a procedure?  I understand an
event to be something that happens (LieRcvd), an action something that
is triggered (I would have assumed SendLie to be a good example), and
there is also a SEND_LIE procedure.  Is there are difference between
SendLie and SEND_LIE?

...
1700	   Actions

[major] This section is all over the place.  It is not ordered by
starting State, or ending State, or Action, or Event, or
alphabetically, or..??   Please order this section in some way;
suggestion: by starting State.

...
1705	      on NeighborDroppedReflection in ThreeWay finishes in TwoWay: no
1706	      action

[major] Why doesn't this event finish in OneWay?  If the neighbor
doesn't see me anymore...

...
1713	      on NewNeighbor in TwoWay finishes in MultipleNeighborsWait: PUSH
1714	      SendLie event

[diagram] "NewNeighbor [StartMulNeighTimer]"

[major] Why is the NewNeighbor event in the TwoWay state and not just
MultipleNeighbors?  The ThreeWay state doesn't use NewNeighbor for the
same transition.

1716	      on LieRcvd in OneWay finishes in OneWay: PROCESS_LIE

[minor] The diagram uses "LieReceived [ProcessLIE]"; neither name is correct.

...
1736	      on LieRcvd in ThreeWay finishes in ThreeWay: PROCESS_LIE

[diagram] "LieReceived [ProcessLIE]"

...
1750	      on UpdateZTPOffer in OneWay finishes in OneWay: send offer to ZTP
1751	      FSM

[minor] The diagram uses both "SendOfferToZTP" and "SendOfferToZTPFSM".

...
1758	      on TimerTick in OneWay finishes in OneWay: PUSH SendLie event

[minor] From the diagram: "TimerTick [PUSH SendLIE]"; inconsistent name.

[major] The OneWay state is the only one where the HoldTimer is not
checked on TimerTick; there's a separate HoldTimerExpired event for
it.  Why?

...
1785	      on TimerTick in MultipleNeighborsWait finishes in
1786	      MultipleNeighborsWait: decrement MultipleNeighbors timer, if
1787	      expired PUSH MultipleNeighborsDone

[major] This is not the only place in the FSM where (according to the
diagram) a timer is checked for expiration, but it is the only place
where the timer value is decremented.  Why?

The other timers (Holdtime) are not decremented anywhere.

Also, the MultipleNeighborsDone event is called out separately -- that
seems to imply that the timer can expire at any time...

...
1795	      on LieRcvd in TwoWay finishes in TwoWay: PROCESS_LIE

[diagram] "LIERcvd [ProcessLIE]" -- again, neither name is correct.

...
1804	      on LevelChanged in OneWay finishes in OneWay: update level with
1805	      event value, PUSH SendLie event

[minor] From the diagram: "LevelChanged [UpdateLevel, PUSH SendLie]";
the UpdateLevel term is used.

1807	      on SendLie in OneWay finishes in OneWay: SEND_LIE

[major] There are other events that result in "PUSH SendLie event".
I'm guessing that it results in SendLie...which then redirects to
SEND_LIE.  And the diagram says this:

   SendLIE [SendLIE]
   TimerTick [PUSH SendLIE]

SendLIE results in SendLIE...which seems to be some type of loop. :-(

1809	      on HATChanged in OneWay finishes in OneWay: store HAT

[diagram] "HATChanged [StoreHAT]"

1811	      on LevelChanged in TwoWay finishes in TwoWay: update level with
1812	      event value

[major] The diagram shows "LevelChanged [StoreLevel]" finishing both
in OneWay and TwoWay.

...
1828	      on HALChanged in OneWay finishes in OneWay: store new HAL

[diagram] "HALChanged [StoreHAL]"

1830	      on UnacceptableHeader in OneWay finishes in OneWay: no action

[minor] The diagram is missing the action:
s/UnacceptableHeader/UnacceptableHeader [-]

...
1848	      on TimerTick in TwoWay finishes in TwoWay: PUSH SendLie event, if
1849	      holdtime expired PUSH HoldtimeExpired event

[major] If the holdtime expired, why doesn't what event finish in
OneWay?  HoldtimeExpired is PUSHed, but (except for the mention in
OneWay) there's no place that points at it finishing in OneWay.

[major] Also, what is the value of sending a LIE if the holdtime
expired?  The description sounds as if the LIE would be sent before
checking the timer, which would result in the neighbor being
reflected.

1851	      on HALSChanged in OneWay finishes in OneWay: store HALS

[diagram] "HALSChanged [StoreHALS]"

...
1858	   Following words are used for well known procedures:

[nit] s/Following words/The following words

1860	   1.  PUSH Event: pushes an event to be executed by the FSM upon exit
1861	       of this action

[minor] Please don't define "PUSH" using "push"...

[minor] "an event...exit of this action"  As mentioned elsewhere,
isn't an event something that is used as a trigger...   If the
terminology is "backwards" (I might be the one backwards), please
clarify at the start.

1863	   2.  CLEANUP: neighbor MUST be reset to unknown

[major] Is there an external action to be taken?  For example,
removing the neighbor form the neighbor list and sending an updated
LIE, or something like that?

1865	   3.  SEND_LIE: create a new LIE packet

[minor] I assume this is create and send, or just create?

1867	       1.  reflecting the neighbor if known and valid and

[major] "reflecting the neighbor"  Please be clear in defining what
"reflecting" means.  This goes back to introducing the packets so we
know there's a Neighbor field in the LIEPacket...

[major] "neighbor if known and valid"  What is a "valid" neighbor?  I
guess that maybe is has to do with step 5 in PROCESS_LIE.  If that's
it, please say so somewhere: PROCESS_LIE doesn't say anything about
validity either.

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

[?] I'm lost.  The level is derived during the neighbor establishment,
right?  Why would the router remember information from a previous
neighbor?  I couldn't find other places that talk about the "last
known neighbor".

1872	       3.  setting `you_are_flood_repeater` to computed value

[major] Where is this computed?  As far as I can tell, the
"computation" is simply figuring out whether the neighbor is
northbound or southbound -- but that hasn't been discussed yet.

1874	   4.  PROCESS_LIE:

1876	       1.  if lie has wrong major version OR our own system ID or
1877	           invalid system ID then CLEANUP else

[major] What does having the "wrong major version" imply?  I'm
guessing that it means that the neighbor's LIE doesn't contain the
same major version as the local router.   ??

[major] s/OR/or/g   Not a normative word.

[nit] s/else/, else/g

1879	       2.  if lie has non matching MTUs then CLEANUP, PUSH
1880	           UpdateZTPOffer, PUSH MTUMismatch else

[nit] s/lie/LIE/g

1882	       3.  if PoD rules do not allow adjacency forming then CLEANUP,
1883	           PUSH PODMismatch, PUSH MTUMismatch else

[] "PUSH PODMismatch" and "PUSH MTUMismatch" both result in the same
thing.  Would it be clearer/simpler if simply an action to go back to
OneWay was used instead?

"PUSH UnacceptableHeader" also has the same result.

Is there a reason to keep these individual events around?  Is there an
expectation that there might be a notification ("telemetry, yang, open
config, splunk channel, dancing bears") for them?

1885	       4.  if lie has undefined level OR my level is undefined OR this
1886	           node is leaf and remote level lower than HAT OR (lie's level
1887	           is not leaf AND its difference is more than one from my
1888	           level) then CLEANUP, PUSH UpdateZTPOffer, PUSH
1889	           UnacceptableHeader else

[] "my level"  You mean the level of the local router is not configured/known...

[nit] s/node is leaf and remote level lower/node is a leaf and the
remote level is lower

[minor] "OR (..."  It looks like you should take the parenthesis out.

1891	       5.  PUSH UpdateZTPOffer, construct temporary new neighbor
1892	           structure with values from lie, if no current neighbor exists
1893	           then set neighbor to new neighbor, PUSH NewNeighbor event,
1894	           CHECK_THREE_WAY else

[major] If the neighbor is new, then we're still in OneWay, which
means that CHECK_THREE_WAY will result in nothing (from below).  IOW,
it looks like CHECK_THREE_WAY is not necessary at this point.  Am I
missing something?

[major] I'm a little confused about the process.  The steps above used
if/else to walk through the steps 1-5, but what does the "else" in #5
(above) means?  The steps below seem to be dependent on the "if" in
step 5 being true...but then I'm not sure where the "else" points to
if this is the last step.  It looks like the steps below should be
numbered as 6...

1896	           1.  if current neighbor system ID differs from lie's system
1897	               ID then PUSH MultipleNeighbors else

1899	           2.  if current neighbor stored level differs from lie's level
1900	               then PUSH NeighborChangedLevel else

1902	           3.  if current neighbor stored IPv4/v6 address differs from
1903	               lie's address then PUSH NeighborChangedAddress else

1905	           4.  if any of neighbor's flood address port, name, local
1906	               linkid changed then PUSH NeighborChangedMinorFields and

[nit] s/linkid/LinkID

1908	           5.  CHECK_THREE_WAY

[] Just to make sure I understand the logic: is CHECK_THREE_WAY only
executed if #4 is true (that's what it looks like from the "and"), or
is it executed if #1-3 are false?

I ask because all the PUSH actions result in moving to OneWay, which
means that CHECK_THREE_WAY will result in nothing.

1910	   5.  CHECK_THREE_WAY: if current state is one-way do nothing else

1912	       1.  if lie packet does not contain neighbor then if current state
1913	           is three-way then PUSH NeighborDroppedReflection else

[] "does not contain neighbor"  In other places reflection (or not
reflected) is used.  Please be consistent.

1915	       2.  if packet reflects this system's ID and local port and state
1916	           is three-way then PUSH event ValidReflection else PUSH event
1917	           MultipleNeighbors

[EoR-3]