[Idr] Re: Shepherd's review of draft-ietf-idr-bgp-ls-sr-policy-05

[Ketan Talaulikar <[e-mail]>]

Hi Sue,

Thanks a lot for your detailed review of this document. Apologies for the
delay in response.

We have also posted an update in line with the discussions below:
https://datatracker.ietf.org/doc/html/draft-ietf-idr-bgp-ls-sr-policy-06

Please check inline below for responses.


On Fri, Aug 30, 2024 at 2:16 AM Susan Hares <[e-mail]> wrote:

> Ketan, Jie, Stefano, Hannes, and Jeff:
>
>
>
> This shepherd review is in preparation for
> draft-ietf-idr-bgp-ls-sr-policy-05 being WG LC.
>
>
>
> Status: My understanding is that Ketan requested WG LC.
>

KT> Yes, this is correct.


>
>
> If you wish a WG LC of this draft during the time period 9/1 to 12/1,
>
>  the following steps need to occur:
>
>
>
>    1. Edits to address this shepherd report
>    2. Report on 2 implementations on the BGP wiki
>    3. A presentation at IDR SR interim (9/9/2024)
>
>
>
> Please let me know if you wish to have a WG LC during 9/1/2024 to
> 12/1/2024.
>
> I will add you to the list of presenters at the IDR SR interim on 9/9/2024.
>

KT> I was not able to present at that interim since I was unwell. The draft
has been stable since the last presentation and updates have been editorial
in nature. We can present to the WG at the IETF 121 if discussion is
required on any aspects of the drafts and if time permits.


>
>
> The shepherd’s review is below. I will post the review on the SR wiki as
> well.
>
>
>
> Cheerily, Sue Hares
>
>
>
> =================
>
>
>
> *Summary:*
>
>
>
> The technology in this specification appears to be consistent.
>
> However, the text seems to have several technical issues that need to
> corrected in an editing pass.
>
>
>
> Beyond the technical issues, there are editorial nits.
>
>
>
>
>
> *Technical issues: *
>
> Issue 1: Section 1, introduction
>
>
>
> Does this draft cover explicit Candidate Policy (CP) information in BGP-LS
> or dynamic and explicit CP information?
>
> Please specify whether this is appropriate for explicit, dynamic, or
> composite Candidate Paths.
>

KT> The draft covers explicit and dynamic CPs. Composite CPs are outside
the scope of this document. Now clarified this in the introduction section.


>
>
> Issue 2: Section 3 diagram:  “Node Descriptor TLV (for the Headend)” in
> the diagram in section 3 should be “Local Node Descriptor”
>

KT> Fixed.


>
>
> Issue 3: Section 4, *Flags – Why is “cleared” used for bits?  It would
> seem that “zero would be better”.
>
>
>
> Old text:/Other bits MUST be cleared by the originator and
>
>           and MUST be ignored by a receiver./
>
>
>
> New text:/Other bits MUST be cleared (set to zero)
>
>          by the originator and MUST be ignored
>
>           by a receiver./
>
>
>
> Note: You can simply indicate that you will be using “cleared”/”sets” for
> bits. I am looking for consistency across the document.
>

KT> We've used the term "clear" to mean set to 0 and "set" to mean set to 1
for individual bits throughout this document. We have not used "set to
zero" anywhere for individual bits. I believe there is consistency in this
regard - please let me know if something has been missed.


>
>
> Issue 4: Section 5.1 – Bit Definition (D-Flag, B-Flag, U-Flag, L-Flag, and
> F-flag) link to section in RFC9256 are unclear.
>

KT> Updated the text to refer to section 6.2 of RFC9256 instead of only the
RFC 9256 reference. This applies to all the bits.


