Re: [Rift] Opsdir early review of draft-ietf-rift-rift-08

[Tony Przygienda <[e-mail]>]

Nagendra, thanks so much for taking the time to go through the doc

inline

On Thu, Oct 31, 2019 at 7:09 PM Nagendra Kumar via Datatracker <
[e-mail]> wrote:

> Reviewer: Nagendra Kumar
> Review result: Has Issues
>
> Hi,
>
> I have reviewed this document as part of the Operational directorate's
> ongoing
> effort to review all IETF documents being processed by the IESG.  These
> comments were written with the intent of improving the operational aspects
> of
> the IETF drafts per guidelines in RFC5706.
>
> Comments that are not addressed in last call may be included
> in AD reviews during the IESG review.  Document editors and WG chairs
> should
> treat these comments just like any other last call comments.
>
> Overall Summary:
>
> This draft is a standard track proposing a very new protocol and not an
> extension of any existing protocol. As such, there are no backward
> compatibility issues expected. Overall this is a well written document.
> Some
> ambiguity that needs attention are listed below. I chose "Has Issues"
> primarily
> to address the issue that may cause behavior inconsistency or traffic blakc
> holes. More details below:
>

yes, multiple thorough reviews came in, I'm incorporating comments and it
will need a document spin.


>
> --> LIE with TTL more than 1 is clearly defined to be discarded. I couldn't
> find a similar clarification when TIE is received with TTL more than 1?.
> If i
> didn't miss it, I think it is better to be clarified for consistent
> behavior.
>

version -09 already says

<t>The TIE exchange
    mechanism uses
    the port indicated by each node in the LIE
    exchange and the interface on which the adjacency has been
    formed as destination. It SHOULD use TTL of 1 as well and
    set inter-network control precedence on according packets.
    </t>



>
>
> 5.3.7.  Label Binding
>
>    A node MAY advertise on its TIEs a locally significant, downstream
>    assigned label for the according interface.  One use of such label is
>    a hop-by-hop encapsulation allowing to easily distinguish forwarding
>    planes served by a multiplicity of RIFT instances.
>
> --> Read this more than a couple of times and not sure if "for the
> accordingly
> interface" means assigning a locally unique label for each
> interface/adjacency
> or just one label for the node and advertise the same label over all
> adjacency.
>

aha, yes, not clear. good catch. per interface (downstream unsolicited).
will add. additionally, it says on its TIEs but it's the LIE that has
interface specific label.

labels are optional in the schema so we can easily add them later in all
kind of variations as well as extend node and link capabilities with
according indications.


>
> --> Since it is mentioned that the use of this label is for HbH
> encapsulation,
> a node at Level n should install (or ignore) the label from node at Level
> (n-1)
> in its Incoming Label Mapping (ILM) table?
>


>
> --> On the same line, a node at Level n should install (or ignore) the
> label
> from node at Level (n-2)?.
>
> --> I think some clarity is better to avoid traffic black holing due to
> inconsistent behavior.
>

well, spec says only

"

One use of such label is
a hop-by-hop encapsulation allowing to easily distinguish forwarding
planes served by a multiplicity of RIFT instances."

since there were discussions how to multi-instantiate RIFT with dataplane
(since control plane is so easy. to run multiple times, even over same
links).  If you suggest a good wording and reference here we can add it to
make the example ore specific. Observe that the use of the label is
non-normative. I would expect people to write drafts adding e.g. adjacency
capability something saying "binding label to ILM" and if the other side
doesn't support that not advertise the label (kind of like BGP
capabilities). Or they add their own optional schema element saying
"ILM-label" or something. Protocol will allow that fine as long it's
optional and has no default. We just bump the schema minor version up and
allocate new codepoint (i.e. thrift field number).


>
> 5.3.8.1.  Global Segment Identifiers Assignment
>
>    Global segment identifiers are normally assumed to be provided by
>    some kind of a centralized "controller" instance and distributed to
>    other entities.  This can be performed in RIFT by attaching a
>    controller to the Top-of-Fabric nodes at the top of the fabric where
>    the whole topology is always visible, assign such identifiers and
>    then distribute those via the KV mechanism towards all nodes so they
>    can perform things like probing the fabric for failures using a stack
>    of segments.
>
> --> According to RFC 8402, Global Segment is a generic term that refers to
> a
> segment from SR Global Block with instruction or forwarding semantic
> defined at
> the domain level. But it does not have a default instruction associated
> with
> it. I think the above should be more specific as Prefix SID or something
> related to RIFT.
>

