Carrying SR-Algorithm in Path Computation Element Communication Protocol (PCEP)
draft-ietf-pce-sid-algo-29

Thank you for the secdir review by Alexey Melnikov.

Section 9:  RFC 8253 is outdated because of the publication of TLS1.3 (RFC8446). Consider listing BCP 195 vice RFC 9325 to ensure the most recent guidance for the implementation of TLS.

Thanks for the work done in this document and for addressing my previous blocking DISCUSS
(https://mailarchive.ietf.org/arch/msg/pce/ayCXnZU4FrhAm9yPH-AbJ3qilYw/ )

The COMMENTS below are non blocking but should be considered though.

## COMMENTS (non-blocking)


### Sections 4.1.1 and 4.1.2

Strongly suggest to add the position of the S flag rather than waiting to reach the IANA considerations section.

### Section 4.2.1

Please add an informative URI reference to the IANA registry.

### Sections 6.2 and 6.4

When can the "SHOULD" be bypassed ? or what are the consequences of doing so.

### Update references

As indicated by idnits, several I-Ds are now RFC (e.g., RFC 9843) even before the submission date of this I-D, so refresh these references.

Thank you for completing this work.

My review did not raise any transport-related concerns.

Gorry

# Thank you for responding and resolving the open DISCUSS observations. I am considering the discussion resolved with the pending v26 of the draft.
https://mailarchive.ietf.org/arch/msg/pce/HK56k2rcwXNMGC-ewsW6EtPVkHI/

# All comments were addressed and answered https://mailarchive.ietf.org/arch/msg/pce/DdEUUopiVdlybs58uKhjq9lBlPs/

Thank you for the swift and smooth follow up.

"Abstract", paragraph 3
>    This document specifies extensions to the Path Computation Element
>    Communication Protocol (PCEP) to enhance support for Segment Routing
>    (SR) with a focus on the use of Segment Identifiers (SIDs) and SR-
>    Algorithms in Traffic Engineering (TE).  The SR-Algorithm associated
>    with a SID defines the path computation algorithm used by Interior
>    Gateway Protocols (IGPs).  This document introduces extensions in
>    three main areas.
> 
>    Mechanisms for informing PCEP peers about the SR-Algorithm associated
>    with SIDs by encoding this information in Explicit Route Object (ERO)
>    and Record Route Object (RRO) subobjects.  This document updates RFC
>    8664 and RFC 9603 to allow such extension.
> 
>    The document specifies SR-Algorithm constraint, enabling refined path
>    computations that can leverage IGP algorithm logic, including
>    Flexible Algorithms, and their associated constraints and
>    optimization metrics.
> 
>    It defines new metric types for the METRIC object required to support
>    SR-Algorithm based path computation, but also applicable to Label
>    Switched Paths (LSPs) setup using different Path Setup Types.

The Abstract is long and most of the text is duplicated in Introduction. Suggest shortening the Abstract to the first paragraph and removing the last statement, and deferring the rest of the details to the Introduction Section as has already been done.

Section 4.4, paragraph 9
>       *  S (Strict): If set, the path computation at the PCE MUST fail
>          if the specified SR-Algorithm constraint cannot be satisfied.
>          If unset, the PCE MUST try to compute the path with SR-
>          algorithm constraint specified.  If the path computation using
>          the specified SR-Algorithm constraint fails, the PCE MUST try
>          to compute a path that does not satisfy the constraint.

In general, I support Gunter's DISCUSS. This section and this paragraph in particular was cited by OPSDIR review done by Nagendra (thanks Nagendra).

Section 6, paragraph 0
>    All manageability requirements and considerations listed in
>    [RFC5440], [RFC8231], [RFC8281], [RFC8664] and [RFC9603] apply to
>    PCEP extensions defined in this document.  In addition, the
>    requirements and considerations listed in this section apply.

Thanks for adding a section of manageability and for identifying additional parameters that need to be defined in the YANG model. In there a plan to update that model?

-------------------------------------------------------------------------------
NIT
-------------------------------------------------------------------------------

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

Document references draft-ietf-lsr-flex-algo-bw-con, but that has been
published as RFC9843.

Document references draft-ietf-pce-pcep-yang, but that has been published as
RFC9826.

Section 3, paragraph 3
> implicit algorithm of BSID is independent from SR algorithm used for the SR 
>                               ^^^^^^^^^^^^^^^^
The usual collocation for "independent" is "of", not "from". Did you mean
"independent of"?

Section 4.4, paragraph 1
> nded along the way. * A network comprises of a set of N links {Li, (i=1...N)
>                                 ^^^^^^^^^^^^
Did you mean "comprises" or "consists of"?

Section 4.4, paragraph 9
> ion that observes the worst (i.e., highest value) delay metric among all dest
>                                    ^^^^^^^
A determiner may be missing.

Section 5.2, paragraph 8
> -con], a PCE implementation may need support these IGP extensions to allow u
>                                 ^^^^^^^^^^^^
The verb "support" needs to be in the to-infinitive form.

Section 10.5, paragraph 1
> EPS: Usage of TLS to Provide a Secure Transport for the Path Computation Ele
>                              ^^^^^^^^^^^^^^^^^^
Uncountable nouns are usually not used with an indefinite article. Use simply
"Secure Transport".

Thanks for a well-sourced terminology section. Many of these terms are abbreviations, and I would encourage you to include the expanded term here (with the abbreviation in parentheses) alongside the reference to the RFC that defines them. Also consider moving FAD and the RFC9050 reference to the top with the same format as the other RFCs where terms are imported. I also noted the terms "headend router" and "RRO" used in the document without definitions here.

There appear to be too many words here:

415	   *  If no SEBF (including A flag defined in this document) is set, the
416	      Length value MUST match the requirements as defined in
417	      Section 5.2.1 of [RFC8664] applies.

I think this should be either "MUST follow the requirements defined in Section 5.2.1 of [RFC8664]." or "If no SEBF (...) is set, the requirements in Section 5.2.1 of [RFC8664] apply."

Throughout the document, the following sets of terms are used inconsistently. Please pick one for each and use it throughout.
- 'A flag', 'the A flag', 'the A-flag', 'the A bit', '"A" flag'
- "Subobject Extension Block", "the Subobject Extension Block"
- "IEEE floating-point format", "floating point format" "IEEE floating point"
- "Flexible Algorithm", "Flex-algo", "Flex-Algorithm"

In 4.2.1 and 4.2.2, normative requirements are placed on future extensions. That doesn't really affect implementers of this specification, and they can't be enforced on those future drafts. Instead, reframe this as guidance to those future draft authors and/or as affirmative statements about what can be done in the future. If there is behavior required now to enable those future extensions (e.g. "if an SEBF is set that you don't understand, fail"), normative requirements for those would be appropriate here.

In Sections 4.5.2 and 4.5.3, please include the appropriate reference to the definition of the floating point format (as you did in 4.5.1) or consider making this a definition used in the Terminology section.

The reference to "Section 5.1, Section 5.1.2, and Section 5.2" is awkward, especially as 5.1.2 is within 5.1. Consider making this "Sections 5.1 and 5.2" or simply "defined in this document" / "defined in this section". Are there multiple extensions here, or a single extension that involves multiple new elements?

In Section 5.2.2, should "The PCE must optimize" be "The PCE MUST optimize" or "The PCE optimizes"?

===NITS FOLLOW===
Section 3, "the mechanisms" => "mechanisms"
Section 5.2, "different SR-Algorithm constraint" => "different SR-Algorithm constraints" or "a different SR-Algorithm constraint"; "use empty ERO" => "use an empty ERO"?
Section 5.2.2, "introduced IANA registry" => "created an IANA registry"
Section 5.3, "new metric types defined in this document." => "the new metric types defined in this document." or simply "the metric types defined in this document."
Section 6, "to PCEP extensions defined in this document" => "to the PCEP extensions defined in this document" and similar throughout the section
Section 6.2, "for PCEP peer" => "for the PCEP peer"
Section 6.2, several of these section references would ideally be cross-references rather than bare section numbers

Hi Samuel, Zoey, Shaofu, Shuping, and Andrew,

Thank you for the effort put into this well-written and clear specification.

Thanks to Nagendra Nainar for the OPSDIR and to the authors for considering these in -14.

I only have minor comments: 

# Unassigend vs. Reserved

I suggest to make this change (and also in the figure and similar uses in the spec):

OLD:
   Reserved (24 bits): This field is reserved for future use and MUST be
   set to zero when sending and ignored when receiving, unless redefined
   by a future extension that is indicated by an associated SEBF and
   capability.

NEW:
   Unassigned (24 bits): This field is reserved for future use and MUST be
   set to zero when sending and ignored when receiving, unless redefined
   by a future extension that is indicated by an associated SEBF and
   capability.

to be consistent with the definitions provided in RFC8126:

      Unassigned:  Not currently assigned, and available for assignment
            via documented procedures.  While it's generally clear that
            any values that are not registered are unassigned and
            available for assignment, it is sometimes useful to
            explicitly specify that situation.  Note that this is
            distinctly different from "Reserved".

      Reserved:  Not assigned and not available for assignment.
            Reserved values are held for special uses, such as to extend
            the namespace when it becomes exhausted.  "Reserved" is also
            sometimes used to designate values that had been assigned
            but are no longer in use, keeping them set aside as long as
            other unassigned values are available.  Note that this is
            distinctly different from "Unassigned".

# Default value

CURRENT
6.1.  Control of Function and Policy

   A PCE or PCC implementation MAY allow the capability of supporting
   PCEP extensions introduced in this document to be enabled or disabled
   as part of the global configuration.

Can we recommend a default here? 

Having a default value would ensure a consistent behavior when the feature is introduced into a network.

# Inappropriate use of normative language

Section 6.4

OLD:
   An implementation SHOULD also allow the operator to view FADs, which
   MAY be used in Flexible Algorithm path computation defined in
   Section 5.2.2.

NEW:
   An implementation SHOULD also allow the operator to view FADs, which
   may be used in Flexible Algorithm path computation defined in
   Section 5.2.2.

# nits

## 1

(1)

OLD: The PCEP extensions specified in this document:

NEW: The PCEP extensions specified in this document are as follows:

(2)

CURRENT: 
  Signaling SR-Algorithm in ERO and RRO:  Mechanisms are introduced for 
  ..
  SR-Algorithm Constraint for Path Computation:  Mechanisms are defined 
  ..
  Extensions to METRIC Object:  Several new metric types are introduced

Maybe point to the section where these mechanisms are introduced.


## 4.5.2

OLD: The Section 4 of [I-D.ietf-lsr-flex-algo-bw-con]

NEW: Section 4 of [I-D.ietf-lsr-flex-algo-bw-con]

## 4.5.3

OLD: The Section 2 of [I-D.ietf-lsr-flex-algo-bw-con]

NEW: Section 2 of [I-D.ietf-lsr-flex-algo-bw-con]


OLD: The proposed metric type range was chosen
         ^^^^^^^^

NEW: The metric type range was chosen

Cheers,
Med