>
>
> 4a) D-Flag:
>
>
>
> Draft text:/
>
>          D-Flag: Indicates the dataplane for the BSIDs and if they are
>
>          16 octet SRv6 SID when set and are 4 octet SR/MPLS label value
>
>          when clear./
>
>
>
> New text:/
>
>          D-Flag: Indicates the dataplane for the BSIDs. If D-Flag is set,
>
>          the BSID is 16 octet SRv6 SID. If D-Flag is clear, the BSID is
>
>          the 4-octet SR/MPLS label value.  [RFC9256, section 6.2]/
>
>
>
>
>
> Text in RFC9256, section 6.2, text:/
>
>   “When the active candidate path has a specified BSID,
>
>    the SR Policy uses that BSID if this value
>
>   (label in MPLS, IPv6 address in SRv6) is available.”/
>
>
>
> 4b) B-Flag
>
>  Draft text:/
>
>        B-Flag: Indicates the allocation of the value in the BSID field
>
>        when set and indicates that BSID is not allocated when clear./
>
>
>
>   Question: Does “B-Flag” set indicate a specified BSID-only case per
>
>   [RFC9256, section 6.2.3].  Does B-Flag clear, indicate unspecified
>
>   BSID (RFC9256, section 6.2.1)? Or just that the node has allocated (or
> not-allocated)
>
>   the BSID value.  The problem with this definition is the linkage to
>
>   RFC9252.
>

KT> It literally means what the text says - i.e. the node has allocated or
not-allocated the BSID value.


>
>
> 4c) U-Flag – Does this U-Flag link to the Unspecified BSID (RFC9256,
> section 6.2.1)?
>
>      Or is it just that the BSID is unavailable due to another cause?
>

KT> This also literally means what the text says "Indicates the specified
BSID value is unavailable when set". It is not related to the "unspecified
BSID" RFC9256 section 6.2.1.


>
>
> 4d) L-Flag – If this explanation references RFC9256 section 6.2, please
> add the section to the text.
>

KT> All flags reference section 6.2 of RFC 9256.


>
>
> RFC9256 text:/Optionally, instead of only checking that the BSID of the
> active path is available,
>
> a headend MAY check that it is available within the given SID range i.e.,
>
> Segment Routing Local Block (SRLB) as specified in [RFC8402]./
>
>
>
> 4e) F-Flag – The explanation is clear, but the link to RFC9256 is not
> clear.
>
>     Is this a reference to section 9 in RFC9256.
>

KT> The reference is still to the text in section 6.2 which describes cases
where the specified BSID is not available and the headend can fallback to
using one from the dynamic label range.


>
>
>
>
> Issue 5: Section 5.1 text on length of BSID fields
>
>
>
> Old text:/ The BSID fields above are 4-octet carrying the MPLS Label or
> 16-octet
>
>    carrying the SRv6 SID based on the BSID D-flag.  When carrying the
>
>    MPLS Label, as shown in the figure below, the TC, S, and TTL (total
>
>    of 12 bits) are RESERVED and MUST be set to 0 by the originator and
>
>    MUST be ignored by a receiver./
>
>
>
> New text:/ The BSDI fields above depend on the dataplane (SRv6 or MPLS)
>
>    indicated by the BSID D-Flag.  If D-Flag set (SRv6 dataplane), then
>
>    the length of the BSID fields is 16 octets.  If the D-Flag is clear
>
>    (MPLS dataplane), then the length of the BSDI Fields is 4 octets.
>
>    When carrying the MPLS Label, as shown in the figure below, the TC, S,
> and TTL (total
>
>    of 12 bits) are RESERVED and MUST be set to 0 by the originator and
>
>    MUST be ignored by a receiver./
>
>
>

KT> Thanks. I've incorporated this text.


>
>
> Issue 6: Section 5.2 – bit definitions link to RFC9256 is not clear.
>
>
>
>     The following text describing the “bit positions” does not give a
> clear and specific reference to sections in RFC9256:
>
>    Draft-text:/“The following bit
>
>      positions are defined and the semantics are described in detail
>
>      in [RFC9256]. /
>
>
>
>    See my comments on section 5.1 regarding my questions on each bit.
>

KT> Fixed same as for section 5.1.


