Segment Routing Point-to-Multipoint Policy
draft-ietf-pim-sr-p2mp-policy-22

Thanks to Corey Bonnell for their secdir review.

# Éric Vyncke, INT AD, comments for draft-ietf-pim-sr-p2mp-policy-16
CC @evyncke

Thank you for the work put into this document.

Please find below some non-blocking COMMENT points/nits (replies would be appreciated even if only for my own education).

Special thanks to Mike McBride for the shepherd's detailed write-up including the WG consensus *but it lacks* the justification of the intended status.

I hope that this review helps to improve the document,

Regards,

-éric

## COMMENTS (non-blocking)

### Section 1

Please expand "P2MP" in the introduction as well as the abstract is stand-alone.

The "bud" considerations should probably be in the terminology section.

Unsure whether a mix of SR-MPLS & SRv6 is specified here as the following sentence is a little ambiguous `enabling efficient packet replication within an SR domain.`.

### Section 1.1

In `construct a P2MP Tree instances` please use singular or plural form ;-)

### Section 2.1

Should there be a reference for `Color of SR Policy identifier` ?

Assuming that P2MP is mainly for multicast traffic, I am a little surprised not to see the mcast group in the tuple. But, I may have missed the point of P2MP.

Also, why using `tuple` rather than "pair" (this is cosmetic though).

### Section 3.3

As this is a SR-MPLS specific section, should there be a SRv6 specific section as well ?

### Section 4.1

What is `SRLG` ? Please expand and perhaps add an informative reference.

### Section 4.5.1

Should there be informative references for the protection mechanisms ?

### Section 6

Unsure whether the paragraphs after the first one are useful.

### Section 9.2

While not critical, it is highly unusual to refer to an individual expired draft such as draft-filsfils-spring-srv6-net-pgm-illustration (especially when used in the appendix).

### Appendix A

Please expand `PSP` and `USD` (plus add references ?).

To make a much nicer HTML rendering, suggest using the aasvg too to generate SVG graphics. It is worth a try especially if the I-D uses the Kramdown file format ;-)

I did not find any transport-specific concerns in my review of this document.

Thank you for the TSV-ART review by David Black, who noticed one 
discrepancy that I think could usefully be addressed:

"- Section 2 says: "An SR P2MP policy is a specialized form of an SR policy as
defined in[RFC9256] ..." - Section 2.1 says: "A SR P2MP Policy is uniquely
identified by the tuple <Root, Tree-ID>, where: ..." - RFC 9256 Section 2.1
says: "An SR Policy MUST be identified through the tuple <Headend, Color,
Endpoint>."

I don't understand how <Root, Tree-ID> is a "specialized form of" <Headend,
Color, Endpoint> that satisfies the RFC 9256 "MUST" requirement quoted above.
In particular, Color appears to be missing from <Root, Tree-ID>. I'm reading
"specialized form of" as implying a "subtype of" relationship, which may be
more restrictive than what was intended.

I suggest adding a paragraph to the end of Section 2.1 (SR P2MP Policy
Identification) that explains the relationship between those two types of
identification tuples and how policies are uniquely identified in an
environment that uses both RFC 9256 SR Policies and this draft's SR PMP
Policies."

Section 2.3, paragraph 0
>    An SR P2MP Policy has one or more CPs.  Identification of a CP in
>    context of the P2MP Policy is as specified in Section 2.9 of
>    [RFC9256].  A CP may include topological and/or resource constraints
>    and optimization objectives which influence the computation of P2MP
>    tree.  The Root node selects the active Candidate Path based on the
>    tie breaking rules defined in[RFC9256].

Is it that the policy has one or more CPs, or that the policy defines/discovers/computes one or more CPs and it is the SR network that has one or more CPs?

Section 2.3, paragraph 0
>    The Replication segments used to instantiate a P2MP tree instance are
>    identified by the tuple: <Root, Tree-ID, Instance-ID, Node-ID>, where
>    Root, Tree-ID of SR P2MP Policy and Instance-ID of the instance map
>    to Replication-ID of Replication segment and Node-ID is as defined in
>    [RFC9524].

