Re: [Gen-art] [Rift] Genart early review of draft-ietf-rift-rift-08

[Robert Sparks <[e-mail]>]

On 10/28/19 3:51 PM, Tony Przygienda wrote:
> Robert, reacting as editor here
>
> thanks for your detailed review, answers inline
>
> I'm raising couple questions myself in the answers, if you'd care to 
> clarify those further I can try to make the
> draft cut-off for an updated version
>
> On Mon, Oct 28, 2019 at 12:00 PM Robert Sparks via Datatracker 
> <[e-mail] <mailto:[e-mail]>> wrote:
>
>     Reviewer: Robert Sparks
>     Review result: Not Ready
>
>     Summary: Not ready for publication as a Proposed Standard
>
>     I found this document very difficult to read. On my first past
>     through, I was
>     fairly certain it didn't specify the protocol well enough to
>     actually implement
>     it, until I got deep into the appendices. Even after a second
>     pass, I am not
>     comfortable that a implementation could be produced without having
>     guess at
>     some behavior, which will lead to interoperability problems.
>
>     I strongly suggest restructuring the draft to pull the actual
>     definition of the
>     protocol from the appendix and into the early part of the document
>     text. You
>     may also want to use the new v3 format so that you can take
>     advantage of its
>     support for SVG to help convey your state diagrams.
>
>
> As first observation, we have two interoperable implementations since 
> a bit, one completely open source which has been produced based on the 
> spec. It was in fact open source work that helped to refine the 
> document content to make sure we can have an implementation produced 
> based on the text without further "guessing things". As example the 
> LIE FSM has been implemented initially in open source without 
> consulting authors of the spec and interoperat'ed without a single 
> defect (but discovered a protocol underspecification in case of 
> misconfiguration that was subsequently added). Please refer to IETF 
> proceedings for the according presentations if necessary. We have a 
> third implemenation progressing now where all questions the 
> implementor asked so far could be answered by pointing directly @  the 
> specification as written. This seems to answer to me the "suspicion of 
> specification maybe not being good enough to implement" as an 
> objective measuring stick as far I can imagine one.
>
> As second observation regarding your comments, the majority of readers 
> seemed to prefer a format where the protocol is "described" in 
> narrative and the tight procedure appendices are left to people 
> implementing the protocol but I have no issue to pull the appendices 
> directly into the document under the according sections. I would 
> simply add something along the lines of "this section of interest to a 
> reader implementing the spec".
>
> I will try the v3 format again but last time we tried couple months 
> ago it wasn't workable. I see first RFC havs been just published using 
> it (albeit I don't see it using any drawings).
>
>
>     At the very least, the document needs a strong editorial scrub.
>     There are many
>     sentences that are missing important articles. There are many
>     sentences that
>     are very complex and would be better written as two or three
>     sentences. There
>     are many sections that too deeply assume the reader already
>     understands the
>     problem space.
>
>
> Point taken and a scrub will be performed along the lines you suggest.
>
>
>     In the remainder of the review, I'm not going to call out each
>     overly-complex
>     sentence individually, but will attempt to point at a few that
>     also had other
>     issues.
>
>
> yes, thank you. that should suffice for a scrub.
>
>
>     Some high-level points before going into a document-order set of
>     comments:
>
>     ** The IANA considerations section does not provide a clear set of
>     instructions
>     for IANA to follow.
>
>
> Ok, could you be a little bit more specific

You can get AD help here. See section 2.2 of 
https://datatracker.ietf.org/doc/rfc8126/.

9.2 is where I am concerned.

Please add an explicit statement that you are asking IANA to create a 
new top-level registry named RIFT with the following subregistries under it.

>
> 9.1.  Requested Multicast and Port Numbers  seems to describe the allocation points and registries.
> 9.2.  Requested Registries with Suggested Values requests new registries with pre-allocated entries, maybe it's not clear that each subsection is a new registry? I can point that out.
>
>
>     * The notion of a "three-way adjacency" is not made clear in the
>     document. It
>     really takes looking at a rendering of the dot for the state
>     machines before
>     this term becomes well-defined.
>
>
> good catch, will be pulled up into "terminology" section and defined.
>
>
>     * There are a few places where there are internal references in
>     the document
>     that are not clear. These center around the word "Paragraph". For
>     instance, in
>     Req4, there is a reference to "Paragraph 17". Does that mean "Req
>     17"?
>
>
> anomaly of rfc2txt, when referencing a list no matter what the prefix 
> it will always generate
> "paragraph" in the reference. I will see how I can fix that.
>
>
>     * K_LEAF suddently appears without support or definition in 5.1.2.
>     That and
>     the other K_ terms should be discussed in the terminology section.
>
>
> the section 5.1.2. was written more as a narrative but no problem, I 
> will add a special terminology section and define the variables.
>
>
>     * It's unclear whether HH symbol means one or two RJ45 jacks. The
>     diagrams
>     appear to use the symbol inconsistently.
>
>
> The document says verbatim /"we have chosen to use the "HH" symbol as 
> ASCII visualisation of a RJ45 jack." /I will add "single" to it.
>
> Figure 9 may cause confusing since a single H is used to visualize the 
> jack.  I also see possible inconsistency in Figure 13, will correct, 
> probably replacing the "HH" with "X"
>
>
>
>     === Issues in document order ===
>
>     In the introduction:
>
>     "The incumbent protocols precondition normally extensive
>     configuration or
>     provisioning during bring up and re-dimensioning which is only
>     viable for a set
>     of organizations with according networking operations skills and
>     budgets." did
>     not parse for me until my second read-through, where I realized
>     "precondition
>     normally" was intended to mean "normally require". "bring up" and
>     "re-dimensioning" are almost jargon in this context.
>
>
> will try to simplify language where it looks possible.
>
>
>     In the Terminology section:
>
>     It would be good to introduce S-TIE and N-TIE as top level terms
>     in this
>     section.
>
>
> terminology section already says:
>
> TIE:  This is an acronym for a "Topology Information Element".  TIEs
>     are exchanged between RIFT nodes to describe parts of a network
>     such as links and address prefixes, in a fashion similar to ISIS
>     LSPs or OSPF LSAs.*We will talk about N-TIEs when talking about TIEs in the northbound 
> representation and S-TIEs for the southbound equivalent.*
> but sure, I will break that out and define more explicitly
>
>
>     The words for Adjacency are not really a definition, but taken as
>     such they are
>     circular.
>
>
> correct, we failed to root the defintions of interface/link/adjacency 
> since in p2p link-state
> protocols they are pretty much equivalent (modulo tunnels). 
> suggestions?  I'll introduce
> 3-way as clear defintion and define adjacency as "3-way state on a link"
>
>
>     The definition for Metric is not useful.
>
>
> ok, will be removed
>
>
>     In the first paragraph of 5.1.1, The construction of 'never ...
>     (we'll talk
>     about exceptions later)' is awkward. It's also unclear where the
>     the "later" is
>     pointing. The last sentence is grammatically incorrect ("We omit
>     ... out"), and
>     talks of omimitting discussion "for the moment". Please make it
>     clear where you
>     return to the discussion.
>
>
> ok, I'll list the exceptions in detail here
>
>
>     The third paragraph of 5.1.2 does not parse. I suspect there is a
>     missing word
>     somewhere in "the introduced Section 3.1".
>
>
> word missing
>
>
>     The nested numbered lists in section 5.2.3.9 are perhaps not the
>     best tool for
>     describing the algorithms you want to convey. On page 46, steps
>     4.3.3 and 4.4
>     are comments, not actions. Numbering them as you number actions is
>     confusing.
>
>
> any better suggestions? (I will move from numbering to 
> number.latin.other and so on). This is
> an old technique in protocol specs to allow implementors to discuss 
> very, very precise statement
> and their meaning (e.g. ISIS spec is all written like that). Otherwise 
> people start to talk about
> "4th line in the algorithm X" which is far more confusing.
number.latin.other will help.
>
> rfc2txt does not have a mechanism to "skip numbering for this item" 
> when genreeating
> lists unless I'm oblivious to it so I'm limited by the tools IETF 
> provides to render documents.
Do you mean xml2rfc when you say rfc2txt?
>
>
>     The last Step 7 in section 5.2.3.9 speaks of "sine flooding
>     reduction". What is
>     that?
>
>
> some editorial slip on find/replace I assume, was "such" AFAIR, seems 
> slipped -04 or -05. missed by
> good amount of readers ;-)
>
>
>     The reference in 5.2.7.2's first paragraph to [EUI64] looks like a
>     normative
>     reference to me.
>
>
> I wasn't sure since the EUI64 is kind of a "side implementation note 
> in IEEE". Can move to normative.
>
>
>     Something is wrong at the bottom of page 68. One branch of the
>     sentence reduces
>     to "we have to decide whether node Y is ... at the same level as Y".
>
>
> correct, sentence is illogical. I will correct
>
>
>     In 5.3.4.1 rule 1, you only consider nodes to which the reciever has a
>     bi-directional adjacency. As opposed to what? Does this mean
>     adjacencies in
>     TwoWay or ThreeWay state? Or something else?
>
>
> I will define "bi-directional adjacency", it cannot hurt.
> But the term is kind of well understood by anyone who
> implemented OSPF/ISIS or is intimately familiar with the specs normally.
>
>
>     There is a dangling reference to [PGP reference] at the end of 5.3.11.
>
>
> slipped. PGP has been folded out the main document and this got 
> missed. I probably remove the reference
> since we cannot publish standard with reference to a draft while PGP 
> work is going slowly.
>
>
>     Section 7.3 is really not useful for the protocol specification.
>     Consider
>     removing it.
>
>
> This question/observation/doubt was coming up many, many times and 
> that's why the document
> introduced this section. I prefer to retain it to prevent the question 
> being asked over and over again
> on the reading of the spec.
>
>
>     In section 8.4 the sentence "The attack vector packet number
>     present is
>     relatively benign." does not parse.
>
>
> yes
>
>
>     It's particularly unclear what you are trying to achive with the
>     "DirectionMaxValue" registry entry defined in 9.2.11.1 Are you
>     trying to say
>     the registry is not allowed any more values? If so, just say that
>     in the
>     instructions to IANA. I don't see where the codepoint is used by
>     the protocol,
>     so I suggest it not be added to the registry.
>
>
> implementation specific basically. Can be removed but allows in 
> implementation to
> use the codepoint to scale arrays for example.
>
> If the registry takes
> DirectionMaxValue
>
> for e.g. version 3.0 schema this value could move. Alternately we 
> could split a registry _per schema version_ but that seems a huge 
> proliferation and replication.
> I'm agnostic either way and would like to hear an opinion here
I'll punt this to the group/AD.
>
>
>     It's a leap to get from the description of Sequence Number Binary
>     Arithmetic in
>     Appendix A to the "rollover" mechanic the paragraphs that
>     reference Appendix A
>     are looking for. Please clarify.
>
>
> ok, will explain rollover in relation to Appendix albeit it's a 
> well-known technique
> since many years in link-state protocols (explained e.g. in depth in 
> John Moy's book) though
> lacking a rigorous implementation description such as Appendix A 
> provides.
>
>
>     Also in Appendix A, I question the sentence "The >: relationship
>     is symmetric
>     but not transitive". Symmetric says "if A>:B then B>:A".
>
>
> yes, symmetric means "if A>:B then B>:A" precisely what you say and the
> relation holds.
>
> non transitive means obviously that
>
> A>:B and B>:C does NOT imply A>:C
>
> Obvious example of the 3-bit arithmetic is
>
> 4 >: 2 & 2 >: 0 does NOT follow in 4 >: 0
I will reread. 4 >: 2 and 2 >: 4 surprises me.
>
> That seems quite clear to me so I don't see easily what you question 
> unless you
> care to explain more.
>
>
>     === Nits ===
>
>     The document has many lines that exceed the maximum line width for txt
>     formatted RFCs.
>
>
> ack, will correct
>
>
>     The definition section uses terms before it defines them. For
>     those that are
>     acronyms (see the first use of PoD and ZTP for example) an
>     expansion would
>     help. The short-hand "E-W" isn't saving much over "East-West" and
>     the latter
>     would be much clearer in the terminology section.
>
>
> will correct
>
>
>     In section 3.2 at " We will use topology in Figure 2 (called
>     commonly a fat
>     tree/network in modern IP fabric considerations [VAHDAT08] as
>     homonym to the
>     original definition of the term [FATTREE]) in all further
>     considerations.".
>     This would better be conveyed as two seperate sentences. I don't think
>     "homonym" (same spelling or pronunciation, but different meanings)
>     is really
>     the word you are looking for.
>
>
> "homonym" is precisely what is meant hear, the term has been 
> overloaded to
> mean two different things over time.
>
> Will split into shorter sentences.
>
>
>     On page 17, you have a block labelled "Following list represents
>     non-requirements", followed by a single PEND requirement, and then
>     "Finally,
>     following are the non-requirements" before the NONREQs. The
>     description of the
>     first block is incorrect?
>
>
> yes, sorry, slipped. we had pending? requirements which slid into non 
> requirements or has
> been removed. this one is a left over.
>
>
>     The last sentence in the paragraph immediately following Figure 10
>     has no verb.
>
>
> correct, thanks
>
>
>     The second paragraph after Figure 10 includes a sentence that
>     begins "If proves
>     convenient". I suspect it meant to be "It proves convenient"
>
>
> correct
>
>     In section 5.2.2 the phrase "if a link has entered three way IPv4
>     and/or IPv6
>     with a neighbor on an adjacency" should be reconstructed. I
>     _think_ you mean
>     has an adjacency with a neighbor that is in the three-way state"?
>
>
>
> yes, should clarify via crisp definition of 3-way.
>
>
>     In section 5.2.3.5, the pronouns in "It should only set it" can become
>     confused.Please rework the sentence to avoid the pronouns.
>
>
> ack
>
>
>     There is a stray double-quote at the end of the first sentence in
>     5.2.7.2
>
>
> ack
>
>
>     At 5.2.8 "(with some finer distinctions not explained further)"
>     adds no useful
>     information to the document. Please remove it.
>
>
> ack
>
>
>     At section 6.1, please explicitly point to figure 2 when
>     mentioning the example
>     topology.
>
>
> ack
>
>
>     The term "ToRs" appears without definition or reference at section 7.1
>
>
> ack