Subject: Opsdir last call review of draft-ietf-mpls-spring-entropy-label-11

From: Joe Clarke <[email-masked]>
Date: Mon, 25 Jun 2018 07:38:47 -0700

Reviewer: Joe Clarke
Review result: Has Nits

I have been asked to review this document on behalf of the ops directorate.  I
know my review is a few days late, but I hope it will still be of some use.

This document describes how to use/place entropy labels in a SPRING path over
an MPLS dataplane for purposes of optimizing load balancing.  Overall, I think
the document is ready, but I did have a couple of operational
questions/comments and found a number of nits.

I found a couple of MAY requirements in here that I feel should be stronger or
offer some additional recommendation (mainly to favor operators or help guide
vendors).  First, section 4 states "For simplicity, an implementation MAY use
the minimum ERLD between each linecard as the ERLD value for the system."  I
feel there should be a stronger recommendation on what to do here given that
ERLD is later described as a key piece of the algorithm for EL placement.  A
vendor may opt for the simple approach, but should they consider a more robust
approach to favor operators?

Second, section 5 states "this node MAY advertise its MSD value or a subset of
its MSD value to the controller."  It MAY NOT do that, too.  It would be good
to highlight pros and cons to this.

Finally, section 7.2 states "In case of a trade-off, an implementation MAY
provide flexibility to the operator to select the criteria to be considered
when placing EL/ELIs..."  I can see this being of great value to operators to
have vendors allow this.  But as with any MAY, the vendor MAY choose NOT to do
it.  SHOULD seems better here for that reason.

On to the nits:

Section 1

Expand LSP before using

===

Section 1

s/local node who/local node which/

===

Section 1

s/disordering/misordering/

I think the latter makes more sense, yes?

===

Section 2

Add LSP to your list of terminology

===

Section 6

s/destinated/destined/

===

Section 6

s/accomodate/accommodate/

===

Section 7.1.2

s/node has not a sufficient/node does not have a sufficient/

===

Section 7.1.2

<Adj_P1P2, Adj_set_P2P3, Adj_P3P4,
   Adj_P4P5, Adj_P5P6, Adj_set_P6P7, Adj_P7P8; Adj_set_P8PE2,VPN_label>

Note the ';' after Adj_P7P8.  This is a comma everywhere else.  I think it
should be a comma here, too.

===

Section 7.1.2

s/PE1 is limited to push a maximum of 11 labels,/PE1 is limited to push a
maximum of 11 labels./

Note the change of a comma to a period.

===

Section 7.2 and throughout

You use EL/ELI and ELI/EL interchangeably.  For consistency, pick one and go
with it.

===

Section 7.2

s/considered as exhaustive/considered exhaustive/

===

Section 7.2.2.2

I would like to point out that you define an adjacency set as a set of
adjacencies.  Perhaps you want to further refine this to add more clarity.

===

Section 9

s/In future/In the future/

===

Section 9

s/capabilities and may be remove/capabilities and may remove/