>
>
>
>
> Issue 7: Section 5.2 – Text on which sub-TLVs can be used in the SR
> Binding SID TLV
>
>
>
>     Note: This is a technical issue because the unclear text could impact
> interoperability
>
>
>
>     Two points are being made in the following paragraph:
>
> 1.    SRv6 Endpoint Behavior TLV (1250) and SRv6 SID Structure TLV (1252)
> are defined in RFC9514
>
> 2.    These two sub TLVs may optionally be used in the SRv6 Binding SID
>
>
>
> Old text:/
>
>    The SRv6 Endpoint Behavior TLV (1250) and the SRv6 SID Structure TLV
>
>    (1252) defined in [RFC9514] are used as sub-TLVs of the SRv6 Binding
>
>    SID TLV to optionally indicate the SRv6 Endpoint behavior and SID
>
>    structure for the Binding SID value in the TLV./
>
> New text:/
>
>    The SRv6 Endpoint Behavior TLV (1250) and the SRv6 SID Structure TLV
>
>    (1252) MAY optionally be used as sub-TLVs of the SRv6 Binding SID TLV
>
>    to indicate the SRv6 Endpoint behavior and SID structure for the
>
>    Binding SID value in the TLV. [RFC9514] defines SRv6 Endpoint Behavior
> TLV
>
>    And SRv6 SID Structure TLV. /
>

KT> Incorporated your suggestion.


>
>
> Issue 8: Section 5.3 – Flag bits definitions
>
> •     S-Flag – Please define what it means when S-Flag is clear (zero),
> and give RFC9256 reference.
>
> •     A-Flag – Please define what it means when A-Flag is clear (zero),
> and give RFC9256 reference.
>
> •     E-Flag – Please define what it means when B-Flag is clear (zero),
> and give RFC9256 reference.
>
> •     V-Flag – Please give RFC9256 reference
>
> •     O-Flag – Please define what it means when O-Flag is clear (zero).
> [Here the RFC9256 reference is given.]
>
> •     D-Flag – Please define what it means when D-Flag is clear (zero),
> and give RFC9256 reference.
>
> •     C-Flag – Please define what it means when C-Flag is clear (zero),
> and give RFC9256 reference.
>
> •     I-Flag – Please define what it means when I-Flag is set (one) or
> cleared (zero).
>
> •     T-flag – Please define what it means when the T-Flag is clear (zero)
>
> •     U-Flag – Please define what it means when the U-Flag is clear
> (clear), and give RFC9256 reference.
>

KT> About the use of the term "clear and set" and "what happens when not
set". To me, the clear and set for a bit are well known operations and the
set indicates presence of a state or condition while clear indicates an
absence. Let me take an example: "Indicates the CP is in an administrative
shut state when set and not shut when clear". The explanation of the
"clear" part seems redundant and obvious. That said, there are places where
the clear state has also been described.


>
>
> Please give references to specific sections in RFC9256 since you state in
> the bit descriptions that:
>
> “The following bit positions are defined and the semantics are described
> in detail in [RFC9256].”
>

KT> The references to RFC9256 sections have been given where necessary -
e.g. where there is some special behavior described and we have a section
for it in the RFC 9256. The expectation is any implementor or reader of
this document is fully conversant with RFC9256 and the SR Policy
Architecture.


>
>
> Issue 9: Section 5.6 – Only Explicit CP and Dynamic CP discussion
>
>
>
> This section describes usage by:
>
> •     explicit candidate paths (tunnels), and
>
> •     dynamic candidate paths (on-demand tunnels).
>
> It does not discuss usage by a composite path.  Is it valid to apply to
> composite?
>

KT> Composite CP is outside the scope.


>
>
> Issue 10: Section 5.6 – Bit flags definitions
>
> •     P-Flag, U-Flag, A-Flag, T-Flag, S-flag, F-Flag, H-Flag – do not
> indicate what a cleared flag means.
>

KT> Same as a similar previous comment ...


> •     What does mutual exclusive mean between P-Flag and U-Flag.  (I think
> it means both cannot be set).
>

KT> Yes


>
>
> Issue 11: Section 5.6, What sub-TLVs can be included
>
>
>
> concern:  The text does not clearly state which sub-TLVs MAY be included.
>
>
>
> Draft-Text:/
>
> *  sub-TLVs: optional sub-TLVs MAY be included in this TLV to describe
> other constraints.
>
>    The following constraint sub-TLVs are defined for the SR CP Constraints
> TLV./
>
> New text:/
>
> *  sub-TLVs: A sequence of optional sub-TLVs MAY be included in this TLV
>
>              to describe other constraints. The optional sub-TLVs that can
> be
>
>               included are: SR Affinity Constraint, SR SRLG Constraint, SR
> Bandwidth
>
>               Constraint, SR Disjoint Group Constraint, SR Bidirectional
> Group Constraint,
>
>               and SR Metric Constraint. These constraint Sub-TLVs are
> defined below./
>

