# 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. - 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. - 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. - 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. - 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. - 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? ## Minor - 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." - 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 - 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! - 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. - 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. ```` - 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. - 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. - 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
