Hi Theresa, Many thanks for your reply! We have updated to address your comments. Please see my reply inline for more information. Hope it address your comments.
Thank you! Cheng Htmlized: https://datatracker.ietf.org/doc/html/draft-ietf-pce-binding-label-sid Diff: https://www.ietf.org/rfcdiff?url2=draft-ietf-pce-binding-label-sid-12 -----Original Message----- From: Theresa Enghardt via Datatracker [mailto:[email protected]] Sent: Saturday, January 22, 2022 7:25 AM To: [email protected] Cc: [email protected]; [email protected]; [email protected] Subject: Genart last call review of draft-ietf-pce-binding-label-sid-11 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". [Cheng] Updated as suggested. "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. [Cheng]Yes and updated. 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.) [Cheng]Updated. 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 […]" [Cheng] Thanks for suggestion, I updated it to - As per [RFC8402] SR allows a head-end node to steer a packet flow along a given path via a Segment Routing Policy (SR Policy). As per [I-D.ietf-spring-segment-routing-policy], an SR Policy is a […] "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 […]" [Cheng]Sure! Perhaps it would be good to add terms like 'binding label/SID' and 'binding value' to the Terminology section as well. [Cheng] Added! 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. [Cheng] Good suggestions, I have added subsections as well to help with readability. 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". [Cheng] Since we are dealing with motivation there, I changed it to - When a stateful PCE is deployed for setting up TE paths, a binding label/SID reported from the PCC to the stateful PCE is useful for the purpose of enforcing end-to-end TE/SR policy. "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"? [Cheng] Updated. _______________________________________________ Pce mailing list [email protected] https://www.ietf.org/mailman/listinfo/pce