we removed the detailed SR support into its own draft and now you mentioned
it I think that belongs there with all the other considerations, otherwise
we'll drag the whole SR right back into RIFT draft which runs counter most
comments that what we have is too long already. So, I'm removing the whole
SR section from the main spec and punting the text to the authros of
https://tools.ietf.org/html/draft-zzhang-rift-sr-01

The SR draft seems expired couple days ago, thought it was adopted
actually, not sure what the status is, up to the chairs.


>
> Few observations below:
>
> Clos/Fat Tree:  This document uses the terms Clos and Fat Tree
>       interchangeably whereas it always refers to a folded spine-and-
>       leaf topology with possibly multiple PoDs and one or multiple ToF
>
> --> ToF and PoD were not expanded in the first occurrence. ToF was
> explained
> later.
>

expanded acronym but the sequence is a circular dependency. Can't introduce
ToF until I have fabric but fabric hard to describe without mentioning
planes/tof/pods as well since otherwise it's not clear how those terms fold
in.


>
> --> While Southbound representation is explained in the terminology
> section,
> northbound representation is not. I think it is better to define the same
> for
> consistency.
>

done


>
> Multiple
>       adjacencies can be formed to a neighbor via parallel interfaces
>       but such adjacencies are NOT sharing a neighbor structure.  Saying
>       "neighbor" is thus equivalent to saying "a three way adjacency".
>
>
yes, it's very different from OSPF here.


> --> The above says "Multiple adjacencies can be formed to a neighbor.."
> where
> the "neighbor" here is implicitly defining the remote node and then the
> sentence further continues stating "Saying "neighbor" is thus equivalent to
> saying "a three way adjacency"".  May be the first "neighbor" can be
> changed to
> "remote node"?
>

done, very good improvement in language.


>
> mobility defined in Paragraph 17
>
> --> I assume that you are referring to REQ#17?. I think it is better to
> refer
> it as REQ#17. But I will leave that to the authors to decide.
>
>  The solution should allow for access to link states of the
>             whole topology to enable efficient support for modern
>             control architectures like SPRING [RFC7855] or PCE
>             [RFC4655].
>
> --> RFC 7855 is an information RFC that outlines the problem statement and
> use
> case. I think you should refer RFC 8402 here for the SR architecture.
>

Section for SR removed into the SR with rift draft.

requirements section removed completely based on other reviewers' comments.


>
> "Future
>    versions of this document may describe support for such tunneling in
>    RIFT."
>
> --> Section 5.3.3.4 talks about Overlay encapsulation and leave it a bit
> ambiguous with the above statement. I think, it should be removed.
>

Yes, Pascal's idea that we moved into the future. Sentence removed.


>
> already to HAT nodes with level different than the adjacent
>
> --> HAT was first expanded in page 66 while the abbreviation is used
> in/from
> Page 34.
>

best I could do was to clarify

8.  [

       i) the node is at level 0 and has no three way adjacencies
       already to Highest Adjacency Three-Way (HAT) nodes (defined in
       Section 4.2.7.1) with level different than the adjacent node OR


since we moved the whole ZTP detailed terminology from main terminology
section given it was overwhelming down into the ZTP section.


> In Section 5.2.3.2, I hope the first node is "ToF21 S-TIEs" instead of
> "Spine21
> S-TIEs"?. Similar references are throughout the example with Spine21 and
> Spine22.
>

yes, came up in another review, all Spine 21/22 corrected to ToF 21/22


>
> "scope of this document and may be covered in a separate document
>    about policy guided prefixes [PGP reference]."
>
> --> There is no PGP reference. This may need to be fixed or removed.
>

yes, it's a separate draft. To progress the document to standard we
actually have to remove all the references to drafts so I remove the
reference here.

thanks

--- tony