[Pce] Re: Shepherd review of draft-ietf-pce-sid-algo

["Samuel Sidor (ssidor)" <[e-mail]>]

Hi Shraddha,

Thanks a lot, for your review. Please find responses inline <S>.

Regards,
Samuel

From: Shraddha Hegde <[e-mail]>
Sent: Wednesday, April 23, 2025 9:16 AM
To: Dhruv Dhody <[e-mail]>; [e-mail]; [e-mail]
Subject: RE: Shepherd review of draft-ietf-pce-sid-algo

Authors,

Pls find review comments on the draft.

1. Sec 1 Introduction
"Defines several new types for METRIC Object required to support
      SR-Algorithm based path computation"

                The METRIC object can be independent of the SR-algorithm.
                It is possible to compute paths based on a new METRIC TYPE
                even when the network has not deployed flex-algorithm.

                If SR-algorithm has been specified as a constraint then
                the type of METRIC to be used for computation should
                match the METRIC type thats defined by the FAD of that algorithm

                These aspects need to be clarified in the document

<S> I’m updating the draft based on comments from Dhruv (please find attached “work-in-progress ” version) – this is the snippet, which I’m adding into section 3.5:

This document specifies new types for the METRIC object to enable the encoding of optimization metric types derived from the FAD during Flexible Algorithm path computation (see Section 4.2.1). While these new metric types are defined to support this specific use case, their application is not restricted to Flexible Algorithm path computation and may be utilized in other relevant scenarios.

For metric-type specific in metric object matching metric-type from FAD if SR-algorithm constraint is specified – that is already specified based on flags and algo specified later in the document (see sections 4.2.1 and 4.2.2). E.g. if algorithm specified is not Flex-algo (0-127), then optimization metric is still specified in metric object only.

2.
" The combinations of constraints specified in the FAD and constraints
   directly included in PCEP messages from PCC may decrease a chance
   that Flex-algo specific Prefix SIDs represents optimal path while
   satisfying all specified constraints, as a result a longer SID list
   may be required for the computed path.  Adding more constraints on
   top of FAD requires complex path computation, and may reduce the
   benefit of this scheme."

   If no other constraints other than the ones specified in Flex-algo are needed
   operator would just use flex-algo paths for the traffic. The reason
   PCE has to compute paths is that there are either additional constraints
   or it's multi-domain. The above paragraph eeds to be reworded or removed.

<S>That statement is just explaining that path-computation with Flex-algo and some other constraints may be complex and may not result in what operator really expected from computed segment-list/path. It is not introducing any new restrictions (all combinations are still allowed). IMHO main use-case for using PCE was always multi-domain path computation and there is no change to that. There were already multiple discussions about allowing/blocking some specific combinations of constraints, but we finally decided to do not block anything and just keep implementation to support what they really consider as useful (e.g. does not it really makes sense to support Flex-algo with affinities/admin-groups specified in FAD and different set of affinities in PCRpt? Most likely not, but some implementation may still decide to allow it) and reject with PCErr if specific combination is not supported.

3. sec 3.5.3
Sec 3.5 need to be consistent with the new Metric types being defined.
The min/max unidirectional link delay does not appear in sec 3.5.

<S> Ack, updated already in attached version.

4. 3.5.6.  Path Bandwidth Metric value
I would suggest to change this to "Automatic Bandwidth Metric"

<S> Any reason for introducing this kind of inconsistency against “draft-ietf-lsr-flex-algo-bw-con” and other existing drafts (e.g. see bandwidth metric-type in section 5.6.6 in draft-ietf-idr-bgp-ls-sr-policy)? If it is not called “Automatic Bandwidth Metric” in LSR or IDR.

5. In the absense of Flex-algo, SR-TE is allowed to use
link attributes from legacy ISIS/OSPF Te advertisements including the
new generic metric. This needs to be captured
<S> The way how draft is structured:

  *   if algo is 0-127 (non-Flex-algo) – no change against existing behavior (legacy attributes applies same way like for other PCEP drafts)
  *   if algo is 128-255 (FA) – dedicated Flex-algo specific section applies, which is describing usage of FA ASLA attributes.
