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
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.
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.
4. 3.5.6. Path Bandwidth Metric value
I would suggest to change this to "Automatic Bandwidth Metric"
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
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
Rgds
Shraddha
Juniper Business Use Only
From: Dhruv Dhody <d...@dhruvdhody.com>
Sent: 06 April 2025 23:05
To: Shraddha Hegde <shrad...@juniper.net>
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 <d...@dhruvdhody.com<mailto:d...@dhruvdhody.com>>
Date: Sun, Apr 6, 2025 at 10:28 PM
Subject: Shepherd review of draft-ietf-pce-sid-algo
To: <draft-ietf-pce-sid-a...@ietf.org<mailto:draft-ietf-pce-sid-a...@ietf.org>>
Cc: <pce@ietf.org<mailto:pce@ietf.org>>
# 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
_______________________________________________
Pce mailing list -- pce@ietf.org
To unsubscribe send an email to pce-le...@ietf.org
