Hi John, Thank you for the detailed review of this draft.
FYI: There were additional WGLC comments from Samuel and Andrew, about using Reverse ERO instead of using Reverse LSP. This will further change the procedures described in this draft. We will share an updated draft for review as soon as it is available. Thanks, Rakesh (for authors) On Mon, Nov 24, 2025 at 11:14 PM Rakesh Gandhi <[email protected]> wrote: > Thank you John for the thorough review of the document and the suggestions. > > Attached please find work in progress, updated draft and the changes. > > Please see inline with <RG>..... > > > On Mon, Nov 17, 2025 at 4:23 PM Scudder, John <john.scudder= > [email protected]> wrote: > >> Hello, >> >> I have been selected to do a routing directorate “early” review of this >> draft. >> >> https://datatracker.ietf.org/doc/draft-ietf-pce-sr-bidir-path/ >> >> The routing directorate will, on request from the working group chair, >> perform an “early” review of a draft before it is submitted for publication >> to the IESG. The early review can be performed at any time during the >> draft’s lifetime as a working group document. The purpose of the early >> review depends on the stage that the document has reached. >> >> As this document is in working group last call, my focus for the review >> was to determine whether the document is ready to be published. Please >> consider my comments along with the other working group last call comments. >> >> For more information about the Routing Directorate, please see >> https://wiki.ietf.org/en/group/rtg/RtgDir >> >> Document: draft-ietf-pce-sr-bidir-path-16 >> >> Reviewer: John Scudder >> >> Review Date: November 17, 2025 >> >> Intended Status: Standards Track >> >> Summary: >> >> I have significant concerns about this document. It needs more work >> before being submitted to the IESG. >> >> Comments: >> >> I found this document to be clear and readable, with the exception of >> Section 4, about which I have concerns. I've called some of these out in >> detail in comments below, but I can summarize my concerns in two overall >> points: >> >> First, Section 4 is called "PCEP Procedures," but it's not written >> procedurally, and it isn't clear to me (a non-expert on PCEP) how I would >> implement this. >> > > <RG> The section 4 is reworked as PCE/PCC behaviours with some redundant > text removed. Please let us know if it reads better. > <RG> We could rename the section to better reflect the updated draft, > "PCE/PCC Behaviours"? > > > >> >> Second, some of Section 4 presents existing requirements as though they >> were new requirements. This is not considered best practice. I can't >> determine if the examples I found are exceptions or if most of Section 4 is >> a restatement of existing requirements, but please look into this. >> > > <RG> Note that association for RSVP LSP works differently than the SR LSPs > as there is no signalling. So the procedures in RFC 9059 are slightly > different. > > >> >> Putting these two concerns together, I wonder if this document could be >> shorter by explaining only the elements that are new and differ from RFC >> 9059 and the rest of the underlying specifications. >> >> (I accept that if I were a PCE expert, I might find Section 4 to be >> “obvious”. You should keep in mind that during IETF LC and IESG review, >> many/most of your reviewers will not be PCE experts.) >> > > <RG> Ack. > > >> Below I have some specific questions, comments, and suggestions. >> >> * In Section 4.1, Figure 1b shows a PCRpt example. The text of Section >> 4.1 doesn't talk about this. A similar point applies to §4.2, Figure 2c. >> > > <RG> Added in PCC behaviour in both cases. > > >> >> * In Section 4.2, Figures 2a-2c are labeled Step 1-3. There is no >> discussion of steps or sequencing in the text of Section 4.2. >> > > <RG> Figure is a high-level pictorial view of the two methods, PCE > initiated and PCC initiated, to be able to see the difference in steps. > <RG> I think, adding text for the steps would be repetitions of PCE/PCC > behaviours already in section 4. > > >> >> * In the IANA section, I suggest, >> >> OLD: >> This document defines a new Association Type, originally described in >> [RFC8697]. IANA is requested to assign the following value in the >> "ASSOCIATION Type Field" registry [RFC8697] within the "Path Computation >> Element Protocol (PCEP) Numbers" registry group: >> >> Type Name Reference >> --------------------------------------------------------------------- >> 8 Double-Sided Bidirectional [This document] >> (Early Allocation) with Reverse LSP Association >> >> NEW: >> This document defines a new Association Type, originally described in >> [RFC8697]. IANA is requested to update the value it has assigned through >> the early allocation process in the "ASSOCIATION Type Field" registry >> [RFC8697] within the "Path Computation Element Protocol (PCEP) Numbers" >> registry group, making it permanent: >> >> Type Name Reference >> --------------------------------------------------------------------- >> 8 Double-Sided Bidirectional [This document] >> with Reverse LSP Association >> > > <RG> Fixed. > > >> >> Similarly, in Section 3.1, >> >> OLD: >> Association Type (value 8, early allocation) to be assigned by IANA) = >> Double-Sided Bidirectional with Reverse LSP Association >> >> NEW: >> Association Type (value 8) = Double-Sided Bidirectional with Reverse LSP >> Association >> > > <RG> Fixed. > > >> >> If you want to reference IANA in Section 3.1 at all (optional IMO), you >> can put in an xref to your IANA section. >> > > <RG> Skipped. > > >> >> * In Section 3.1, you have, >> >> "The bidirectional association is considered to be both dynamic and >> operator-configured in nature." >> >> In reading the subsequent description, I think you must mean something >> more like, >> >> "The bidirectional association can be either dynamic or >> operator-configured." >> > > <RG> Fixed. > > >> >> * In Section 3.1, you have, >> >> "An SR Path (forward or reverse) MUST NOT be part of more than one >> 'Double-Sided Bidirectional with Reverse LSP Association'." >> >> Is there any suggested or mandated protocol action if the MUST NOT is >> violated? >> > > <RG> Added following text: > > > * An SR Path (forward or reverse) MUST NOT be part of more than one > 'Double-Sided Bidirectional with Reverse LSP Association'. > > *A PCE, upon detecting this condition, MUST NOT send the reverse SR > Path to ingress node PCC.* > > > >> >> * Throughout the document, you refer to LSPs. Am I correct that in PCEP, >> "LSP" has come to mean "any path established using PCEP" and doesn't have >> its usual meaning of "an MPLS Label Switched Path"? If so, even though PCE >> experts may know this intuitively, it might be worth adding a short >> definition at the front. >> > > <RG> Added following text: > > > > > > > * As specified in [RFC8664], an SR path corresponds to an MPLS Label > Switching Path (LSP) in PCEP when using the SR-TE path setup type. As > specified in [RFC9603], the term "LSP" used in the PCEP specifications > would be equivalent to an SRv6 path (represented as a list of SRv6 > segments) in the context of supporting SRv6 in PCEP using SRv6 path setup > type.* > > > >> >> * In Section 4, you have "These PCInitiate message MUST NOT trigger >> initiation of SR Paths on PCCs." Is this an example of restatement of >> existing requirements? >> > > <RG> Fixed. > > >> >> * In Section 4.5, you have "Note that the PLSP-ID space is independent at >> each PCC, the PLSP-ID allocated by the egress PCC cannot be used for the >> LSP at the ingress PCC (PLSP-ID conflict may occur)." >> Shouldn't that be "might not be able to be used", i.e. we can't assume >> it can be used that way? The way you have written the sentence right now, >> you are saying I am forbidden from using the same PLSP-ID on both ends. >> That can't be right if the spaces are independent; independence implies >> that it would be fine to use the same number in both places if that's how >> it happens by chance. >> > > <RG> Fixed. > > >> >> * There are many RFC 2119 keywords in Section 4 and its subsections >> (MUST, MAY, etc). It's not clear to me that these are genuinely stating new >> requirements, as opposed to reminding the reader of requirements that exist >> in the underlying specifications. > > > <RG> The section 4 is reworked as PCE/PCC behaviours with some redundant > text removed. Please let us know if it reads better. > > > >> For example, Section 4.5: "The PCC upon receiving the PCInitiate MUST >> locally assign a new PLSP-ID and it MUST issue a PCRpt to PCE for this LSP >> containing the new PLSP-ID." But these appear to be restating requirements >> that exist in RFC 8281. > > > <RG> Section 4.5 has been edited. Please let us know if it reads better. > > >> Another example is Section 4.6, which appears to do nothing but restate >> existing requirements. >> > > <RG> Removed. > > >> Please do not restate existing requirements as though they were new >> requirements. Either don't mention them at all if not needed, or mention >> them in a way that makes it clear you're restating the underlying >> requirement as a reminder. IMO you should only do this if the reminder is >> needed for you to explain your (new) procedure. >> >> > <RG> Added following to clarify that the procedure in this draft is > different. > > > > > * Note that the procedure in this document differs from the procedure > defined in Section 5 of [RFC9059] where the ingress and egress node PCCs > learn the reverse LSPs via RSVP signaling and report them to PCE. * > > > > > > > > > > > > > > > > > > Nits: >> >> * The draft is in decent shape with respect to readability, thank you! I >> would still recommend running a grammar checker, which could help correct >> minor problems such as "For bidirectional SR paths, there are use-cases >> such as directed BFD [RFC9612] and Performance Measurement (PM) >> [I-D.ietf-spring-stamp-srpm] those require ingress node (PCC) to be aware >> of the reverse direction SR Path." (Should be "which require the ingress" >> not "those require ingress". My grammar checker was able to detect this.) >> > > <RG> Fixed. > > >> >> * "Furthermore, PCEP can be used for computing SR TE paths in the >> network." >> >> PCEP is a protocol; it doesn't do computation. I guess something like >> "PCEP can be used to allow a PCE to compute SR TE paths in the network"? >> > > <RG> Fixed. > > >> >> * In the introduction, you xref and inline-expand PCEP in paragraph 2 and >> again in paragraph 3. Thanks for referencing and expanding it, but you only >> need to do it once. >> >> > <RG> Fixed. > > Many thanks, > Rakesh (for authors) > > > > >> _______________________________________________ >> Pce mailing list -- [email protected] >> To unsubscribe send an email to [email protected] >> >
_______________________________________________ Pce mailing list -- [email protected] To unsubscribe send an email to [email protected]
