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

[Jordan Head <[e-mail]>]

Thanks Alvaro,

You'll have my replies for parts 1 and 2 this week, then I'll start reviewing this part.

Jordan

On 8/11/22, 8:00 AM, "RIFT on behalf of Alvaro Retana" <[e-mail] on behalf of [e-mail]> wrote:

    [External Email. Be cautious of content]


    Hi!

    [Continuing with Part 3.]

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


    I have a couple of replies from the thread and then some comments on
    the related text in -15.

    Thanks!

    Alvaro.



    ...
    > > 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
    >
    > ValidReflection:
    >
    > received reflection of this node from neighbor, i.e. `neighbor`
    > element in `LiePacket` corresponds to this node.

    Yes, however, the definition is based on what is received, not what is
    sent.  Can we at least add ValidReflection to the sentence:

       1.reflecting the neighbor (ValidReflection)...


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

    Yes, please do.



    *****
    What follows are comments on -15 covering the same sections.
    *****

    [Line numbers from idnits.]

    ...
    1650    4.2.2.1.  LIE Finite State Machine

    1652       This section specifies the precise, normative LIE FSM.  For easier
    1653       reference the according figure is given as well in Figure 14.
    1654       Additionally, some sets of actions repeat often and are hence
    1655       summarized into well-known procedures.

    [] "according figure"

    There are over 40 uses of "according" -- mostly as above.  English is
    not my first language so I thought I was learning something new.  I
    went to look it up and I can't find the word as used here.  I think
    you may mean "corresponding figure".

    https://urldefense.com/v3/__https://www.merriam-webster.com/dictionary/according__;!!NEt6yMaO-gk!E9NEVXsiFnv6Ey2PDM-YwFjKBRil8flZEKlZArFnpd3xcI3Lw_thMZTs42h2EsQmepiaTTd4e_CpSt_WWalK$
    https://urldefense.com/v3/__https://wikidiff.com/corresponding/according__;!!NEt6yMaO-gk!E9NEVXsiFnv6Ey2PDM-YwFjKBRil8flZEKlZArFnpd3xcI3Lw_thMZTs42h2EsQmepiaTTd4e_CpSrhvnQkS$



    ...
    1793       The following words are used for well known procedures:
    ...
    1798       *  CLEANUP: neighbor MUST be reset to unknown

    [major] The "unknown" state is not defined anywhere.  I imagine that
    you mean that on CLEANUP we go back to the OneWay state (back to
    "Enter").



    1800       *  SEND_LIE: create and send a new LIE packet
    ...
    1804          2.  setting the necessary `not_a_ztp_offer` variable if level was
    1805              derived from last known neighbor on this interface and

    [nit] s/from last known neighbor/from the last known neighbor



    1807          3.  setting `you_are_not_flood_repeater` to computed value

    [major] s/you_are_not_flood_repeater/you_are_flood_repeater



    1809       *  PROCESS_LIE:

    1811          1.  if LIE has major version not equal to this node's *or* system
    1812              ID equal to this node's system ID or `IllegalSystemID` then
    1813              CLEANUP else

    [nit] s/if LIE has major version/if the LIE has a major version

    Most of the steps in this procedure include references to the LIE
    being processed, which is a little redundant given that we're in
    PROCESS_LIE.  You might want to  simplify a little; for example:

       1. the major version is not equal to this node's...


    [minor] The first "or" is highlighted ("*or*"), but not the second.
    Is there a reason?



    1815          2.  if LIE has non matching MTUs then CLEANUP, PUSH
    1816              UpdateZTPOffer, PUSH MTUMismatch else

    [nit] /LIE/the LIE/g



    1818          3.  if LIE has undefined level OR this node's level is undefined
    1819              OR this node is a leaf and remote level is lower than HAT OR
    1820              (LIE's level is not leaf AND its difference is more than one
    1821              from this node's level) then CLEANUP, PUSH UpdateZTPOffer,
    1822              PUSH UnacceptableHeader else

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

    [major] s/AND/and   Not a normative word.



    ...
    1852       States:

    1854       *  OneWay: initial state FSM is starting from.  In this state the
    1855          neighbors did not see any valid LIEs from a neighbor after the
    1856          state was entered.

    [nit] s/state FSM/state the FSM


    [] In many places the document treats nodes as people (or with
    people-like abilities).  For example: "neighbors did not see any valid
    LIEs".  Strictly, a node cannot see, but it can receive a LIE.

    Please consider rewording the text.  While I think the current form is
    not correct, I won't insist on it -- but others may.


    [major] "neighbors did not see any valid LIEs from a neighbor"

    The text is not precise.  You're referring to what the local node has
    received (or not) -- using "neighbors did not see...from a neighbor"
    gives both the impression that the state is controlled by others, or
    that the local node is not involved (because the definition is based
    on its neighbors).

    I was going to make a suggestion, but the description is not clear to
    me.  Reading from the FSM...  In OneWay, the node hasn't seen a new
    neighbor (NewNeighbor) but it may have sent a LIE already (not
    necessary), right?

    Suggestion>
       OneWay: Initial state.  In this state the node has not received
       a valid LIE from another node on the interface.


    [major] BTW, what is a "valid LIE"?

    §4.2.7.1 (Terminology) says this: "valid LIE (i.e. passing all checks
    for adjacency formation while disregarding all clauses involving level
    values)".  But at this point (in OneWay), the adjacency hasn't been
    formed.  What are the checks that need to be made?  Please mention
    them explicitly, or point to where they are listed.

    The end of the last section has a list, but it doesn't mention "valid"
    (or any related words) -- is that it?  That list matches the checks in
    the PROCESS_LIE procedure.

    [There are 6 instances of "valid LIE" in the document.]



    ...
    1861       *  ThreeWay: this state signifies that lies from a neighbor are seen
    1862          with correct reflection.  On achieving this state the link can be
    1863          advertised in `neighbors` element in `NodeTIEElement`.

    [nit] s/lies/LIEs


    [major] This is the only place where "correct reflection" is used.  To
    keep in line with the TwoWay definition, use "its relfection" instead,
    or use something to the effect of "a valid reflection
    (ValidReflection)".



    1865       *  MultipleNeighborsWait: occurs normally when more than two nodes
    1866          see each other on the same link or a remote node is quickly
    1867          reconfigured or rebooted without regressing to `OneWay` first.
    1868          Each occurrence of the event SHOULD generate a clear, according
    1869          notification to help operational deployments.

    [minor] Maybe I don't remember this from the overview -- where is it
    specified that only point-to-point links (or links with only two
    nodes) are supported?


    [major] "SHOULD generate a clear, according notification"

    "clear" can be subjective, so it is not a good idea to use that next
    to Normative language.

    s/.../SHOULD generate a notification



    1871       Events:

    [major] InstanceNameMismatch is not defined.



    1873       *  TimerTick: one second timer tic, i.e. the event is generated for
    1874          FSM by some external entity once a second.  To be quietly ignored
    1875          if transition does not exist.

    [minor] I don't feel comfortable with "some external entity" because
    it gives the impression that there are uncontrolled, maybe even
    unsecure, dependencies.

    Suggestion>
       TimerTick: one second timer tic; this event is generated
       once a second in an implementation-specific way which is
       outside the scope of this specification.

    Or you could simply just say "one second timer tic".

    BTW, should that be "tick" (instead of "tic")?

    Note that there's one more reference to "some external entity" when
    talking about ShortTic.


    [?] "To be quietly ignored if transition does not exist."

    What is ignored, the TimerTick?  How can it be ignored if it doesn't
    exist?  Or maybe I'm missing what the transition is.



    ...
    1915       *  HoldtimeExpired: adjacency holddown timer expired.

    [major] The ASCII figure includes 2 HoldtimerExpired


    [EoR P3-15]

    _______________________________________________
    RIFT mailing list
    [e-mail]
    https://urldefense.com/v3/__https://www.ietf.org/mailman/listinfo/rift__;!!NEt6yMaO-gk!E9NEVXsiFnv6Ey2PDM-YwFjKBRil8flZEKlZArFnpd3xcI3Lw_thMZTs42h2EsQmepiaTTd4e_CpSioWHXeY$


Juniper Business Use Only