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

From: Tony Przygienda <[email-masked]>
Date: Sun, 13 Jun 2021 17:31:02 +0200

inline, I also used those comments to improve the ZTP FSM accordingly

On Tue, May 25, 2021 at 11:50 PM Alvaro Retana <[email-masked]>
wrote:

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

done

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

you are correct, states were not included. fixed.

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

hmm, which event? The FSM text is generated programatically so I wouldn't
know
how it can miss anything but then again, e'thing possible.

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

figure has been broken up into multiple one due to limitations of xml2rfc
originally, now
it's merged again due to v3.

referenced @ start of section.

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

no problem, I moved well known procedures upfront now albeit IMO that puts
lots of non-obvious details upfront relating to
stuff that shows up later and I find that confusing.

>
> I would love to see a description similar to rfc4271/§8, but I can
> live with other formats.
>
>
this format is generated programmatically from running code and hence
is always consistent without possibility for text editing errors on ASCII
figures or
description of things.

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

Yes, Events happen (i.e. they are on queue & then being dequed and
processed)

They cause transitions from a state to another (or same), transitions
contain actions.

 entry/exit action allow to vastly simplify a machine and possibly prevent
repetition of
lots of same action description.

I thought that FSM format used was already well described in the very
beginning of specification section
but I extended it further including explaining what timers are etc.

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

added to begin of specificatoin section

<t>
    The term `holdtime` is used often as short-hand for `holddown
timer` and signifies either the
    length of the holding down period or the timer used to expire
after such period. Such timers are used to
    "hold down" state within an FSM that is cleaned if the machine
triggers a `HoldtimeExpired`
    event.
</t>

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

corrected

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

removed from schema. improved timer language.

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

added in beginner to section

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

defined `

multiple neighbors timer

`

unified, added `

multiple_neighbors_lie_holdtime_multipler

` default in schema

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

yes, SendLie is an event, explicitly described.

SEND_LIE is a procedure, also explilcitly described.

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

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

because neighbor has been seen and logically this is TwoWay

neighbor is considered gone away only if holdtimer expired or where the
explicit CLEAR procedure is used.

>
>
> ...
> 1713          on NewNeighbor in TwoWay finishes in MultipleNeighborsWait:
> PUSH
> 1714          SendLie event
>
> [diagram] "NewNeighbor [StartMulNeighTimer]"
>

curse of manually maintained ASCII art. PDF SVG is programatically
generated from code now.

removed all actions from ASCII art. it's redundant there anyway and will
lead to nothing but
inconsistencies when manually patched

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

corrected, again, manual crafting of ASCII during spec development

>
>
> ...
> 1736          on LieRcvd in ThreeWay finishes in ThreeWay: PROCESS_LIE
>
> [diagram] "LieReceived [ProcessLIE]"
>

corrected

>
>
> ...
> 1750          on UpdateZTPOffer in OneWay finishes in OneWay: send offer
> to ZTP
> 1751          FSM
>
> [minor] The diagram uses both "SendOfferToZTP" and "SendOfferToZTPFSM".
>

removed all actions

>
>
> ...
> 1758          on TimerTick in OneWay finishes in OneWay: PUSH SendLie event
>
> [minor] From the diagram: "TimerTick [PUSH SendLIE]"; inconsistent name.
>

corrected

