Skip to main content

Last Call Review of draft-ietf-pce-binding-label-sid-11
review-ietf-pce-binding-label-sid-11-genart-lc-enghardt-2022-01-21-00

Request Review of draft-ietf-pce-binding-label-sid
Requested revision No specific revision (document currently at 15)
Type Last Call Review
Team General Area Review Team (Gen-ART) (genart)
Deadline 2022-01-24
Requested 2022-01-10
Authors Siva Sivabalan , Clarence Filsfils , Jeff Tantsura , Stefano Previdi , Cheng Li
Draft last updated 2022-01-21
Completed reviews Rtgdir Last Call review of -10 by Ben Niven-Jenkins (diff)
Genart Last Call review of -11 by Reese Enghardt (diff)
Secdir Last Call review of -11 by Dan Harkins (diff)
Assignment Reviewer Reese Enghardt
State Completed
Review review-ietf-pce-binding-label-sid-11-genart-lc-enghardt-2022-01-21
Posted at https://mailarchive.ietf.org/arch/msg/gen-art/IyiRN3TEd1p_yEs0UviVwhgH04Q
Reviewed revision 11 (document currently at 15)
Result Ready with Issues
Completed 2022-01-21
review-ietf-pce-binding-label-sid-11-genart-lc-enghardt-2022-01-21-00
I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at

<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document: draft-ietf-pce-binding-label-sid-11
Reviewer: Theresa Enghardt
Review Date: 2022-01-21
IETF LC End Date: 2022-01-24
IESG Telechat date: Not scheduled for a telechat

Summary: The specification itself looks alright to me, but the document has
some clarity issues: From the abstract and introduction, it is difficult to
understand what is actually being specified in the document, so these parts
should be clarified before publication.

Major issues: None.

Minor issues:

Abstract:

"This document specifies the binding value as an MPLS label or Segment
   Identifier."
Please define the term "binding value" here or later in the document. Is this
an existing term from prior work or is this a new term introduced in this
document? Is the above sentence intended to say something like "This document
specifies the concept of a binding value, which can be either an MPLS label or
Segment Identifier"? If so, please rephrase. If not, please clarify in what way
the document "specifies the binding value".

"It further specifies an approach for reporting binding
   label/Segment Identifier (SID)"
Is "binding label/Segment Identifier" the same as "binding value" in the
previous sentence, or is it the same as a BSID? Is "Segment Identifier (SID)"
the same as Segment Identifier in the previous sentence? Please unify terms
when referring to the same concept.

As the document appears to specify an extension to PCEP, please mention this
protocol in the abstract. (And/or, if the extension applies to other protocols
as well, please say so in the abstract.)

Introduction:

"As per [RFC8402] SR allows a head-end node to steer a packet flow
   along any path. The head-end node is said to steer a flow into a
   Segment Routing Policy (SR Policy). Further, as per
   [I-D.ietf-spring-segment-routing-policy], an SR Policy is a framework
   that enables the instantiation of an ordered list of segments on a
   node for implementing a source routing policy with a specific intent
   for traffic steering from that node."

As written, this sounds like the second sentence describes the same thing as
the first sentence, i.e., as if a Segment Routing Policy is what happens when
the end-end note can steer a packet flow along any path, and the "Further"
makes it sound like the third sentence introduces a separate and new concept.
To me, it seems more likely that the first sentence refers to a different
concept than the second and third sentence, so the paragraph contrast steering
along any path VS steering into an SR policy constraining the choice of paths.
If that is true, I think it would help to make explicit which
sentences/concepts belongs together, e.g., to rephrase this part as: "As per
[RFC8402] SR allows a head-end node to steer a packet flow
   along any path. To constrain the paths along with a packet flow can be
   steered,
  the head-end note can steer a flow into a
   Segment Routing Policy (SR Policy). As per
   [I-D.ietf-spring-segment-routing-policy], an SR Policy is a framework […]"

"In this
   document, binding label/SID is used to generalize the allocation of
   binding value for both SR and non-SR paths."
Consider making it explicit that this sentence talks about terminology (if it
indeed does): "In this document, the term 'binding label/SID' is used to
generalize […]"

Perhaps it would be good to add terms like 'binding label/SID' and 'binding
value' to the Terminology section as well.

Reading the introduction, at times I found it difficult to understand what is
existing technology, what is new technology that this document specifies, what
is a motivation or use case for the newly specified technology, and whether
anything in there might just be desirable future work. I think it would help to
add a high-level summary early in the introduction, e.g., "This document
specifies an extension to PCEP to manage of BSIDs", after having introduced
PCEP, but before going into the complex specifics of PCEP and the presented use
case.

Other than a specifying protocol extension, the document also specifies
operational behavior in Sections 5 to 8. I think the Introduction should
mention these, e.g., adding something like "In addition to specifying a new
TLV, this document specifies how and when a PCC and PCE can use this TLV, how
they can can allocate a BSID, ...." (and/or summarize other aspects that the
document specifies). This would make it easier to get an overview of what kinds
of things are specified in the document.

Then, in several places where the document currently says things like "it may
be desirable for a PCC to report the binding
   label/SID to the stateful PCE", the document could instead say "this
   extension specified in this document provides a way for a PCC to report the
   binding label/SID to the stateful PCE", if that's true, or otherwise say
   something like "it may be desirable […] but this is out of scope for this
   document".

"A PCC could report to the stateful PCE the binding label/SID it
   allocated via a Path Computation LSP State Report (PCRpt) message.
   It is also possible for a stateful PCE to request a PCC to allocate a
   specific binding label/SID by sending a Path Computation LSP Update
   Request (PCUpd) message."
Is this another extension to PCEP that is being specified in the document? Is
it merely using the same PCEP extension talked about elsewhere in the document?
Or is it an existing mechanism specified in another document? Or is it merely
something that could be specified in the future? To make this clear, it would
help to add something like "Using the extension defined in this document, it is
also possible for a stateful PCE […]".

" In this document, we introduce a new OPTIONAL TLV that a PCC can use
   in order to report the binding label/SID associated with a TE LSP, or
   a PCE to request a PCC to allocate a specific binding label/SID
   value."
Is this the only extension to PCEP (at least I assume it's PCEP - or can this
TLV be used in multiple protocols? Please make this explicit) that is being
specified in this document? Perhaps it'll be good to connect this part to the
paragraphs above that talk about extensions to PCEP that are needed and why.
For example, this paragraph could start with "To implement the needed changes
to PCEP, in this document, we introduce a new OPTIONAL TLV [...]"

"   Additionally, to support the PCE-based central controller [RFC8283]
   operation where the PCE would take responsibility for managing some
   part of the MPLS label space for each of the routers that it
   controls, the PCE could directly make the binding label/SID
   allocation and inform the PCC.  See Section 8 for details."
Is this another way to use the PCEP extension defined in this document? If yes,
please say so, e.g., start the paragraph by "As another way to use the
extension specified in this document, to support the PCE-based central
controller […]".

Nits/editorial comments:

Section 11.1:

"For BT is 2"
Should this be "If BT is set to 2"?