Subject: Re: [Last-Call] Genart last call review of draft-ietf-pce-binding-label-sid-11

From: Lars Eggert <[email-masked]>
Date: Thu, 3 Feb 2022 12:18:26 +0200

Theresa, thank you for your review. I have entered a No Objection ballot for this document.

Lars

On 2022-1-22, at 1:25, Theresa Enghardt via Datatracker <[email-masked]> wrote:
> 
> Reviewer: Theresa Enghardt
> Review result: Ready with Issues
> 
> 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"?
> 
> 
> --
> last-call mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/last-call