KT> Thanks. Incorporated your suggestion.


>
>
> Issue-12: Section 5.6.3, text description of SR Bandwidth Sub-TLv
>
>
>
> old text:/  Only a single instance of
>
>             this sub-TLV is advertised for a given CP./
>
>
>
> problem: I think you mean for a given CP Constraint TLV.
>
>          Since the link is unclear, I have added this as a technical
> issues.
>
>              I suspect the issues is mostly editorial.
>
>
>
> New text:/ Only  a single instance of
>
>            this sub-TLV is advertised for a given
>
>                CP Constraint TLV, and CP.  Recall that only one
>
>                CP constraint TLV is allowed per CP./
>

KT> This is editorial and this is already clear for each level of
TLV/sub-TLV.


>
>
> Issue-13: Section 5.6.4, clear of Request Flag Bits when cleared
>
>
>
> The descriptions of the request flags (S-Flag, N-Flag, L-Flag, F-Flag, and
> I-Flag)
>
> do specify a behavior when the bit is set.  What is the behavior when the
> bit is cleared?
>
> This needs to be described either in the introduction the request bits or
> on each bit.
>

KT> Same as previous response ...


>
>
> Issue-14, Section 5.6.4, clear meaning of Status Flag bits when cleared
>
>
>
> The descriptions of the status flags (S-Flag, N-Flag, L-Flag, F-Flag,
> I-Flag, and X-flag)
>
> do specify the status meaning when the bit is set.   What is the behavior
> when the bit is cleared?
>
> This needs to be described either in the introduction the status bits or
> on each bit.
>

KT> Same as previous response ...


>
>
> Issue-15: Section 5.6.5, length maximum
>
>
>
> The minimum length is clearly stated.  Is there a maximum length for this
> field?
>

KT> We cannot specify a maximum here since the size is coming from PCEP
protocol encoding which may be extended from time to time.


>
>
> Issue-16: Section 5.6.5, C-Flag, what does C-Flag mean if clear
>
>
>
> Current text:
>
>       -  C-Flag: Indicates that the bidirectional path is co-routed when
>
>          set/
>
>
>
> Problem: What does it mean when the C-Flag is clear?
>

KT> Same as the previous comment ...


>
>
> Issue-17: Section 5.6.6
>
>
>
> The SR Metric Constraint Sub-TLV is used for a dynamic path and an
> explicit path.
>
> Is this sub-TLV supported for a composite path?
>

KT> Composite path is out of scope


>
>
> Issue-18: Section 5.6.6, Metric-Type
>
>
>
> Are all other values except the values in section 8.6 invalid?
>

KT> Why would they be invalid? New metric types may be defined by other
documents and this encoding does not need to change.


>
>
> Issue-19: Section 5.6.6, Flags
>
>
>
> a) What is the meaning of the O-Flag when the bit is cleared?
>
> b) What is the meaning of the M-Flag when the bit is cleared?
>
> c) What is the meaning of the B-Flag when the bit is cleared?
>

KT> Same as previous comment


>
>
> Issue-20: Section 5.7, Flags clarity
>
>
>
> The link between RFC9256 and the Flag bits (D-Flag, E-Flag, C-Flag,
> V-Flag,
>
> R-Flag, F-Flag, A-Flag, T-Flag, M-Flag)is vague.  Please give a
>
> section reference per flag bit in the form (RFC9256, section x.x (or
> x.x.x)).
>

KT> The descriptions should be clear enough. Same as a previous response.


>
>
> Please indicate what action occurs when the following flags are clear:
> C-Flag,
>
> A-Flag, and T-Flag,
>

KT> Same as previous comment


>
>
> Issue-21: Section 5.7, Algorithm values
>
>
>
> Please indicate what values can be set in the Algorithm octet.
>

KT> Clarified.


