Re: [spring] AD review draft-ietf-spring-mpls-path-segment-15

[Cheng Li <[e-mail]>]

Hi Jim,

Please see my reply, the updated text is attached, if you are ok with the update, we can submit the draft very soon.

Thanks,
Cheng


From: spring <[e-mail]> On Behalf Of James Guichard
Sent: Monday, October 9, 2023 4:47 PM
To: [e-mail]
Cc: [e-mail]; SPRING WG <[e-mail]>; James Guichard <[e-mail]>
Subject: [spring] AD review draft-ietf-spring-mpls-path-segment-15

Dear Authors,

Please find my review below of draft-ietf-spring-mpls-path-segment-15. I have used idnit on this version so that you can see the line numbers for my comments.

##########

24              In SR for MPLS data plane (SR-MPLS), an Egress node can not determine

Jim> replace 'can not' with 'cannot'

26              because the segment identifiers are stripped from the label stack as
[Cheng]done

Jim> More accurately the segment identifiers are 'swapped' or 'popped' from the label stack. Suggest changing 'stripped' to 'removed'.

91           1.  Introduction

93              Segment Routing (SR) [RFC8402] leverages the source-routing paradigm
94              to steer packets from a source node through a controlled set of
95              instructions, called segments, by prepending the packet with an SR
96              header in the MPLS data plane SR-MPLS [RFC8660] through a label stack
97              to construct an SR path.

[Cheng]Done

Jim> I would suggest rewording the above paragraph as it is somewhat confusing for those who do not know the technology. Suggest:

                   Segment Routing (SR) [RFC8402] leverages the source-routing paradigm
                   to steer packets from a source node through a controlled set of
                   instructions, called segments, by prepending the packet with an SR
                   header. In the MPLS data plane SR-MPLS [RFC8660] the SR header is
                 instantiated through a label stack.

100           the labels in the MPLS label stack will be swapped or popped.  So
101           that no label or only the last label (e.g. a service label or an

Jim> I would replace 'So that ...' with 'The result of this is that no label ...'. In addition, I would remove the text '(e.g. a service label or an Explicit-Null label)' as it is not necessary.

106           However, to support various use-cases in SR-MPLS networks, such as
107           end-to-end 1+1 path protection (Live-Live case) Section 3.3,
108           bidirectional path Section 3.2, or Performance Measurement (PM)
109           Section 3.1, the ability to implement path identification on the
110           egress node is a pre-requisite.

Jim> Please reorder the above paragraph to list the use cases in the order in which they appear in the document e.g. 3.1, 3.2, 3.3.

112           Therefore, this document introduces a new segment type, Path Segment.

Jim> Change the above sentence to 'Therefore, this document defines a new segment type, referred to herein as a Path Segment.'

114           egress node of the path.  It MAY be used by the egress nodes for path

Jim> Change 'nodes' to 'node' above.

115           identification hence to support various use-cases including SR path
116           PM, end-to-end 1+1 SR path protection, and bidirectional SR paths
117           correlation.  Note that, per-path states will be maintained in the

Jim> Remove text 'hence to support various use-cases including SR path PM, end-to-end 1+1 SR path protection, and bidirectional SR paths correlation' as you already stated above. Also, change 'states' to 'state'.

118           egress node due to the requirements in these use cases, though in

Jim> Change 'requirements in these use cases' to 'requirements in the aforementioned use cases'

119           normal cases that the per-path states will be maintained in the
120           ingress node only in the SR architecture.

Jim> Change 'states' to 'state' and remove the text 'in the SR architecture.'

159           A Path Segment is a Local Segment which uniquely identify an SR path

Jim> Change 'identify' to 'identifies'.
[Cheng]Done for above comments

164           The term of SR path used in this document is a path described by a
165           Segment-List (SL).  A PSID is used to identify a Segment List.

Jim> Is the first sentence above necessary? If not please remove it.
[Cheng] yes, it is needed. But how about let move it to the terms section?

166           However, one PSID can be used to identify multiple Segment Lists in
167           some use cases if needed.  For example, one single PSID may be used
168           to identify some or all Segment lists in a Candidate path or an SR
169           policy, if an operator would like to aggregate these Segment Lists in
170           operation.  How to use the PSID to Segment Lists depends on the
171           requirements of the use cases.

Jim> I would remove the last sentence above as it adds no value. The previous sentence explains that a single PSID may be used to identify > 1 segment lists and gives examples. Also should 'may' be 'MAY'?
[Cheng]Done

173           When a PSID is used, the PSID can be inserted at the ingress node and

Jim> 'Can be'? Is it possible for the PSID to be inserted anywhere else other than the ingress node? If not then replace 'can be' with 'is'.
[Cheng] yes, it can be inserted by other nodes other than the ingress node. For example in the nesting use case, a path segment of a sub-path can be inserted the ingress node of the whole path.

174           MUST immediately follow the last label of the SR path associated to it

Jim> Remove text 'associated to it'.

178           when receiving on an intermidate node of the associated path, but it

Jim> Replace 'receiving' with 'received'. Replace 'intermidate' with 'intermediate'.

179           can be the top label in the label stack on the penultimate node after
180           the last forwarding label with Penultimate Hop Popping (PHP) is

Jim> I would remove the text 'after the last forwarding label' as this has already been stated and the text makes the reading awkward. Also, remove the text 'is popped off.'

181           popped off.  Otherwise, the PSID may be processed by an intermediate
182           node, which may cause error in forwarding because of mis-matching.
[Cheng]Done for above comments

Jim> I do not think the last sentence is necessary and I would remove it. It is obvious from the previous text that processing of the PSID by an intermediate node will cause forwarding errors.
[Cheng]This is suggested by Adrian, but I am ok to remove it, and I agree with you

207           RFC8491 the MSD signals the total number of MPLS labels that can be

Jim> Please make RFC8491 a reference and enclose with [].

241        2.1.  Equal-Cost Multipath Considerations

243           If Entropy Label(EL) is also used on this egress node, as per

Jim> Replace 'this' with 'the'.

312           The mechanism of constructing bidirectional path using path segment
313           is out of the scope of this draft and has been described in several

Jim> Replace 'draft' with 'document'.

332           This equivalence group can be instantiated in the network by an SDN

Jim> Remove 'SDN' from the above.

337           Binding SID (BSID) [RFC8402] can be used for SID list compression.
338           With BSID, an end-to-end SR path in a trusted domain can be splitted

Jim> Replace 'splitted' with 'split'.

391           the ingress node of the associated path will learn the info of PSID.

Jim> Replace 'info' with 'information'.

396           deeper label stack which representing a longer path.  For example the

Jim> Replace 'representing' with 'represents'.

397           case described in {#psid-nesting} and the related BSID is not used

Jim> Replace '{#psid-nesting}' with a reference to Section 3.4 of this document.

400           a trusted domain under the considerations defined in [RFC8402].

Jim> What specifically are these considerations and where in RFC8402 are they documented? You need some text here or at a minimum a reference as to which section of RFC8402 you are referring too.

[Cheng]Done for above comments
581        5.6.  Interoperability Test

Jim> Should this section also be removed by the RFC editor upon publication? If so please state this specifically as you have done in the implementation section. If it is not to be removed you need to explicitly state that the information was at the time of publication.
[Cheng]This subsection is included in the section of implementation, so that note applies to here. But it is ok to me to add another note.

Jim