I can still add some statement in section 4.2.2.

6.  sec 3.5
It is not clear to me why PCEP needs a seperate code point
for P2MP for the metric type.
Metric type is derived from IGP based on link advertisements
It is better to keep the metric-types congruent to IGP
metric-type registry. That simplifies the operations to a large extent

<S> This is done mostly because definitions for P2P and P2MP metric-types are different (see for example P2P IGP metric type from RFC5440 vs P2MP IGP metric type from RFC 8306).
Same thing is done for other metric-types in PCEP, see IANA registry in PCEP:
https://www.iana.org/assignments/pcep/pcep.xhtml#metric-object-ni-field
I can still imagine introducing just single metric-type for both (P2P and P2MP), but I personally prefer consistency across PCEP metric types over partial (we have our own definition, different metric type values,…) consistency between IGP and PCEP metric types.

Rgds
Shraddha



Juniper Business Use Only
From: Dhruv Dhody <[e-mail]<mailto:[e-mail]>>
Sent: 06 April 2025 23:05
To: Shraddha Hegde <[e-mail]<mailto:[e-mail]>>
Subject: Fwd: Shepherd review of draft-ietf-pce-sid-algo

[External Email. Be cautious of content]

Hi Shraddha,

Can I make a request for you to review the IGF Flex Algo relationship in draft-ietf-pce-sid-algo with RFC 9350 and draft-ietf-lsr-flex-algo-bw-con? You are the expert and your opinion will help me make sure that the I-D is ready to be sent to the IESG!

Thanks!
Dhruv

---------- Forwarded message ---------
From: Dhruv Dhody <[e-mail]<mailto:[e-mail]>>
Date: Sun, Apr 6, 2025 at 10:28 PM
Subject: Shepherd review of draft-ietf-pce-sid-algo
To: <[e-mail]<mailto:[e-mail]>>
Cc: <[e-mail]<mailto:[e-mail]>>

# 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/<https://urldefense.com/v3/__https:/datatracker.ietf.org/doc/statement-iesg-iesg-statement-normative-and-informative-references-20060419/__;!!NEt6yMaO-gk!FT5v1vDNJqxR59K-LDd3Xy3E9qVbJTv3aE7Q8DwGWQWB1NxWmfEfBMxSXimmB_mnlQxOt0De-aTY0Zc$>; 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/<https://urldefense.com/v3/__https:/datatracker.ietf.org/doc/statement-iesg-statement-on-clarifying-the-use-of-bcp-14-key-words/__;!!NEt6yMaO-gk!FT5v1vDNJqxR59K-LDd3Xy3E9qVbJTv3aE7Q8DwGWQWB1NxWmfEfBMxSXimmB_mnlQxOt0DeM8WK2Vw$>] 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<https://urldefense.com/v3/__https:/www.rfc-editor.org/rfc/rfc7322.html*section-4.8.2__;Iw!!NEt6yMaO-gk!FT5v1vDNJqxR59K-LDd3Xy3E9qVbJTv3aE7Q8DwGWQWB1NxWmfEfBMxSXimmB_mnlQxOt0DeYyo7Qic$>

- 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<https://urldefense.com/v3/__https:/www.iana.org/assignments/pcep/pcep.xhtml*sr-ero-flag-field__;Iw!!NEt6yMaO-gk!FT5v1vDNJqxR59K-LDd3Xy3E9qVbJTv3aE7Q8DwGWQWB1NxWmfEfBMxSXimmB_mnlQxOt0Dedbch614$> and https://www.iana.org/assignments/pcep/pcep.xhtml#srv6-ero-flag-field<https://urldefense.com/v3/__https:/www.iana.org/assignments/pcep/pcep.xhtml*srv6-ero-flag-field__;Iw!!NEt6yMaO-gk!FT5v1vDNJqxR59K-LDd3Xy3E9qVbJTv3aE7Q8DwGWQWB1NxWmfEfBMxSXimmB_mnlQxOt0DeEss2-54$> 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