>
>
> Issue-22: Section 5.7, Methodology paragraph clarity, ";" issue
>
>
>
> The ";" causes the methodology to be unclear.
>
>
>
>  old text:/
>
>   A SID-List may be empty in certain cases
>
>    (e.g. for a dynamic path) where the headend has not yet performed the
>
>    computation and hence not derived the segments required for the path;
>
>    in such cases, the SR Segment List TLV SHOULD NOT include any SR
>
>    Segment sub-TLVs. /
>
>
>
>   New text (suggested):
>
>     A SID-List may be empty in certain situations
>
>       (e.g. for a dynamic path) where the headend has not yet performed
>
>       the computation and hence not derived the segments required for the
> path.
>
>     In such cases where the SID-LIST is empty, the SR Segment List TLV
>
>       SHOULD NOT include any SR Segment sub-TLVs. /
>

KT> Thanks for your suggestion. Incorporated it.


>
>
> Issue-23, Section 5.8, Segment type
>
>
>
>   Other proposed specifications give other segment types.
>
>   The following text does not take this into consideration:
>
>
>
>  Old text:/
>
>    *  Segment Type: 1 octet which indicates the type of segment (refer
>
>       Section 5.8.1 for details)/
>
>
>
> A suggestion for the new text is:
>
> New text:/
>
>    *  Segment Type: 1 octet which indicates the type of segment. Initial
>
>       values are specified by this document (See Section 5.8.1 for
> details).
>
>         Additional segment types are possible, but out of scope for this
> document./
>

KT> Thanks for your suggestion. Incorporated it.


>
>
> Issue-24, Section 5.8, Flag bits
>
>
>
> The link between RFC9256 and the flag bits is unclear for S-Flag, E-Flag,
> V-Flag,
>
> R-Flag, and A-Flag.  Please give a reference for a section in RFC9256 for
> each bit type.
>
>
>

KT> Same as a previous comment.


> V-Flag grammar makes the meaning unclear.
>
>
>
> Old text:/
>
>       -  V-Flag: Indicates the SID has passed verification or did not
>
>          require verification when set and failed verification when
>
>          clear./
>
>
>
> Suggested new text:/
>
>       -  V-Flag: Indicates the SID has passed verification or did not
>
>          require verification when set.  When V-Flag is cleared, it
>
>          indicates the SID has failed verification. /
>
>
>
> R-Flag and A-Flag might als benefit from a similar rewrite.
>

KT> Ack. Changed accordingly.


>
>
> Issue-25: Section 5.8, methodology paragraph at end
>
>
>
> Text:/
>
>    The SRv6 Endpoint Behavior TLV (1250) and the SRv6 SID Structure TLV
>
>    (1252) defined in [RFC9514] are used as sub-sub-TLVs of the SR
>
>    Segment sub-TLV to optionally indicate the SRv6 Endpoint behavior and
>
>    SID structure when advertising the SRv6 specific segment types./
>
>
>
> problem: Too much compression of meaning as method is squished into 1
> sentence.
>
>
>
> alternative text:/
>
>    The SRv6 Endpoint Behavior TLV (1250) and the SRv6 SID Structure TLV
>
>    (1252) defined in [RFC9514] are used as sub-sub-TLVs of the SR
>
>    Segment sub-TLV.  These twoi sub-sub-TLVs are used to optionally
>
>    indicate the SRv6 Endpoint behavior and SID structure when
>
>    advertising the SRv6 specific segment types./
>

KT> Updated accordingly.


>
>
> Issue-26: Section 5.8.1.1, 5.8.1.2, 5.8.1.3, and 5.8.1.4
>
>
>
> Algorithm: What are the valid values for Algorithm.
>
> Is there a Minimum and maximum value for the algorithm?
>

KT> Provided pointer to the IANA registry for where Algorithm values are
taken.


>
>
> Issue-27: Section 5.8.1.3, 5.8.1.4, 5.8.1.5, 5.8.1.6, 5.8.1.7
>
>
>
> Are there any invalid IPv4 Node addresses or IPv6 Node addresses.
>
> If so, please indicate this in the text.
>

KT> We are not doing validation of this information in BGP-LS.


>
>
> Issue-28: Section 5.8.1.6, 5.8.1.7, 5.8.1.10
>
>
>
> Please indicate why point-to-point link might not have a remote address or
> interface ID.
>