>
>
> [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?
>

because there is no holdtimer, there is no neighbor, that's why it's one
way

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

because this timer is only of relevance in multiple neighbors state

timer is started on entry into

MultipleNeighborsWait

and

being checked on every tick until it expires and then

the state is left

>
> The other timers (Holdtime) are not decremented anywhere.
>

yes, I abstracted timer description, I basically say "start timer" and
"check for expired" now everywhere
so there is no decrement

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

not really, expiration is only checked on

<li>on TimerTick in MultipleNeighborsWait finishes in MultipleNeighborsWait:
   check MultipleNeighbors timer, if timer expired PUSH MultipleNeighborsDone
   </li>

>
>
> ...
> 1795          on LieRcvd in TwoWay finishes in TwoWay: PROCESS_LIE
>
> [diagram] "LIERcvd [ProcessLIE]" -- again, neither name is correct.
>

removed action, corrected name.

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

removed actions

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

removed actions

sorry, manual fiddling in ASCII leads to predictable confusion but AFAIR
one of the reviewers (or maybe some authors) felt strongly the ASCII FSM
pictures done manually would improve the document. Here we are.

I'm all for automatically generated FSM pictures (which PDF SVG is now),
e'thing else is the usual
unmaintainable manual copy/paste stuff.  Hence I will cut the FSM ASCII
figures
to backbone.

>
>
> 1809          on HATChanged in OneWay finishes in OneWay: store HAT
>
> [diagram] "HATChanged [StoreHAT]"
>

removed actions

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

good catch, corrected

>
>
> ...
> 1828          on HALChanged in OneWay finishes in OneWay: store new HAL
>
> [diagram] "HALChanged [StoreHAL]"
>

removed actions

>
>
> 1830          on UnacceptableHeader in OneWay finishes in OneWay: no action
>
> [minor] The diagram is missing the action:
> s/UnacceptableHeader/UnacceptableHeader [-]
>

checked, it seems to be there

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

<li>on HoldtimeExpired in TwoWay finishes in OneWay:
    no action
</li>

                  <li>on HoldtimeExpired in OneWay finishes in OneWay:
                      no action
                  </li>

                  <li>on HoldtimeExpired in ThreeWay finishes in OneWay:
                      no action
                  </li>

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

there is good amount of LIEs being pushed to speed up convergence and
tear-down of adjacency.

This has been all carefully debug'ed and inter'oped. I can't remember
reasoning for this particular case.

>
>
> 1851          on HALSChanged in OneWay finishes in OneWay: store HALS
>
> [diagram] "HALSChanged [StoreHALS]"
>

removed actions

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

         <li>PUSH Event: queues an event to be executed by the FSM upon
exit of this action</li>

start of specification explains now in excruciating details how a
sophisticated extended FSM is implemented

*The FSMs, as usual, are presented as states the FSM can assume,   events
that it can be given and according actions performed when   transitioning
between states on event processing.   Actions are performed before the end
state is assumed.   The FSMs can queue events against itself to chain
actions or against   other FSMs in the specification.  Events are always
processed in the   sequence they have been queued.   Consequently, "On
Entry" actions on FSM state are performed every   time and right before the
according state is entered, i.e. after any   transitions from previous
state.   "On Exit" actions are performed every time and immediately when a
 state is exited, i.e. before any transitions towards target state are
 performed.   Any attempt to transition from a state towards another on
reception   of an event where no action is specified MUST be considered an
 unrecoverable error, i.e. the protocol MUST reset all adjacencies,
 discard all the state and MAY NOT start again.   FSMs are strictly
deterministic, i.e. no bi-furcation within actions   depending on which
state the FSM assumed is possible.   The data structures and FSMs described
in this document are   conceptual and do not have to be implemented
precisely as described   here, as long as the implementations support the
described   functionality and exhibit the same externally visible
behavior.   The machines can use conceptually "timers" for different
situations.   Those timers are started through actions and their expiration
leads   to queuing of according events to be processed.   The term
`holdtime` is used often as short-hand for `holddown timer`   and signifies
either the length of the holding down period or the   timer used to expire
after such period.  Such timers are used to   "hold down" state within an
FSM that is cleaned if the machine   triggers a `HoldtimeExpired` event.*

I do not think starting to define what "queues" means will improve the
spec.

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

there are on exit and on entry actions on the FSM

how the states are queued and processed has been described
at beginning of specification section and is expanded now as
pasted above

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

no, lots of action have compound things they do and CLEANUP is just
one procedure

>
>
> 1865       3.  SEND_LIE: create a new LIE packet
>
> [minor] I assume this is create and send, or just create?
>

yes

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

explained in ValidReflection event now

<li>ValidReflection:

    received reflection of this node from neighbor, i.e. `neighbor`
    element in `LiePacket` corresponds to this node.</li>

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

This is explained in LIE processing section already per text below. It
should not be
repeated
 in FSM since it's a big mouthful (except coarse text under PROCESS_LIE
breaking that up
into events generated on FSM)

A node MUST form a ThreeWay adjacency if and only if the following
first order logic conditions are satisfied on a LIE packet as
specified by the `LIEPacket` schema element and received on a link

1.  the neighboring node is running the same major schema version as
    indicated in the `major_version` element in `PacketHeader` *and*

2.  the neighboring node uses a valid System ID (i.e. value different
    from `IllegalSystemID`) in `sender` element in `PacketHeader`
    *and*

3.  the neighboring node uses a different System ID than the node
    itself

4.  the advertised MTUs in `LiePacket` element match on both sides
    *and*

5.  both nodes advertise defined level values in `level` element in
    `PacketHeader` *and*

6.  [

       i) the node is at `leaf_level` value and has no ThreeWay
       adjacencies already to nodes at Highest Adjacency ThreeWay
       (HAT as defined later in Section 4.2.7.1) with level different
       than the adjacent node *or*

       ii) the node is not at `leaf_level` value and the neighboring
       node is at `leaf_level` value *or*

       iii) both nodes are at `leaf_level` values *and* both indicate
       support for Section 4.3.9 *or*

       iv) neither node is at `leaf_level` value and the neighboring
       node is at most one level difference away

    ].

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

"last known neighbor" is plain english and that's why we don't drop from
ThreeWay to TwoWay e.g. on lost reflection.

A neigbhor not reflecting can very well provide an offer for ZTP level.

The `

not_a_ztp_offer`

needs this knowledge.

That's why the LIE FSM  maintains a neighbor structure

