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

[Tony Przygienda <[e-mail]>]

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

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.

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.


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



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

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