KT> The text does not say "point-to-point link might not have a remote
address" - it says it is not necessary to provide a remote address as well
as a local address to identify the link.


>
>
> Issue-29: Section 5.8.1.8, 5.8.1.9, 5.8.1.1
>
>
>
> Are there any invalid Global IPv6 addresses? If so, please indicate what
> addresses are invalid.
>

KT> We are not doing validation of this information in BGP-LS.


>
>
> Issue-30: Section 5.9, Metric type
>
>
>
> Metric type:  Are values not listed in Section 8.6 valid?
>
> I believe you are allowing additional values in the future, but
>
> those values are outside the scope of this work.
>

KT> Yes, they are valid. They may be defined in the future.


>
>
> Issue-31: Section 5.9, Flags
>
>
>
> What is the meaning of the following flags when they are not set:
>
> M-Flag, A-Flag, B-Flag, and V-Flag.  Please provide this information.
>

KT> Same as a previous comment.


>
>
> Issue-32: Section 6, Does this text apply to all protocol origins?
>
>
>
> Current text:/ Then
>
>    the SR Policy Candidate Path's state and attributes are encoded in
>
>    the BGP-LS Attribute field as SR Policy State TLVs and sub-TLVs as
>
>    described in Section 5.  The SR Candidate Path State TLV as defined
>
>    in Section 5.3 is included to report the state of the CP.  The SR
>
>    BSID TLV as defined in Section 5.1 or Section 5.2 is included to
>
>    report the BSID of the CP when one is either specified or allocated
>
>    by the headend.  The constraints and the optimization metric for the
>
>    SR Policy Candidate Path are reported using the SR Candidate Path
>
>    Constraints TLV and its sub-TLVs as described in Section 5.6.  The SR
>
>    Segment List TLV is included for each of the SID-List(s) associated
>
>    with the CP.  Each SR Segment List TLV in turn includes SR Segment
>
>    sub-TLV(s) to report the segment(s) and their status.  The SR Segment
>
>    List Metric sub-TLV is used to report the metric values at an
>
>    individual SID List level./
>
>
>
> This text come after the PCEP discussion, so it is unclear if this applies
>
> to information originated by all BGP-LS producers (see table 8.4).
>

KT> Thanks for catching that. I've replaced "Then the" with "The" since
this text is for all origins.


>
>
>
>
>
>
>
>
> ==============================
>
> Editorial comments:
>
>
>
> Nit-1.      Section 1, paragraph 1, English language usage of “;”
>
>
>
> Old text:/
>
>    Each CP in turn may have one or
>
>    more SID-List of which one or more may be active; when multiple are
>
>    active then traffic is load balanced over them. This document covers
>
>    the advertisement of state information at the individual SR Policy CP
>
>    level./
>
>
>
> New text:/
>
>    Each CP may have one or more SID-List and one or
>
>    more of these SID-LIST may be active when traffic is
>
>    load-balanced over them. This document covers
>
>     the advertisement of state information at the individual SR Policy CP
>
>    level via BGP [RFC9552].
>

KT> I've clarified but with different text.



> ----
>
> Nit-2.      Section 1, paragraph 2, unclear sentence
>
>
>
> Old text:/
>
>    SR Policies are generally instantiated at the head-end and are based
>
>    on either local configuration or controller-based programming of the
>
>    node using various APIs and protocols, e.g., PCEP or BGP./
>
>
>
> New text:/
>
>    SR Policies are generally instantiated at the head-end and are based
>
>    on either local configuration or controller-based programming of the
>
>    node using various APIs and protocols (e.g., PCEP or BGP). /
>

KT> Fixed.


>
>
> ----
>
> Nit-3.      Section 1, paragraph 3 run-on sentence, broken into two
> statements.
>
>
>
> Old text:/
>
>    In many network environments, the configuration, and state of each SR
>
>    Policy that is available in the network is required by a controller
>
>    which allows the network operator to optimize several functions and
>
>    operations through the use of a controller aware of both topology and
>
>    state information./
>
>
>
> New text:/
>
>    In many network environments,  the network operator uses a controller
>
>    to optimize operations in the network.  The controller needs information
>
>    regarding the configuration and the state of each available SR Policy
>
>    to calculate the optimized topologies.  A description of five
> controllers
>
>    that can benefit from using BGP [RFC9552] to collect SR Policy state is
> given below.
>
>    (management-based PCE, composite PCE, parent-child PCE deployments,
>
>     centralized controller, and  NMS visualization of SR Policy
> (tunnels)). /
>