per actions and uses the knowledge of "neighbor" (which

does not mean it's in 3-way, that's why the spec makes

precise differentations) for ZTP purposes.

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

Section '

Northbound TIE Flooding Reduction'

explains how this variable is computed.

Added new event 'FloodLeadersChanged' to explain that

new leader election results update the FSM

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

replaced with

<li>if lie has major version not equalt to this node's *or* equal to this
node's system ID or `IllegalSystemID` then CLEANUP
                    else
                </li>

>
>
> [major] s/OR/or/g   Not a normative word.
>
>
> [nit] s/else/, else/g
>

comma? this seems just confusing. this is pseudocode, not a narrative

>
> 1879           2.  if lie has non matching MTUs then CLEANUP, PUSH
> 1880               UpdateZTPOffer, PUSH MTUMismatch else
>
> [nit] s/lie/LIE/g
>

done, good catch

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

nope, those events can be logged and hence operationally differentiated.
This is very valuable in practice.

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

yes, roughly.

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

yes, replaced 'my' and 'own' with 'this node's'

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

done

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

nope, it's correct like this, this corresponds to the first level logic in
according verbose section

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

you may end up with a LIE that transitions directly from OneWay to ThreeWay

actions can end up queuing a whole set of events which are processed then
and
result in multiple transitions

>
>
> [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
>
>
the and means after all those are processed finally do the 3-way check.

I moved it out the list to clarify

> [nit] s/linkid/LinkID

done

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

moved out the list for clarify

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

incorrect. OneWay may very well pick up a valid reflection from the event
queue
after the other events have been processed before it and it sees a valid
reflection.
Please read the new specification explanation how the extended FSM works.
this kind of processing allows for very fast progression of states compared
to
"simple" FSM where an event cannot generate events, i.e. you do something
on
data structures
and then stop processing. In worst case instead of generating 5 events and
progress
a normal FSM waits for 5 packets to process all that or ends up with
extremely
complicated action code that basically does implicit processing in a single
huge
if/then/else construct per what rift generates as separate events.

generally, the LIE spec has been implemented 2 or 3 times independently
(and good
amount of problems found early and corrected) and
validated (at least for J here) @ very large scale under heavy randomized
testing
and lots of different environments. It's not impossible but very unlikely
you'll find errors by "processing in your head" by now
unless you implement it and imply some sophisticated test methodology.

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

I don't see where the spec uses "does not reflect"

Pretty close to -13 now. I asked Pascal for last full read before we push
it out.

---

From: Alvaro Retana <[email-masked]>
Date: Tue, 22 Jun 2021 12:29:28 -0700

On June 13, 2021 at 11:31:39 AM, Tony Przygienda wrote:

Tony:

Hi!

I know that you asked for reviews of the proposed -13 from the WG.  I
haven't seen any replies -- but I have a couple of comments in the
meantime.  I'll wait for -13 to be committed before looking at the
details.

Thanks!

Alvaro.

...
> because there is no holdtimer, there is no neighbor, that's why it's one way

Ok, but there's still a HoldTimerExpired event in the FSM figure (in -13).

...
...
> This is explained in LIE processing section already per text below. It s
> hould not be repeated in FSM since it's a big mouthful (except coarse text
> under PROCESS_LIE breaking that up into events generated on FSM)

My confusion is that nowhere in the text you quoted is it mentioned
that those are the conditions that determine if the neighbor is
"valid".  Maybe saying something like "reflecting the neighbor if the
conditions for a ThreeWay adjacency are met".

...

...
> > 1882 3. if PoD rules do not allow adjacency forming then CLEANUP,
> This is very valuable in practice

Maybe an operational consideration...there's just one case (that I've
found so far) where logging (or using telemetry, or whatever) is
mentioned.  It would be nice to include that expectation somewhere.

...

---

From: Tony Przygienda <[email-masked]>
Date: Tue, 22 Jun 2021 21:44:55 +0200

Hey, inline

> meantime.  I'll wait for -13 to be committed before looking at the

yepp.

STill on my list to poke the thrift folks for discussion on stable
reference BTW.

> Ok, but there's still a HoldTimerExpired event in the FSM figure (in -13).

yes, the fsm may queue multiple events on complex progressions as described
and consequently we could end up with HoldTimerExpired in OneWay during
processing of the queue. Whether that will happen given current events I
don't know. It may be the do nothing catch all for all those events in
OneWay. During heavy randomized testing I found couple of such cases I
thought "would never happen" and the resulting FSM is ultimately a mix of
all the stuff we found in both implementations putting  things through
paces in our disjoint test frameworks.

> conditions for a ThreeWay adjacency are met".

well, neighbor being "valid" is another word to say "you can accept it and
build a ThreeWay". I'll add a sentence as you suggest if that's still
confusing.

> mentioned.  It would be nice to include that expectation somewhere.

yeah, ok, fair 'nuff. I'll add a sentence along the lines of "those events
can be logged to simplify operation of the protocol".