Can the definition of Instance-ID and Node-ID be called out along with Root and Tree-ID in a Terminology section instead of scattering them in the document?

-------------------------------------------------------------------------------
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.

Section 3.3, paragraph 1
>    A P2MP tree can associated with one or more multi-point services on
>    the Root and Leaf nodes.  In SR-MPLS deployments, if it is known a
>    priori that multi-point services mapped to a SR-MPLS P2MP tree can be
>    uniquely identified within the SR domain, a controller MAY opt not to
>    instantiate Replication segments at Leaf nodes.  In such cases,
>    Replication Nodes upstream of the Leaf nodes effectively implement
>    Penultimate-Hop Popping (PHP) behavior by removing the Tree-SID from
>    the packet before forwarding it.  A multi-point service context
>    allocated from an upstream assigned label or Domain-wide Common Block
>    (DCB), as specified in [RFC9573], is an example of a globally unique
>    context that facilitates this optimization.

s/tree can associated/tree can be associated/

Section 4, paragraph 0
>    A controller is provisioned with SR P2MP Policy and its Candidate
>    Paths to compute and instantiate P2MP trees in an SR domain.  Once
>    computed, the controller instantiates the Replication segments that
>    compose the P2MP tree in the SR domain nodes using signalling
>    protocols such as PCEP, BGP, NetConf, etc.  The procedures for
>    provisioning a controller and the instantiation of Replication
>    segments in an SR domain are outside the scope of this document.

The protocol is NETCONF, the WG is NetConf, therefore in this case it better to say NETCONF. Same comment applies to Section 4.4.

"Appendix A.", paragraph 12
> sing N-SID6, steers packet via IGP shortest path to that node. Replication to
>                                    ^^^^^^^^
A determiner may be missing.

"Appendix A.", paragraph 13
> ing N-SID7, steers packet via IGP shortest path to R7 via either R5 or R4 ba
>                                   ^^^^^^^^
A determiner may be missing.

"A.1.1.", paragraph 6
> ation to R6, steers packet via IGP shortest path to that node. Replication to
>                                    ^^^^^^^^
A determiner may be missing.

In both the abstract and the introduction, it is unclear whether P2MP was an existing concept that this document built on, or a new concept being defined by this document. The first paragraph reads as if it's providing the necessary context for what this document does, but then the second paragraph states that it defines... much of the stuff that was just said? Consider moving "This document specifies..." earlier in the abstract or adjusting scope to make it clear which elements already exist. For example, "RFC 9524 defines a mechanism for one Segment Routing node to distribute traffic to multiple other nodes, called a Replication segment. Using multiple layers of Replication segments can enable [better scale, etc.], but requires centralized coordination of these Replication segments. This document defines a mechanism to perform this coordination and distribute the resulting configuration." Similarly, the introduction would benefit from an explanation of the current state of things, in what situations that state is suboptimal, and how this new element improves the situation.

Thank you for expanding CP into Candidate Path on its first use. Also consider mentioning the abbreviation at the definition of Candidate Path in the Terminology section.