KT> Rephrased existing text for clarity without adding "a description of
five controllers ...".


>
>
> Nit-4.      Section 1, paragraph 4, plural/singular issues and clarity of
> text
>
>
>
> Old text:/
>
>    One example of a controller is the stateful Path Computation Element
>
>    (PCE) [RFC8231], which could provide benefits in path optimization.
>
>    While some extensions are proposed in the Path Computation Element
>
>    Communication Protocol (PCEP) for the Path Computation Clients (PCCs)
>
>    to report the LSP states to the PCE, this mechanism may not be
>
>    applicable in a management-based PCE architecture as specified in
>
>    section 5.5 of [RFC4655]./
>
>
>
> New text:/
>
>    One example of a controller is the stateful Path Computation Element
>
>    (PCE) [RFC8231].  The stateful PCE controller could be useful in
> calculating
>
>    optimized paths if Path Computation Clients (PCCs) use PCE
> Communication
>
>    Protocol (PCEP) to report LSP states to the PCE.  However, this
> mechanism
>
>    may not be applicable in a management-based PCE architecture as
> specified in
>
>    section 5.5 of [RFC4655]./
>

KT> I see no issue in the old text.


>
>
> Old text:/
>
>   This document proposes using the BGP-LS protocol [RFC9552] to collect SR
> Policy state
>
>   In a mechanism complementary to the mechanism defined in [RFC8231]./
>
>
>
> New text:/
>
>   This document proposes SR Policy state collection
>
>    mechanism complementary to the mechanism defined in [RFC8231]./
>

KT> I see no issue in the old text.


>
>
>
>
> Nit-5.      Section 1, paragraph 5
>
> Old text:/ An external
>
>    component may also need to collect the SR Policy information from all
>
>    the PCEs in the network to obtain a global view of the SR Policy
>
>    paths' state in the network./
>
>
>
>    New text:/  An external
>
>    component may also need to collect the SR Policy information from all
>
>    the PCEs in the network to obtain a global view of the state of all SR
> Policy
>
>    paths (tunnels) in the network./
>

KT> Rephrased.


>
>
>
>
> Nit-6.      Section 3, diagram
>
>
>
>
>
> Current diagram:/
>
>       0                   1                   2                   3
>
>       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
>
>      +-+-+-+-+-+-+-+-+
>
>      |  Protocol-ID  |
>
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
>      |                        Identifier                             |
>
>      |                        (64 bits)                              |
>
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
>      //            Node Descriptor TLV (for the Headend)            //
>
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
>      //           SR Policy Candidate Path Descriptor TLV           //
>
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
>
>
> New diagram:
>
>       0                   1                   2                   3
>
>       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
>
>      +-+-+-+-+-+-+-+-+
>
>      |  Protocol-ID  |
>
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
>      |                        Identifier                             |
>
>      |                        (64 bits)                              |
>
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
>      //      Local Node Descriptor TLV (for the Headend)            //
>
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
>      //           SR Policy Candidate Path Descriptor TLV           //
>
>      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
>
>

KT> Fixed


>
>
>
>
>
>
> Nit-7. Section 5.6,  repeat the abbreviation (CP) before reusing it in
> this section.
>
>
>
> Why: The refresh of the CP abbreviation provides easier reading.
>
> Otherwise, the reader must go back and find where CP was defined.
>

>
> Old text: / The SR Candidate Path Constraints TLV is an optional TLV that
> is used
>
>    to report the constraints associated with the candidate path./
>
>
>
> New text:/ The SR Candidate Path Constraints TLV is an optional TLV that
> is used
>
>    to report the constraints associated with the candidate path (CP)./
>
>
KT> If the reader is not aware of "CP" until they have come to this point
in the document then that indicates they are not familiar with the very
base spec that this document depends on.


