[mpls] Opsdir last call review of draft-ietf-mpls-rfc6374-sr-11

[Dhruv Dhody via Datatracker <[e-mail]>]

Reviewer: Dhruv Dhody
Review result: Has Issues

# OPSDIR review of draft-ietf-mpls-rfc6374-sr-11

I have reviewed this document as part of the Operational directorate's ongoing
effort to review all IETF documents being processed by the IESG.  These
comments were written with the intent of improving the operational aspects of
the IETF drafts. Comments that are not addressed in the last call may be
included in AD reviews during the IESG review.  Document editors and WG chairs
should treat these comments just like any other last-call comments.

The document describes the Performance Delay and Loss Measurement techniques in
the SR-MPLS network.

The document does not include an explicit manageability & operational
considerations section. Authors could consider adding it. Both RFC 6374 and RFC
7876 have it and it would make sense to call out considerations that are
specific to SR-MPLS (if any).

## Comments

- Perhaps it was a mistake to include "rfc6374" in the filename.

- The document uses the term "policies" without any qualifier. Suggest to use
the explicit term SR Policy.

- Section 1, "Segment Routing (SR) leverages the source routing paradigm for
Software Defined Networks (SDNs)"; what is the purpose of "for SDN"? SDN term
is not used in this document and is not a common framing in the spring RFCs
either.

- Section 2.2, consider adding references where appropriate. Add LSE, DM, LM

- Section 3, "The packet loss measurement using Alternate-Marking Method
defined in [RFC9341] MAY use Block Number (BN) for data correlation."; Since
you are restating from RFC 9341, either do a quote or avoid using normative
MAY. (also in section 6.4)

- Section 4.1.2, In "The query messages MUST be transmitted using each SL of
the SR-MPLS Policy Candidate-Path", IMHO each SL is unclear. The next sentence
"A query message for an end-to-end SR-MPLS Policy, used for delay and/or loss
measurement, contains SR-MPLS label stack of the Segment List(s) of the
Candidate-Path, with the G-ACh Label (GAL) at the bottom of the stack (with
S=1) as shown in Figure 2" gives an impression that a single query message
contains label stack of all segment list. How about "every SL" in the first
sentence and rephrase the 2nd? Also, it would make sense for Figure 2 to be
more explicit on what 0x0001, version, reserved, and channel Type is and what
follows it (ACH).

- Section 4.2.2, I don't think that "[I-D.ietf-pce-sr-bidir-path]" is the
correct reference for "return SR-MPLS path". I suggest removing the reference.
If you prefer you can add a separate sentence with the role of PCE in SR
bidirectional path setup.

- Section 4.2.3, same comment as above! Also, I wonder if this is the correct
usage of the normative keyword in "MUST NOT be punted out of the fast path".
BTW I don't see similar text in RFC 6374 FWIW. It would also be a good idea to
add an example showing a label stack with forward and reverse paths.

- Section 5.1, what do you mean by the same value in "the same MPLS DM ACH
value MUST be used"? Is it value 0x000C for delay measurement? The use of MUST
feels off. (similar phrases in 6.1 and 6.2).

- Section 6.3, should this document explicitly state that PSID MUST be used for
LM? Should Figure 3 also include other labels, this gives the impression that
it is just PSID and GAL. Should add text to explain how the loss measurement
works when the same PSID is used for all SL and candidate paths and the packet
loss accounting can be done on the SR policy level?

- Section 7.1, the text "The Return Path TLV is a Mandatory TLV Type." gives
the impression that the TLV is mandatory. Perhaps say that the TLV belongs to
the Mandatory subspace as per RFC6374?

- Section 7.1.1, I don't think that the PCE document is the correct reference
to Binding SID as currently phrased. Either use RFC 8402 and phrase the text in
terms of the role of PCE in BSID allocation.

- Section 7.2, clearly states what is the meaning of the R flag. I am also
unsure on why it is needed.

- Section 8, talks about anycast in the context of ECMP. But an SR path made up
of node SIDs could also have ECMP between the nodes, why is that not called
out? Also, what do you mean by "sweeping of entropy label"? You say that "loss
measurement for different ECMP paths of an SR-MPLS Policy are outside the
scope", what about delay?

- Section 12, IANA no longer prefers the term "sub-registry", they ask us to
use "registry group" and "registry" instead.

## Nits

- Suggested rewording of the abstract as - “Segment Routing (SR) leverages the
source routing paradigm. SR applies to the Multiprotocol Label Switching data
plane (SR-MPLS) as specified in RFC 8402. RFC 6374 and RFC 7876 specify
protocol mechanisms to enable efficient and accurate measurement of packet
loss, one-way and two-way delay, as well as related metrics such as delay
variation in MPLS networks. RFC 9341 defines the Alternate-Marking Method using
Block Number (BN) as a data correlation mechanism for packet loss measurement.
This document utilizes mechanisms from RFC 6374, RFC 7876, and RFC 9341 for
performance delay and loss measurements in SR-MPLS networks, covering both
links and end-to-end SR-MPLS paths, including SR Policies.”

- s/requirements to measure network performance/requirements for measuring
network performance/

- s/an UDP return path/a UDP return path/

- s/well-suited in SR-MPLS/well-suited to SR-MPLS/

- s/are sent as following:/are sent as follows:/

- s/to collect transmit and receive/to collect, transmit, and receive/g

- s/include or more optional TLVs/include one or more optional TLVs/

- Please run through a grammar check, various issues such as article missing.

Thanks!
Dhruv