Thanks a lot, Dhruv for your review. Please check response inline <S>. I'm updating the draft meanwhile.
Regards, Samuel From: Dhruv Dhody <d...@dhruvdhody.com> Sent: Sunday, April 6, 2025 6:58 PM To: draft-ietf-pce-sid-a...@ietf.org Cc: pce@ietf.org Subject: Shepherd review of draft-ietf-pce-sid-algo # Shepherd review of draft-ietf-pce-sid-algo I've reviewed the document in preparation for its publication request. The I-D is nearly ready to progress, but there are a few issues that should be addressed before it is sent to the IESG. These issues are aimed at ensuring the document meets the requirements for publication on the Standards Track; they do not pertain to the core protocol extension itself, which is sound. ## Major - Almost all references are currently marked as Normative. Please review whether each one truly needs to be classified as such. Refer to - https://datatracker.ietf.org/doc/statement-iesg-iesg-statement-normative-and-informative-references-20060419/; I would consider RFC 5541, RFC 8665, RFC 8667, I-D.ietf-pce-pcep-yang to be Informative. <S> I agree for RFC5541 and I-D.ietf-pce-pcep-yang, but I personally still consider RFC8665, RFC8667 as normative as really any PCE implementation MUST follow multiple rules from those RFCs (e.g. FAD selection logic, topology pruning,...), so it is practically impossible to implement PCEP extensions from this draft without understanding and implementing logic described in those RFCs. - Section 3.1, please remove the figures, as this section is only introducing a new flag. Some of the flags have early allocations while others do not, which can be perceived as codepoint squatting. In any case, redrawing the entire figure is not considered good practice. Instead, you should reference the RFC where the field is originally defined and clearly state that a new flag is being introduced in this document. Also, please provide a clearer description for the new flags being defined. <S> Sure, I can drop them. - Section 3.2 and 3.3, the text in this section needs improvement. Please include explicit references for where these subobjects are defined. Since this document modifies an existing subobject, it is important to clearly state what changes are being made. It is appropriate to formally update RFC 9603 and RFC 8664 to reflect these changes. Additionally, please include clear text explaining how interoperability will be handled with implementations that do not support this extension-it is best to be explicit about such backward compatibility behavior. A precise description of the new flag and additional field being introduced is also needed. <S> Ack, I'll add some details about Algorithm field and flag and I'll point to section 4.1, which is describing already more details about processing. For backward compatibility - there is already capability described in sections 3.1.1/3.1.2 and capability handling is described in section 4.1 and also section 4 is already stating that extensions MUST NOT be used if capability was not advertised by both peers, so why we need to mention anything else about backward compatibility? For update of RFC9603 and RFC8664 - sure, I can add it. - Section 4.2, please make things clear on what are the MUST conditions when the strict flag is set and what "a path that that does not satisfy the specified SR-algorithm constraint" means when unset. The current text does not mention the strict flag at all. Same for 4.2.1. <S> Path-computation is always done based on combination of S (strict) and F (Flexible Algorithm Path Computation). In general, MUST conditions if S flag is set are described in sections 4.2.1/4.2.2 based on F flag set. S flag = 0 is then just allowing fallback to path which is satisfying all other constraints same way like if this constraints is not specified at all. I'll check if there is more to add or clarify it somehow. - Section 4.2.1, as per the IESG statement[https://datatracker.ietf.org/doc/statement-iesg-statement-on-clarifying-the-use-of-bcp-14-key-words/] we should be using BCP14 keywords only for interop or limit behavior leading to harm, but some use is about how PCE does path computation - "The PCE MUST follow IGP Flexible Algorithm path computation logic..." and "The PCE MUST optimize computed path ...", please check and update where needed. <S> Sure, I can change to "must". I can still imagine that if PCE is not following those rules, then it can compute path, which can lead to traffic drops or at least to traffic to be forwarded using suboptimal path, but even "must" should be sufficient. - Section 4.2.2, how does PCE ensure the MUST condition at - "If the specified SR-Algorithm is Flexible Algorithm, the PCE MUST ensure that IGP path of Flexible Algorithm SIDs is congruent with computed path."; we need some description of what being congruent looks like. Also, what does PCE do if this is not the case? <S> This is same as in case of non-Flex-algo path-computation on PCE done for any metric type except IGP - e.g. if PCE is doing SR-MPLS path-computation with TE optimization metric, then it may need to use adjacency SIDs instead of prefix SIDs to ensure that traffic is forwarded in optimal path and not in best path based on IGP metric. We are not introducing anything new here, it is mostly to mentioned as reminder to reader that same rule applies here. How implementation will achieve it is out of scope of the draft. I'll see if I can come with some example or at least some extra statement to clarify it. ## Minor <S> Ack for minor comments, where I'm not responding explicitly. - Add some PCEP-related text in the introduction as well. It jumps directly to SR Algo. I suggest having a seperate motivation section and some of the text from the Introduction can be moved there. - Move the "Requirements Language" section in the body of the I-D as per https://www.rfc-editor.org/rfc/rfc7322.html#section-4.8.2 - In "Only the first instance of this TLV SHOULD be processed, subsequent instances SHOULD be ignored", change SHOULD to MUST. - Section 3.4, please add clear length for each of the fields in the TLV. We need some text to also be clear on what exactly it means to use an Algorithm as a constraint, with a forward reference to section 4.2 - Section 3.4, hope this is clearer ```` OLD: * S (Strict): If set, the PCE MUST fail the path computation if specified SR-Algorithm constraint cannot be satisfied. ... If such computation is not successful, then a path that that does not satisfy the specified SR- algorithm constraint can be computed. NEW: * S (Strict): If set, the path computation at the PCE MUST fail if the specified SR-Algorithm constraint cannot be satisfied. ... If the path computation using the specified SR-Algorithm constraint fails, the PCE SHOULD try to compute a path that does not satisfy the constraint. ```` - Section 3.4, can one sentence description be added for F flag? Also why a SHOULD in "The flag SHOULD be ignored if the Algorithm field is set to value in range 0 to 127" (and not MUST)? - Section 3.5, add some description of why new metric types are needed. Move text from section 4.2.1. In the initial list, the sub-section numbers do not match the list. Use xref to avoid this. And remove this text - "Metric type values for "Path Bandwidth Metric", "P2MP Path Bandwidth Metric" and "User Defined metric" are suggested values only for IANA to allocate." <S> Reason why new metric types are needed is already described in Introduction and also in Section 4.2.1, but I can mentioned there as well. I'll update based on rest of your comment. - Section 3.5, add this - ```` The following terminology is used and expanded along the way. o A network comprises of a set of N links {Li, (i=1...N)}. o A path P of a point-to-point (P2P) LSP is a list of K links {Lpi,(i=1...K)}. o A P2MP tree T comprises a set of M destinations {Dest_j, (j=1...M)}. ```` - Section 3.5.1, ```` OLD: * A Min Link Delay metric of link L is denoted D(L). * A path P of a P2P LSP is a list of K links {Lpi,(i=1...K)}. * A Path Min Delay metric for the P2P path P = Sum {D(Lpi), (i=1...K)}. NEW: * A Min Link Delay metric of link L is denoted D(L). * A Path Min Delay metric for the P2P path P = Sum {D(Lpi), (i=1...K)}. ```` - Section 3.5.2 and section 3.5.5, remove the "A P2MP tree T..." as that is moved to the top. - Section 3.5.4, there needs to be a reference to Bandwidth Metric for the link early on. You can move the sentence from 3.5.6 to 3.5.4 <S> Wouldn't it be better than to just move section 3.5.6 before 3.5.4 and 3.5.5? - Section 3.5.4, remove the "A path P of a P2P LSP..." as that is moved to the top. - Section 3.5.6, inappropriate use of MAY in "The section 4 of [I-D.ietf-lsr-flex-algo-bw-con] defines a new metric type "Bandwidth Metric", which MAY be advertised in their link metric advertisements." (similar issue in 3.5.7) - Rephrase ```` OLD: When performing path computation for other algorithms and Generic Metric sub-TLV with Bandwidth metric type is not advertised for the link then PCE implementation MAY have local policy to specify attributes similar to section 4.1.3 and 4.1.4 in [I-D.ietf-lsr-flex-algo-bw-con] and compute metric value automatically or the link SHOULD be treated as if the metric value is not available for other metric types (e.g. use default value instead). If Bandwidth metric value is advertised for the link, then PCE MUST use value advertised and compute path metric as described in Section 3.5.5 and 3.5.6. NEW: When performing path computation for other algorithms (0-127) and the Generic Metric sub-TLV with Bandwidth metric type is not advertised for a link, the PCE implementation MAY apply a local policy to derive a metric value (similar to the procedures in section 4.1.3 and 4.1.4 of [I-D.ietf-lsr-flex-algo-bw-con]) or the link MAY be treated as if the metric value is unavailable (e.g. by using a default value). If the Bandwidth metric value is advertised for a link, the PCE MUST use the advertised value to compute the path metric in accordance with Section 3.5.4 and 3.5.5. ```` - Section 3.5.7, remove "or P2MP path" as the sum in the sentence is applicable to P2P only. - Section 3.5, I am assuming this is applicable without an algorithm as well? Let's be explicit about it. - Section 4, rephrase and add an error handling ```` OLD: The PCEP protocol extensions defined in Sections 3.2, 3.3 and 3.4 of this draft MUST NOT be used if one or both PCEP speakers have not indicated the support using S flag in Path Setup Type specific Sub- TLVs in their respective OPEN messages. NEW: The PCEP extensions defined in Sections 3.2, 3.3 and 3.4 of this document MUST NOT be used unless both PCEP speakers have indicated support by setting the S flag in the Path Setup Type Sub-TLV corresponding to the PST of the LSP. If this condition is not met, the receiving PCEP speaker MUST respond with a PCErr message with Error-Type 19 (Invalid Operation) and Error-Value TBD (Attempted use of SR-Algorithm without advertised capability). ```` - Section 4.1, please break this section into separate sections for SR-ERO and SRv6-ERO. The merge is not helping and in fact, leading to some errors such as mentioning SID structure for SR-ERO cases! <S> I was trying to avoid repeating as most of statements in that section is really applicable to both, but I'll see what can be done. - I suggest this rephrase because RFC 5440 was about unrecognized TLV. ```` OLD: If PCEP peer receives LSPA object with SR-Algorithm TLV in it, but the S flag was not advertised, then PCEP peer MUST ignore it as per Section 7.1 of [RFC5440]. NEW: A PCEP peer that did not advertise the S flag in the Path Setup Type Sub-TLV corresponding to the LSP's PST MUST ignore the SR-Algorithm TLV on receipt. ```` - Section 4.2.1, in "The PCE MUST optimize computed path based on metric type specified in the FAD, metric type included in PCEP messages from PCC MUST be ignored"; rephrase this for the optimization metric only so that the bound metric type like hop-limit can still be supported. <S> Isn't it sufficient to just extend existing statement like this: "The PCE MUST optimize computed path based on metric type specified in the FAD, optimization metric type included in PCEP messages from PCC MUST be ignored". It may be better than trying to cover all other existing or future flags, which can be set in metric object. - Section 4.2.1, how about this rephrase - ```` OLD: The PCE SHOULD use metric type from FAD in messages sent to the PCC. If corresponding metric type is not defined in PCEP, PCE SHOULD skip encoding of metric object for optimization metric. NEW: The PCE MUST include the METRIC object with the computed metric value corresponding to the metric type specified in the FAD, in messages sent to the PCC, unless that metric type is not defined in PCEP. ```` <S> I personally consider use of MUST in this case as risky. What if FAD was not found (so PCE does not have any metric-type to use) or if PCEP metric is defined in PCEP, but it was introduced independently from this draft and implementation does not support it? There can be more cases like that. - Section 5.1, change MAY to SHOULD. - Section 5.3, the first sentence is not needed, it is already covered at the start of section 5. The requirements there do not seem applicable to PCEP implementation, they are more for the IGP implementation. Also, you are missing a subsection "Requirements on Other Protocols", where this interaction with IGP can be captured. <S> I don't understand why those should be related to IGP. PCE may not be even part of any IGP, but it can learn topology from BGP-LS for example and operator should be able to check which FADs are learned from BGP-LS for the path computation by PCE. For "Requirements on Other Protocols", I can repeat reference to "RFC8665, RFC8667". - Section 5.4, the first sentence is not needed. - There is scope for improvement in Section 6. It could be expanded to further list operational considerations on how the IGP and PCEP interact and the operational challenges involved there. <S> I'll think about it, but I personally don't see much to be listed on top what is already described in existing section 4. - Section 8, I suggest adding "Hence, securing the PCEP session using Transport Layer Security (TLS) [RFC8253] is RECOMMENDED." - Looking at https://www.iana.org/assignments/pcep/pcep.xhtml#sr-ero-flag-field and https://www.iana.org/assignments/pcep/pcep.xhtml#srv6-ero-flag-field we should add an alphabet (A) for this Flag in the description and update the section 9.3 and 9.4 ## Nits - Please do a grammar check. I suggest using grammarly. - Section 3.4, s/PST (Path Setup type) = SR or SRv6/ PST (Path Setup type) = 1 or 3 for SR-MPLS and SRv6 respectively/ - Add a reference for IEEE floating point format. - s/The encoding for the User Defined metric values is encoded in IEEE floating point format/User-Defined metric values are encoded using the IEEE floating-point format/ - s/PCEP protocol extensions/PCEP extensions/ - s/PCUpdate/PCUpd/ - Fix the affiliation/email for Mike - You list Tom both in Ack and as a contributor; is that intentional? - All tables have weird formatting with the 2nd-row blank! Thanks! Dhruv
_______________________________________________ Pce mailing list -- pce@ietf.org To unsubscribe send an email to pce-le...@ietf.org