>
>
> Nit-8. Section 5.6.3, lack of clarity of length of SRLG value.
>
>
>
> old text:/
>
>   *  SRLG Values: One or more SRLG values (each of 4 octets)./
>
>
>
> new text:/
>
> *SRLG VAlues: One or more SRLG values.  Each SRLG value is 4 octets.
>
> /
>

KT> Fixed.


>
>
> NIT-8, Section 5.6.4, "a" candidate path instead of "the" candidate path
>
>
>
> Old text:/
>
>    The SR Disjoint Group Constraint sub-TLV is an optional sub-TLV of
>
>    the SR CP Constraints TLV that is used to carry the disjointness
>
>    constraint associated with the candidate path. /
>
> New text:/
>
>     The SR Disjoint Group Constraint sub-TLV is an optional sub-TLV of
>
>    the SR CP Constraints TLV that is used to carry the disjointness
>
>    constraint associated with a candidate path. /
>
>
>

KT> I believe the use of "the" is correct since the reference is to the
specific CP that is being advertised


>  NIT-9, Section 5.6.5, clarity of R-Flag description
>
>
>
>  Old text:/
>
>       -  R-Flag: Indicates that this CP of the SR Policy forms the
>
>          reverse path when set and otherwise it is the forward path when
>
>          clear/
>
>
>
>  New text:/
>
>       -  R-Flag: Indicates that this CP of the SR Policy forms the
>
>          reverse path when the R-Flag is set.  If the R-Flag is clear,
>
>          this CP forms the forward path./
>

KT> Incorporated this suggestion.


>
>
> NIT-10: Section 6, sentence clarity can be improved by breaking long
> sentence into two sentences.
>
>
>
> Original text:/
>
>    For the reporting of SR Policy Candidate Paths, the NLRI descriptor
>
>    TLV as specified in Section 4 is used.  An SR Policy candidate path
>
>    (CP) may be instantiated on the headend node via a local
>
>    configuration, PCEP, or BGP SR Policy signaling and this is indicated
>
>    via the SR Protocol Origin.  When a PCE node is the BGP-LS Producer,
>
>    it uses the "reported via PCE" variants of the SR Protocol Origin so
>
>    as to distinguish them from advertisements by headend nodes.  Then
>
>    the SR Policy Candidate Path's state and attributes are encoded in
>
>    the BGP-LS Attribute field as SR Policy State TLVs and sub-TLVs as
>
>    described in Section 5.
>
>
>
> new text:/
>
>     An SR Policy candidate path
>
>    (CP) may be instantiated on the headend node via a local
>
>    configuration, PCEP, or BGP SR Policy signaling.  The protocol
>
>    that instantiates the SR Policy candidate path is indicated
>
>    via the SR Protocol Origin.
>
>
>
>    When a PCE node is the BGP-LS Producer, the PCE node uses the
>
>    "reported via PCE" variants of the SR Protocol Origin to
>
>    distinguish them from advertisements by headend nodes.
>
>    These "report via PCE" variants include "PCEP reported via PCE/PCEP"
> (10),
>
>    "BGP SR Plicy reported via PCE/PCEP" (20), Configuration (CLI, Yang
> Model
>
>     via NETCONF, etc.) reported via PCE/PCEP).    Then
>
>    the SR Policy Candidate Path's state and attributes are encoded in
>
>    the BGP-LS Attribute field as SR Policy State TLVs and sub-TLVs as
>
>    described in Section 5. /
>

KT> I think the original text is clear as well.


>
>
>  NIT-11: Security considerations, paragraph 3, unclear use of ";"
>
>  Original text:/  BGP peerings are
>
>    not automatic and require configuration; thus, it is the
>
>    responsibility of the network operator to ensure that only trusted
>
>    nodes (that include both routers and controller applications) within
>
>    the SR domain are configured to receive such information./
>
>
>
>  Suggested revision:/ BGP peer connections are
>
>    not automatic and require configuration. Thus, it is the
>
>    responsibility of the network operator to ensure that only trusted
>
>    nodes (that include both routers and controller applications) within
>
>    the SR domain are configured to receive such information./
>
>
>

KT> Fixed.

Thanks,
Ketan


>
>
>
>