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
<[email protected] <mailto:[email protected]>> 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
_______________________________________________
Gen-art mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/gen-art
