[Pce] Review for draft-ietf-pce-sr-p2mp-policy

[Dhruv Dhody <[e-mail]>]

Hi Authors, WG,

Following up on draft-ietf-pce-sr-p2mp-policy; I reviewed it and also
had some back-and-forth with Andrew, who shepherds this I-D. Sharing a
consolidated set of comments that would be good to resolve before
WGLC.

## General

- Make sure to sync any late changes in the PIM draft/RFCs.

- Please respond to two early directorate reviews and confirm that
they are happy with the changes made in -14

- Does the scope of the I-D only SR-MPLS or does it include SRv6 —
this needs to be made explicit. Right now it reads a bit in-between.

  Either:
  - clearly scope to SR-MPLS only, or
  - fully support SRv6, including consistent text (not just “label”),
PST handling, CCI encoding, references, with some examples.

### RBNF

- The current RBNF reads more like an example of message instances
rather than a proper definition of protocol grammar. It should define
how the base PCEP grammar is extended, stand on its own, and remain
backward compatible with existing PCEP messages. As written, it
appears to illustrate SR-P2MP message structures rather than formally
specifying the protocol extension.

- Lets take PCRpt in
https://www.ietf.org/archive/id/draft-ietf-pce-sr-p2mp-policy-14.html#section-4.4.1
as an example. I suggest to update the RBNFas shown below; note that
this method remains backward compatible with the PCRpt message in RFC
8231 with <path>.

````
   The format of the PCRpt message as specified in [RFC8231] and
   extended by [RFC8697] is now further extended as follows:

      <PCRpt Message> ::= <Common Header>
                          <state-report-list>

   Where:

         <state-report-list> ::= <state-report>[<state-report-list>]

         <state-report> ::= [<SRP>]
                            <LSP>
                            [<association-list>]
                            (<path>|<sr-p2mp>)


   Where:

         <sr-p2mp>::=<ENDPOINT>

         <path> is as per RFC8231 and extended by other PCEP extensions.
         <association-list> is as per RFC 8697
````

- Having two RBNFs for same message (policy vs replication) is fine if
that’s the intent, but both should still “compile” and align with the
base grammar. I would write
https://www.ietf.org/archive/id/draft-ietf-pce-sr-p2mp-policy-14.html#section-4.4.2
as

````
   The format of the PCRpt message with
   [I-D.ietf-pce-pcep-extension-pce-controller-sr] as base is updated
   as follows:

         <PCRpt Message> ::= <Common Header>
                             <state-report-list>
      Where:

         <state-report-list> ::= <state-report>[<state-report-list>]

         <state-report> ::= (<lsp-state-report>|
                             <central-control-report>)

         <lsp-state-report> ::= [<SRP>]
                                <LSP>
                                <path>

         <central-control-report> ::= [<SRP>]
                                      <LSP>
                                      (<cci-list>|
                                      (<FEC>
                                      <CCI>)|
                                      (<CCI>
                                      <intended-path-multipath>))

       Where:

                <intended-path-multipath> is defined in
                [I-D.ietf-pce-multipath]
````

- This is important as incorrect or ambiguous RBNF impacts
interoperability and makes it unclear how implementations should
extend existing PCEP behavior.

### Error handling

- I think this needs a bit more explicit treatment.

- Even if we are relying on existing PCEP error handling, it would
help to say so clearly in the document and point to the relevant error
codes/reference to existing RFCs.

- Also, where we introduce new MUST conditions, it should be clear
what happens if those are violated on receipt. Examples
>PLSP-ID: value MUST be set to zero and will be assigned by PCC.

>Symbolic path name: generated by PCE, MUST be unique for each CP on PCC.

- "Association object MUST be present for CP PCUpdate and PCRpt
message. CCI object MUST be present in the Replication segment
updates." - As a message receiver, these MUST conditions are difficult
to verify. What would be the error handling?


### Normative language

- There are a few places where the draft restates existing PCEP
behavior using MUST/SHOULD, even though no new behavior is being
introduced or modified. This should be avoided.

- Some of the MUST conditions are also not easily verifiable from a
receiver’s point of view (e.g., presence of certain objects or
uniqueness constraints). This makes interoperability ambiguous as
expected behavior is undefined.

## Section-specific

### **§5.1**

- Length field looks incorrect — should be fixed.

### **§5.4 (symbolic name)**

- To make it clear, consider phrasing the text in this
manner(suggested by Andrew): “To help the name be unique on PCC, it is
RECOMMENDED the name contain the Root, Tree-ID and CP Discriminator
values.”

### **§5.5.1**
- The current text “This first PCRpt message does not have a
corresponding PCUpdate message but it does include the Association
object accordingly” is confusing. Suggest rewording along the lines
of:
  > "Note that, this first PCRpt message does not have an SRP object
corresponding to a PCUpd message."

### **§5.5.2**

- Please remove the figure — nothing new is being defined there.

### **§5.6.1**

- Please explicitly say the LSP object MUST include the new TLV for
SR-P2MP Policy.

### **§5.7.2 (CCI)**

- The text should be clearer that this is a new CCI object type, not a
redefinition of the existing one.

### **§A (Examples)**

- Packet-style examples are not commonly used in PCE WG documents ever
as they tend to become stale over time.

- Please consider something more structured (like in RFC8306), which
iseasier to read and maintain.

````
    PCRpt Message with
       SRP (ID=0, PST=1)
       LSP (PLSP-ID=1, Symbolic Path Name=A,Y,0)
       ENDPOINT (Leaf type=5, Source=A, Destination=D,E)
````

## Editorial

- A general editing pass would help.

- avoid restating existing PCEP behavior if nothing has changed.

- avoid phrases like “variation of” in a standards track I-D as that
does not help with interoperability

- use consistent naming (PCInitiate, PCRpt, etc.)

Thanks,
Dhruv