Early Review of draft-ietf-rift-rift-08
review-ietf-rift-rift-08-opsdir-early-kumar-2019-10-31-00

Request Review of draft-ietf-rift-rift-08
Requested rev. 08 (document currently at 10)
Type Early Review
Team Ops Directorate (opsdir)
Deadline 2019-10-31
Requested 2019-09-12
Requested by Jeff Tantsura
Authors Tony Przygienda, Alankar Sharma, Pascal Thubert, Bruno Rijsman, Dmitry Afanasiev
Draft last updated 2019-10-31
Completed reviews Secdir Early review of -04 by Scott Kelly (diff)
Rtgdir Early review of -06 by Russ White (diff)
Genart Early review of -08 by Robert Sparks (diff)
Secdir Early review of -08 by Scott Kelly (diff)
Opsdir Early review of -08 by Nagendra Kumar (diff)
Rtgdir Early review of -08 by Jonathan Hardwick (diff)
Comments
Dear colleagues,

In preparation for the WGLC, RIFT chairs would like to ask you to review the document.

Thanks,
Jeff and Jeffrey
Assignment Reviewer Nagendra Kumar
State Completed
Review review-ietf-rift-rift-08-opsdir-early-kumar-2019-10-31
Posted at https://mailarchive.ietf.org/arch/msg/ops-dir/3HY_Cv3km_t8OKCQfCVrFYUBNKk
Reviewed rev. 08 (document currently at 10)
Review result Has Issues
Review completed: 2019-10-31

Review
review-ietf-rift-rift-08-opsdir-early-kumar-2019-10-31

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:

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

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.

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

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. 


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.

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

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

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

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.

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

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.

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.

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