The document is inconsistent about whether it's "a" SR Policy (if SR is pronounced "segment routing") or "an" SR Policy (if SR is pronounced "ess arr"). They're about evenly split right now -- please pick one. (For what it's worth, RFC9524 uses "an" throughout.)

In Section 3.2, I'm unclear what "a shared Replication segment MUST NOT be associated with an SR P2MP tree" means. Can you expand on this? It seems natural that a node might need to see whether a given Replication segment is being used by any trees at the moment, and it's unclear why tracking that information would be explicitly prohibited.

You probably need a definition for "Penultimate-Hop Popping behavior," either in this document or by reference. Alternatively, don't make it a Capitalized Term and just say something like "Replication Nodes upstream of the Leaf nodes can remove the Tree-SID from the packet before forwarding, avoiding the need to configure the Leaf nodes to [whatever]."

In Section 3.4, isn't this the process at *each* node, not just the Root?

Hi Rishabh, Dan, Clarence, Hooman, and Jeffrey,

Thank you for the effort put into this specification, which leverages RFC9524 and RFC9256.

Please find some comments below:

# Check 

Section 2 has the following:

   It is similar to SR Policy
   [RFC9256].  Like SR Policy, SR P2MP Policy has one or more Candidate
   Paths and uses same criteria to select the Active Candidate Path.

I’d like to check this as I’m not sure that all parameters are inherited. For example, do we still have Discriminator for the P2MP case as well?

# Active instance of a Candidate Path?!

Section 2.3 says:
   The controller designates an active instance of a CP at the Root node
   of SR P2MP Policy by signalling this state through the protocol used
   to instantiate the Replication segment of the instance.

(1) What is meant by “an active instance of a CP”?

(2) How this behavior interacts with the tie-breaking rules?

As I’m there, please fix this: s/The controller/A controller

# Section 2.4

CURRENT:
   The Tree-SID of the active
   instance of the active Candidate Path SHOULD be used as the Binding
   SID of the SR P2MP Policy.

Why this is not MUST? (see also next comment)

# Steering behavior

CURRENT:
   The Root node can steer an incoming packet into a SR P2MP Policy in
   one of following methods:

   *  Local Policy-Based Routing: The Root node selects the active P2MP
      tree instance of the active Candidate Path of the SR P2MP Policy
      based on local policy.  The procedures to map an incoming packet
      to a SR P2MP Policy are out of scope of this document.

   *  Tree-SID Based Routing: The Binding SID (Tree-SID) in the incoming
      packet is used to map the packet to the appropriate P2MP tree
      instance.

(1) Should the behavior of the root node be part of the instructions received from the controller?

(2) As discussed earlier in the document, there is room for a case where BID!=Tree-SID. What is the expected behavior in such cases?

(3) s/Local Policy-Based Routing/Local Policy-Based forwarding and s/Tree-SID Based Routing/Tree-SID Based forwarding

# Section 3.1

CURRENT:
   The Tree-SID SHOULD also
   serve as the Replication-SID for the Replication segments at
   intermediate Replication nodes and Leaf nodes.   

Please provide the rationale for this one. What are the implications if this SHOULD is not followed?

# Section 3.2

(1)

CURRENT:
   A shared Replication Segment SHOULD be identified using a Root-ID set
   to zero (0.0.0.0 for IPv4 and :: for IPv6) along with a Replication-
   ID that is unique within the context of the node where the
   Replication segment is instantiated.

Idem as previous point, why this isn’t a MUST? At least the rationale should be called out.

(2)

CURRENT:
   However, a shared Replication segment MUST NOT be associated with an
   SR P2MP tree.

Does this apply even if this is shared only between a subset and not all instances?

# Section 3.3

## Transport/Service Context

CURRENT:
   For multi-point services, the transport identifier which is the Tree-
   SID or Replication SID at a Leaf node is also associated with the
   service context because it is not always feasible to separate the
   transport and service context with efficient replication in core
   since a) multi-point services may have differing sets of end-points,
   and b) downstream allocation of service context cannot be encoded in
   packets replicated in the core.

I guess I understand what is meant here by these contexts, but it would be better to introduce these first.

## Deployment matter?

CURRENT:
   However, for SR-MPLS deployments, if it is known a priori that multi-
   point services mapped to a P2MP tree can be uniquely identified
   within the SR domain, a controller MAY opt not to instantiate
   Replication Segments at Leaf nodes.   

How is that made known to the controller? Also, shouldn’t this better handled by a policy?

# Section 4.2

CURRENT:
   A controller performs the following functions in general:

   *  Topology Discovery: A controller discovers network topology across
      Interior Gateway Protocol (IGP) areas, levels or Autonomous
      Systems (ASs).

   *  Capability Exchange: A controller discovers a node's capability to
      participate in SR P2MP tree as well as advertise it’s capability
      to compute P2MP trees.

It also need to retrieve installed tree instances in the underly domain when it first bootstraps.

# Section 4.3

## Loops

CURRENT:
   A controller MUST compute a P2MP tree such that there are no loops in
   the tree at steady state as required by [RFC9524]).
 
