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 <noreply@xxxxxxxx> 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 > last-call@xxxxxxxx > https://www.ietf.org/mailman/listinfo/last-call
Attachment:
signature.asc
Description: Message signed with OpenPGP
-- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
