Thanks for the quick response, Samuel. I'll await a response from you and your co-authors.
Thanks, Ketan On Wed, May 28, 2025 at 3:22 PM Samuel Sidor (ssidor) <ssi...@cisco.com> wrote: > Thanks a lot, Ketan for your review. > > > > I’ll go over your comments and I’ll respond to you are concluding on some > of them with other co-authors. > > > > Regards, > > Samuel > > > > *From:* Ketan Talaulikar <ketant.i...@gmail.com> > *Sent:* Tuesday, May 27, 2025 5:41 PM > *To:* draft-ietf-pce-sid-a...@ietf.org > *Cc:* pce@ietf.org > *Subject:* AD review of draft-ietf-pce-sid-algo-19 > > > > Hi Authors/WG, > > Thanks for your work on this important document that tries to leverage IGP > technologies like FlexAlgo in PCE and builds on top for delivering new > use-cases. > > I would like to start with a high-Level overview of the specifications > based on my reading and some questions/suggestions. Please correct me if my > understanding is wrong. > > 1) There are 3 main types of extensions: > > a) Extensions to Metric Objects: these aren't specific to SR path types, > they aren't specific to FlexAlgo, they are more general. I suggest that > this be brought out in the Abstract and Introduction sections. Also, > that > the specifications for this be brought out as a top-level section in the > document. Please consider this as a major editorial comment. > > b) Algo support in SR-ERO/RRO: this is purely a signaling extension to > convey additional algo information and unrelated to any computational > enhancements to the PCE functionality. E.g., it could be used for say a > PCE-initiated explicit path. Would be great if this could be called out > and the document sections were re-ordered accordingly. > > c) Algorithm Constraint: this is getting algo (both FlexAlgo and any > other specific IGP algo) related aspects into the path computation. This > now enables the conveying of IGP aspects (e.g., for FlexAlgo the > optimization metric and other constraints from the FAD) in PCEP. > > Both (b) and (c) are covered by the same new capability being introduced > and specific to the SR path setup types. Ideally, it would have been > nice > to use different capabilities, but I guess (b) is really a small add-on > and > hence covered together with (c). I hope there is no one that tries to > implement (b) but not (c). > > 2) The (1)(c) functionality can be further split into two major types: > > a) FlexAlgo related (algo 128-255): this requires that the PCE is aware > of IGP signaling information related to FlexAlgo (such as FAD, algo > participation, metric types including generic metric, ASLA, etc.). It > would > be good to call out that it is expected that this topology information > is conveyed to the PCE via existing mechanisms (viz. IGPs or BGP-LS). > > b) non-FlexAlgo related (algo 0-127): Of these algos, only default (0) > and strict-SPF (1) are specified. Since the rest is undefined, they > cannot > be supported in PCEP as well - this needs to be called out along with > the > necessary error handling, if those are received. Further, default (0) > has > been already in use in PCEP for ages now - this needs to be called out. > That > brings the question on why algo 0 signaling is required when it is the > same as the absence of the algo? What remains (and is new) then is the > strict-spf (1) that is specified in RFC8402 (the specific section > reference > would be good to add). Now, this algo uses the same computation as > default > algo in the IGP (i.e., optimizing for IGP metric alone and no > participation or > constraints). Therefore, other than the strict SPF SIDs that can be > used, > there is nothing new. It would be good if there was a dedicated section > for > these non-FlexAlgos. > > 3) For FlexAlgo constraint, there is a need for PCE to perform > functionality > similar to what is done by IGPs : (a) performing the FAD selection, > (b) determining the FlexAlgo topology based on the FAD, the algo > participation, and the attributes being signaled, (c) doing the > topology > computation based on the participation and FAD. More specifically, I > believe, the computed path by PCE must lie within this FlexAlgo > topology. > While this is covered somewhat in 5.2.1, it does not cover all the > above > aspects. It would be good to specify with references to the RFC9350 > sections. Note that flexalgo features keep getting added in newer LSR > documents so it is better to just reference the base and call this out. > > 4) Then there is this "filtering" thing in 5.2.2 that is not clear to me. > Perhaps it is only meant or makes sense for algo 0 and 1? But the > section > also talks about FlexAlgo. The distinction and benefit of this over > what is > in section 5.2.1 is not clear to me. Could you please clarify? > > 5) Finally, there is this aspect for FlexAlgo constraint where there are > also > additional constraints that are carried using what exists currently in > PCEP. The handling of this is not clear. I believe the intention is that > both can be used and the computation would need to use "the union of > them". > There is however, the odd case of conflicts between the two - say FAD > optimization metric is different from what is being explicitly signaled > via PCEP; how is this error/conflict handled? > > What follows further below is the more detailed review inline in the > idnits output for > the v19 of the document. Please look out for <EoRv19> at the end of the > review > and if you don't see that, then the email has gotten truncated - please > look > at the mailer list archive for the full review. > > Thanks, > > Ketan > > > > > > 13 Carrying SR-Algorithm in Path Computation Element Communication Protocol > 14 (PCEP). > > <nit> please drop the "." from the title > > > > 17 Abstract > > 19 This document specifies extensions to the Path Computation Element > 20 Communication Protocol (PCEP) to enhance support for Segment Routing > 21 (SR) with a focus on the use of Segment Identifiers (SIDs) and SR- > 22 Algorithms in Traffic Engineering (TE). The SR-Algorithm associated > 23 with a SID defines the path computation algorithm used by Interior > 24 Gateway Protocols (IGPs). This document proposes an approach for > 25 informing PCEP peers about the SR-Algorithm associated with each SID > 26 used, as well as signaling a specific SR-Algorithm as a constraint to > 27 the Path Computation Element (PCE). The mechanisms for specifying an > 28 SR-Algorithm constraint allow for refined path computations that meet > 29 specific operational needs, such as low-latency or high-bandwidth > 30 paths based primarily on operator-defined criteria using Flexible > 31 Algorithms. Additionally, this document updates RFC 8664 and RFC > 32 9603 to allow encoding of SR-Algorithm of specific SID in Explicit > 33 Route Object (ERO) subobjects. > > <minor> Please consider splitting abstract into perhaps 3 paragraphs to > cover > the distinct key functionalities. And perhaps separate the last sentence > into > its own paragraph? > > > > 130 This document specifies extensions to PCEP to enhance support for SR > 131 Traffic Engineering (TE). Specifically, it focuses on the use of > 132 Segment Identifiers (SIDs) and SR-Algorithms. An SR-Algorithm > 133 associated with a SID defines the path computation algorithm used by > 134 Interior Gateway Protocols (IGPs). This document introduces > 135 mechanisms for PCEP peers to exchange information about the SR- > 136 Algorithm associated with each SID and to signal specific SR- > 137 Algorithm constraints to the PCE for path computation. Furthermore, > 138 this document updates [RFC8664] and [RFC9603] enabling the encoding > 139 of SR-Algorithms for specific SIDs within ERO subobjects. > > <minor> Same as the abstract. Please consider giving a high-level > functional overview of the reader (perhaps leverage the high-level > overview at > the start of this review?) before getting into the encoding bits/pieces > below. > > > > 147 * Extending SR-ERO, SRv6-ERO, SR-RRO and SRv6-RRO Subobjects to > 148 include Algorithm field > > <nit> s/include Algorithm field/include an Algorithm field > > > > 154 * Defining several new types for the METRIC Object required to > 155 support SR-Algorithm based path computation > > <major> As mentioned in my overview, the new metric types go beyond the > use-case related to SR-Algorithm. Please clarify. > > > > > > 190 The base PCEP specification [RFC4655] originally defined the use of > 191 the PCE architecture for MPLS and GMPLS networks with LSPs > 192 instantiated using the RSVP-TE signaling protocol. Over time, > 193 support for additional path setup types, such as SRv6, has been > 194 introduced [RFC9603]. The term "LSP" is used extensively in PCEP > 195 specifications and, in the context of this document, refers to a > 196 Candidate Path within an SR Policy, which may be an SRv6 path (still > 197 represented using the LSP Object as specified in [RFC8231]). > > <nit> Perhaps the above paragraph should be preceded by "Note:" ? > > > > 199 3. Motivation > > 201 Existing PCEP specifications lack the mechanisms to explicitly signal > 202 and negotiate SR-Algorithm capabilities and constraints. This limits > 203 the ability of PCEs to make informed path computation decisions based > 204 on the specific SR-Algorithms supported and desired within the > 205 network. > > <minor> Isn't this about PCE being able to leverage the computation and > simplification that IGPs have already done with their FlexAlgo (or algo) > computation in the underlying network? Plus, help stitch a path across a > multi-domain network where each of the domains uses FlexAlgo within it? > This > also has the potential to reduce the number of sids/labels that may be > required > for those paths that match exactly or closely the FlexAlgo constraints? > These > are just some suggestions for the WG to consider for providing a "high > level" > motivation for an operator to use this feature. > > > > 216 and constraints. This involves using the same ordered rules to > 217 select FADs when multiple options are available and considering node > > <major> Please add reference (not here but perhaps in 5.2.1?) to > > https://www.ietf.org/archive/id/draft-ietf-lsr-igp-flex-algo-reverse-affinity-06.html#section-11.3 > where the rules are being moved into an IANA registry. With a RFC editor > note to update the reference to this IANA registry at publication. > > > > 243 * SID types without algorithm specified - Certain SID types, such as > 244 Binding SIDs (BSIDs) [RFC8402], may not have an SR-algorithm > 245 specified. It may be inaccurate to state that an entire end-to- > 246 end path adheres to a specific algorithm if it includes a BSID > 247 from another policy. > > <major> In SRv6, the BSID can be allocated from an algo-specific SRv6 > Locator > which will result in the path to that BSID headend node following that > algo-specific path. However, it does not imply that the SR Policy > associated > with that BSID is also following that algo-specific path. Perhaps this > needs > to be clarified? > > > > 315 Reserved (24 bits): This field is reserved for future use and MUST be > 316 set to zero when sending and ignored when receiving. > > <minor> The "future use" seems to be rather limited here since it can be > only > used for a purpose that is associated with the Algorithm (i.e., A bit has > to > be set). Could you please clarify this? Also, wanted to make sure that > this is > indeed what the WG intended ... > > > > > > 352 * If A bit is 0, consistency rules defined in Section 5.2.1 of > 353 [RFC8664] applies. > > <question> Is it not time to introduce sub-TLVs for these objects? I > really hope > that the WG sees the challenges with continuing with this form of "flags > based > TLV encoding" without the use of sub-TLVs. I am not sure if this point was > discussed > previously in the WG. I am really concerned with the complexity and > fragility > that is being introduced in PCEP by such design choices. To be clear, this > question is not directed for this specific document - more for future/other > similar work. > > > > 428 * S (Strict): If set, the path computation at the PCE MUST fail > 429 if the specified SR-Algorithm constraint cannot be satisfied. > 430 If unset, the PCE SHOULD try to compute the path with SR- > 431 algorithm constraint specified. If the path computation using > 432 the specified SR-Algorithm constraint fails, the PCE SHOULD try > 433 to compute a path that does not satisfy the constraint. > > <major> I am not sure that I understand the two SHOULDs. The first one is > clear but the second one is not. Could you please elaborate on what is the > intention here? > > > > 435 * F (Flexible Algorithm Path Computation): If set, the PCE MUST > 436 perform path computation according to the Flexible Algorithm > 437 procedures outlined in Section 5.2.1. If unset, the PCE MUST > 438 adhere to the path computation procedures with SID filtering > 439 defined in Section 5.2.2. The flag MUST be ignored if the > 440 Algorithm field is set to a value in the range of 0 to 127. > > <major> It is not clear to me why there is an algo constraint when the PCE > is > not going to follow it. I don't understand the purpose of F flag when we > already have the S flag. Perhaps this is related to the "filtering" > feature in > 5.2.2 that I have not fully understood? > > > > > > 445 4.5. Extensions to METRIC Object > > 447 The METRIC object is defined in Section 7.8 of [RFC5440]. This > 448 document specifies new types for the METRIC object to enable the > 449 encoding of optimization metric types derived from the FAD during > 450 Flexible Algorithm path computation (see Section 5.2.1). While these > 451 new metric types are defined to support this specific use case, and > 452 their application is not restricted to Flexible Algorithm path > 453 computation and may be utilized in other relevant scenarios. > > <minor> Please introduce these extensions separately and as a standalone by > themselves. Perhaps also need to cover applicability to other path setup > types > like RSVP-TE as well as regular CSPF. Then, additionally link to the > FlexAlgo > to provide some context on why they were introduced in this document. > > > > > > 475 4.5.1. Path Min Delay Metric value > > <minor> Please consider renaming sections 4.5.1/2/3 for more clarity. Also > indent .2 and .3 under .1 since they are talking about the same type. Same > goes for the Bandwidth metric. Perhaps "Path minimum delay" as the .1 > title? > > > > > > 508 The P2MP Path Min Delay metric type of the METRIC object in PCEP > 509 encodes the Path Min Delay metric for the destination that observes > 510 the worst delay metric among all destinations of the P2MP tree. > > <major> The definition of "worst" is not clear. I believe it should say > "highest" value? Please check the same for other metric types. > > > > > > 529 When performing path computation for other algorithms (0-127) and the > > <major> Also for CSPF (e.g., RSVP-TE) and others? > > > > 591 5. Operation > > 593 The PCEP extensions defined in Section 4 of this document MUST NOT be > 594 used unless both PCEP speakers have indicated support by setting the > 595 S flag in the Path Setup Type Sub-TLV corresponding to the PST of the > > <major> This is clearly not the case for metric object. Right? Perhaps > consider introducing sub-sections for each of the features as in the > high-level overview? > > > > 601 The SR-Algorithm used in this document refers to a complete range of > 602 SR-Algorithm values (0-255) if a specific section does not specify > 603 otherwise. Valid SR-Algorithm values are defined in the registry > 604 "IGP Algorithm Types" of "Interior Gateway Protocol (IGP) Parameters" > 605 IANA registry. Refer to Section 3.1.1 of [RFC8402] and [RFC9256] for > 606 the definition of SR-Algorithm in Segment Routing. [RFC8665] and > 607 [RFC8667] are describing the use of the SR-Algorithm in IGP. Note > > <major> The default algo 0 and strict SPF algo 1 with a reference to > RFC8402 > need to be explicitly called out. Perhaps also a note that future standard > algos may be introduced by other documents and they will require PCE to be > updated for support - so this document covers only 0 and 1 in the range > 0-127. > > > > > > 611 5.1. ERO Subobjects > > 613 If a PCC receives the Algorithm field in the ERO subobject within > 614 PCInitiate, PCUpd, or PCRep messages and the path received from those > 615 messages is being included in the ERO of PCRpt message, then the PCC > 616 MUST include the Algorithm field in the encoded subobjects with the > 617 received SR-Algorithm value. > > <question> Is it so then that the PCC simply needs to copy this algo info > from > the ERO to the RRO? Is that something which needs to be done in all cases? > > > > > > 624 5.1.1. SR-ERO > > <major> Isn't it necessary to also cover RRO here briefly? > > > > > > 676 5.2. SR-Algorithm Constraint > > <major> I believe this applies to algo values 0 thru 255? Is there not a > need > for two sub-sections here - one for non-FlexAlgo and another for FlexAlgo? > For > non-FlexAlgo, these are standard algos that are published - today only > default > (0) and strict SPF (1). They need to be implemented and supported on the > PCE. > If there is an unsupported Algo received in the 0-127 range, then an error > needs > to be generated. > > > > > > 694 The PCE MUST NOT use Prefix SIDs of SR-Algorithm other than specified > 695 in SR-Algorithm constraint. It is allowed to use other SID types > 696 (e.g., Adjacency or BSID). Furthermore, the inclusion of a path BSID > > <major> Note that in SRv6, the adj-sid are algo-specific by the virtue of > their being allocated from an SRv6 Locator that is algo-specific. Even in > SR-MPLS, we now have algo-specific adj-sids - check > draft-ietf-lsr-algorithm-related-adjacency-sid. However, this algo-specific > characteristics of adj-sids may be of interest only for when protected > adj-sids > are used. Please clarify this. > > > > > > 703 Specified SR-Algorithm constraint is applied to the end-to-end SR > 704 policy path. Using different SR-Algorithm constraint in each domain > 705 or part of the topology in single path computation is out of the > 706 scope of this document. One possible solution is to determine FAD > 707 mapping using PCE local policy. > > <major> I am not sure how the above is supposed to work. The Algo > constraints > gives a specific algo number - there is no reference to its definition. > So, I > wonder how a solution is even possible with this constraint. If it is a > template for an intent that is needed, why even use algo and not use the > color > to algo mapping instead? This is changing the semantics for the Algo > Constraint and looks like overloading functionality. > > > > 716 If the headend is part of multiple IGP domains and the winning FAD > 717 for the specified SR-Algorithm has different constraints in each > 718 domain, the PCE implementation MAY have a local policy with a defined > 719 behavior for selecting a FAD for such path computation, or it may not > > <major> This is related to the previous comment. This "local policy" is > putting FAD > in PCEP as a standalone which looks problematic since FAD is a property of > IGPs. > This "may ... or may not" seems like hand waving to me ... am I missing > something? > > > > > > 725 If the NO-PATH object is included in PCRep, then PCE MAY include SR- > 726 Algorithm TLV to indicate constraint, which cannot be satisfied as > 727 described in section 7.5 of [RFC5440]. > > <major> How does PCE handle unsupported or unrecognized sub-TLVs/flags in > the > FAD definition? The way IGP routers handle is that they stop advertising > support for that particular algo if the selected FAD has something that > they > don't understand or support. However, this cannot be done by the PCE. This > document needs to cover this error handling. I think that at a minimum, > this > would need to be logged for the operator and then perhaps a new error > returned > by the PCE to the PCC when it is requested to compute with an algo > constraint > for such an algo. > > > > 743 document. Path computation must occur on the topology associated > 744 with the specified SR-Algorithm. It is allowed to use SID types > 745 other than Prefix SID (e.g., Adjacency or BSID), but only from nodes > 746 participating in specified SR-Algorithm. > > <major> Would it be correct to say ... The path computed by the PCE MUST > lie > within the FlexAlgo topology as indicated by the FAD in the underlying IGP. > > > > > > 749 specified in the FAD. The optimization metric type included in PCEP > 750 messages from the PCC MUST be ignored. The PCE SHOULD use the metric > 751 type from the FAD in messages sent to the PCC. If a corresponding > 752 metric type is not defined in PCEP, PCE SHOULD NOT include any METRIC > 753 object for the optimization metric. > > <major> Perhaps rephrase this as ... MUST use the metric type from the FAD > unless that metric type is not defined in PCEP ... and in that case, that > algo > cannot be supported (see error related to unsupported FAD). > > > > > > 764 The PCE MUST use constraints specified in the FAD and also > 765 constraints (except optimization metric type) directly included in > > <major> What happens in case of conflicting optimization metric types? Are > other parameters like metric bounds allowed in PCEP to be applied on top of > the FlexAlgo optimization metric? > > > > 792 If the specified SR-Algorithm is a Flexible Algorithm, the PCE must > 793 ensure that the IGP path of Flexible Algorithm SIDs is congruent with > 794 computed path. > > <major> So, for flex algos this requires the flex algo computation and > everything else that comes with that. What is the value/advantage of this > over > what is there in 5.2.1 ? How would the path be congruent if the > optimization > metrics are different between FAD and what is signaled via PCEP? > > > > 848 6.1. Control of Function and Policy > > 850 A PCE or PCC implementation SHOULD allow the capability of supporting > 851 PCEP extensions introduced in this document to be enabled or disabled > 852 as part of the global configuration. > > <minor> Is there an issue with this capability being always enabled? > > > > 866 6.4. Verify Correct Operations > > 868 An implementation SHOULD also allow the operator to view FADs, which > 869 MAY be used in Flexible Algorithm path computation defined in > 870 Section 5.2.1. > > 872 An implementation SHOULD allow the operator to view nodes > 873 participating in the specified SR-Algorithm. > > <major> Why would these not be a MUST? > > > > <EoRv19> > > >
_______________________________________________ Pce mailing list -- pce@ietf.org To unsubscribe send an email to pce-le...@ietf.org