I guess this should be conditional: IF the controllers computes a tree, then it must be forwarding loop-free.

(nit) delete the extra “)”.

## Policy-based

CURRENT:
   A controller SHOULD modify a P2MP tree of a Candidate Path on
   detecting a change in the network topology or in case a better path
   can be found based on the new network state.  In this case, the
   controller MAY create a new instance of a P2MP tree and remove the
   old instance of the tree from the network in order to minimize
   traffic loss.

The SHOULD is scoped vaguely. No every topology change will trigger a modification of the tree.

Also, I guess some policy is needed to drive the controller behavior for migrating to a new path/instance.

# Section 4.5.2

CURRENT:
   It is possible for a controller create a disjoint backup tree
   instance for providing end-to-end path protection.

Well, this depends on the underlying topology.

Not sure this statement (even if adjusted) adds much to the discussion.

# Additional Operational Considerations

I was expecting some discussion about scalability matters and how to test an active CP. 

Please consider adding some discussion about these. Adding readily-available pointers (if any) would work as well. Thanks

# Minor points

## Abstract

(1) Circular definition

CURRENT :
   A SR P2MP Policy consists of Candidate Paths (CP) which
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^            
   define the topology of P2MP tree instances in each Candidate Path.
   ^^^^^^                                     ^^^^^^^^^^^^^^^^^^^^^^  

(2) (nit) s/Paths and and how P2MP trees/Paths and how P2MP trees

## Introduction

(1) Cite an authoritative reference

CURRENT:
   A Multi-point service delivery can be realized with P2MP trees in a
   Segment Routing domain.
   ^^^^^^^^^^^^^^^^^^^^^^  

(2) There might be multiple roots

OLD:
   A controller computes P2MP tree instances, from the Root to Leaf
   nodes, 

NEW:
   A controller computes P2MP tree instances, from a Root to Leaf
   nodes, 

(3) nit

OLD: Once computed, the controller instantiate a P2MP tree instance
NEW: OLD: Once computed, the controller instantiates a P2MP tree instance

(4) Missing references

CURRENT:
   The Replication segments of a P2MP tree can be instantiated for both
   SR-MPLS and SRv6 dataplanes, enabling efficient packet replication
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   within an SR domain.

## Section 2.2: point the reader to Section 4 where these matters are zoomed into

OLD: A SR P2MP Policy is provisioned on a controller.   

NEW: An SR P2MP Policy is provisioned on a controller (see Section 4).   

## Section 2.3

CURRENT:
   The Root
   node selects the active Candidate Path based on the tie breaking
   rules defined in [RFC9256].
                 ^^^^^^^^^^^^

Please add the exact section to look at.

## Section 4

OLD:
   A controller is provisioned with SR P2MP Policy and it's Candidate
                                                       ^^^^^^
   Paths to compute and instantiate P2MP trees in SR domain.  Once
                                               ^^^^^^^^^^^^ 
   computed, the controller instantiates the Replication segments that
   compose the P2MP in the SR domain nodes using signalling protocols
   such as PCEP, BGP, NetConf etc.  The procedures for provisioning a
                      ^^^^^^^^^^^^  
   controller and the instantiation Replication segments in SR domain
                                  ^^^^                   ^^^^^^^^
   are outside the scope of this document.

NEW:
   A controller is provisioned with SR P2MP Policy and its Candidate
   Paths to compute and instantiate P2MP trees in an SR domain.  Once
   computed, the controller instantiates the Replication segments that
   compose the P2MP in the SR domain nodes using signalling protocols
   such as PCEP, BGP, NETCONF, etc.  The procedures for provisioning a
   controller and the instantiation of Replication segments in an SR domain
   are outside the scope of this document.

## Section 4.2

s/ASs/ASes
s/it’s capability/its capability

## Appendix

Replication SIDs formatted as SRv6 Segment Identifier (SID). The textual representation of those has to adhere to RFC 5952, especially this part:

   The characters "a", "b", "c", "d", "e", and "f" in an IPv6 address
   MUST be represented in lowercase.

Cheers,
Med