Looks good. I see Dhruv already pointed out that the new registry needs an allocation policy. That was my only note. Please go ahead and publish, including the change Dhruv requested.
Thanks, —John > On Jan 31, 2024, at 12:02 PM, Cheng Li <c...@huawei.com> wrote: > > > Hi John, > > Thank you so much for your comments, we think they are very helpful. > We have modified the draft to address your comments accordingly, please > review it again. > If they can address your comments, we will update the draft very soon. > > Thanks, > Cheng > > > > -----Original Message----- > From: John Scudder <j...@juniper.net> > Sent: Thursday, January 25, 2024 12:49 AM > To: draft-ietf-pce-segment-routing-i...@ietf.org > Cc: pce@ietf.org > Subject: AD review of draft-ietf-pce-segment-routing-ipv6-20 > > Hi Authors, WG, > > Thanks for this document. > > I’ve supplied my questions and comments in the form of an edited copy of the > draft. Minor editorial suggestions I’ve made in place without further > comment, more substantive questions and comments are done in-line and > prefixed with “jgs:”. You can use your favorite diff tool to review them; > I’ve attached the iddiff output for your convenience if you’d like to use it. > I’ve also pasted a traditional diff below in case you want to use it for > in-line reply. > > One further point regarding “minor editorial suggestions”. While I did make a > few, I didn’t do exhaustive copy-editing of this draft, and of course, the > RFC Editor will do their usual comprehensive job. However, I did notice a > pervasive need for a few particular kinds of changes (for example definite > articles, agreement in number) that mar what’s otherwise a high-quality > document and are likely to bother some reviewers. It’s optional, but it > wouldn’t be a bad idea for you to pass the document through a grammar checker > before we send it for IETF Review. Your call. > > Thanks, > > —John > > --- draft-ietf-pce-segment-routing-ipv6-20.txt 2024-01-24 14:39:13.000000000 > -0500 > +++ draft-ietf-pce-segment-routing-ipv6-20-jgs-comments.txt 2024-01-24 > 18:40:57.000000000 -0500 > @@ -29,12 +29,15 @@ > A Segment Routed Path can be derived from a variety of mechanisms, > including an IGP Shortest Path Tree (SPT), explicit configuration, or > a PCE. > +-- > +jgs: I think it would be appropriate to expand PCE. > +-- > > Since SR can be applied to both MPLS and IPv6 forwarding planes, a > - PCE should be able to compute SR-Path for both MPLS and IPv6 > + PCE should be able to compute an SR path for both MPLS and IPv6 > forwarding planes. The PCEP extension and mechanisms to support SR- > MPLS have been defined. This document describes the extensions > - required for SR support for IPv6 data plane in the Path Computation > + required for SR support for the IPv6 data plane in the Path > + Computation > Element communication Protocol (PCEP). > > Status of This Memo > @@ -151,7 +154,7 @@ > [RFC8754]. > > An SR path can be derived from an IGP Shortest Path Tree (SPT), but > - SR-TE (Segment Routing Traffic Engineering) paths may not follow IGP > + SR-TE (Segment Routing Traffic Engineering) paths might not follow > + IGP > SPT. Such paths may be chosen by a suitable network planning tool, > or a PCE and provisioned on the ingress node. > > @@ -186,7 +189,7 @@ > stateful PCE for computing one or more SR-TE paths taking into > account various constraints and objective functions. Once a path is > chosen, the stateful PCE can initiate an SR-TE path on a PCC using > - PCEP extensions specified in [RFC8281] using the SR specific PCEP > + PCEP extensions specified in [RFC8281] and the SR-specific PCEP > extensions specified in [RFC8664]. [RFC8664] specifies PCEP > extensions for supporting a SR-TE LSP for the MPLS data plane. This > document extends [RFC8664] to support SR for the IPv6 data plane. > @@ -228,6 +231,16 @@ > > The message formats in this document are specified using Routing > Backus-Naur Format (RBNF) encoding as specified in [RFC5511]. > +-- > +jgs: as far as I can tell, no they are not. Either remove this > +paragraph, and the normative reference, or help me understand why I am > +wrong. > + > +Speaking of RBNF, the fact this spec doesn't use it seems to put it out > +of step with some of the other PCE document set. I'm assuming it's the > +consensus of the WG that this is fine. (I don't have a problem with the > +style used here.) > +-- > > Further, following terms are used in the document, > > @@ -240,6 +253,11 @@ > SID: Segment Identifier. > > SRv6: Segment Routing for IPv6 forwarding plane. > +-- > +jgs: in the introduction you expanded this as "Segment Routing over > +IPv6 data plane", which seems like a better expansion to me. But either > +way, please pick one and stick with it. > +-- > > SRH: IPv6 Segment Routing Header. > > @@ -255,6 +273,14 @@ > Basic operations for PCEP speakers are as per [RFC8664]. SRv6 Paths > computed by a PCE can be represented as an ordered list of SRv6 > segments of 128-bit value. > +-- > +jgs: probably just "128 bit SRv6 segments", or if you feel that's not > +explicit enough, "SRv6 segments, each of which is 128 bits in length." > +(The problem with the current version is that first, 128 bits is the > +length of the segment, not the value of it, and second, there's > +possible ambiguity as to whether 128 bits is the length of the list, or > +the length of each individual segment.) > +-- > > [RFC8664] defined a new Explicit Route Object (ERO) subobject denoted > by "SR-ERO subobject" capable of carrying a SID as well as the @@ -271,6 > +297,16 @@ > > This document defines new subobjects "SRv6-ERO" and "SRv6-RRO" in the > ERO and the RRO respectively to carry the SRv6 SID (IPv6 Address). > +-- > +jgs: it's not at all clear to me that it's formally correct to say an > +SRv6 SID is the same as an IPv6 address; for example, the abstract of > +draft-ietf-6man-sids-05 tells me that "Segment Identifiers (SIDs) used > +by SRv6 *can resemble* IPv6 addresses and behave like them while > +exhibiting *slightly different* behaviors in some situations" (my > +emphasis). > + > +Probably the best fix is just to remove the text inside the parentheses. > +-- > SRv6-capable PCEP speakers MUST be able to generate and/or process > these subobjects. > > @@ -304,6 +340,15 @@ > necessary information to guide the packets from the ingress node to > the egress node of the path, and hence there is no need for any > signaling protocol. > +-- > +jgs: that last clause can't possibly be right as written. SR networks > +aren't completely devoid of any and all signaling... consider that PCEP > +itself is a signaling protocol of sorts. While it would be possible to > +expand and rewrite the clause more precisely to be formally correct, I > +think the easiest fix is just to delete it, since it's not directly > +relevant to the subject at hand. (I see this language may have been > +inherited from RFC 8664; I think it's wrong there too.) > +-- > > For the use of an IPv6 control plane but an MPLS data plane, > mechanism remains the same as specified in [RFC8664]. > @@ -412,6 +457,10 @@ > SRv6-SID. > > - Unassigned bits MUST be set to 0 and ignored on receipt. > +-- > +jgs: I think this should be "set to 0 on transmission and ignored on > +receipt", right? > +-- > > A pair of (MSD-Type, MSD-Value): Where MSD-Type (1 octet) is as > per the IGP MSD Type registry created by [RFC8491] and populated @@ > -426,12 +475,29 @@ > to ensure those midpoints are able to correctly process their > segments and for the tailend to dispose of the SRv6 encapsulation. > The SRv6 MSD capabilities of these other nodes MAY be learned as part > +-- > +jgs: The above RFC 2119 style "MAY" should probably be "may", or better > +still "might", I don't think you're stating a requirement here (or in > +this case, since "MAY", a non-requirement). I assume you'd take the > +position that it's beyond the scope of this specification to say how > +the capabilities of the other nodes have to be learned. > +-- > of the topology information via IGP/BGP-LS or via PCEP if the PCE > also happens to have PCEP sessions to those nodes. > +-- > +jgs: you need a reference for BGP-LS. > +-- > > It is RECOMMENDED that the SRv6 MSD information be not included in > the SRv6-PCE-Capability sub-TLV in deployments where the PCE is able > to obtain this via IGP/BGP-LS as part of the topology information. > +-- > +jgs: By using the RFC 2119 style "RECOMMENDED" you're making this an > +almost-MUST. Do you intend that? If so, please consider providing > +guidance as to when the requirement can be disregarded. > + > +But I think probably you mean "recommended". > +-- > > 4.2. The RP/SRP Object > > @@ -478,6 +544,12 @@ > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > > Figure 2: SRv6-ERO Subobject Format > +-- > +jgs: the fields marked "optional" aren't really completely optional, > +are they? I'm not aware of a totally standard pattern for this kind of > +field, but I have seen the term "conditional" used in such a context, > +or even "conditional, see below". > +-- > > The fields in the SRv6-ERO subobject are as follows: > > @@ -511,6 +583,13 @@ > below) then the NT field has no meaning and MUST be ignored by the > receiver. This document reuses NT types defined in [RFC8664], but > assigns them new meanings appropriate to SRv6. > +-- > +jgs: I feel very uncomfortable about being told that you're using an > +existing registry, but assigning different semantics to > +already-allocated code points. I think the most straightforward fix to > +this would be to create a new "PCEP SRv6-ERO NAI Types" registry, > +populated with values 0, 2, 4 and 6, defined however you like. > +-- > > If NT value is 0, the NAI MUST NOT be included. > > @@ -531,6 +610,11 @@ > identify the IPv6 Adjacency and used with the SRv6 Adj-SID. > > SR-MPLS specific NT types are not valid in SRv6-ERO. > +-- > +jgs: if you take my suggestion of creating a new registry, the above > +line becomes unnecessary. There would be no SR-MPLS specific NT types > +in your new registry. > +-- > > Flags: Used to carry additional information pertaining to the > SRv6-SID. This document defines the following flag bits. The other @@ > -575,9 +659,23 @@ > is recommended to signal it always if possible. It could be used for > maintainability and diagnostic purpose. If behavior is not known, > the opaque value '0xFFFF' is used [RFC8986]. > +-- > +jgs: please be explicit that the behavior values are taken from the > +registry "SRv6 Endpoint Behaviors". > + > +It sure seems strange to me to choose to use a reserved value called > +"opaque" to signal "behavior not known" instead of having a registered > +value called "behavior not known". But, it's probably too late to > +change this now. > +-- > > SRv6 SID: SRv6 Identifier is an 128-bit IPv6 address representing the > SRv6 segment. > +-- > +jgs: but is it in fact an address? See my comment with reference to > +draft-ietf-6man-sids-05 earlier on. An easy fix would be to replace > +"IPv6 address" with "value". > +-- > > NAI: The NAI associated with the SRv6-SID. The NAI's format depends > on the value in the NT field, and is described in [RFC8664]. > @@ -650,7 +748,7 @@ > validation of SRv6 SIDs being instantiated in the network and checked > for conformance to the SRv6 SID allocation scheme chosen by the > operator as described in Section 3.2 of [RFC8986]. In the future, > - PCE can also be utilized to verify and automate the security of the > + PCE might also be utilized to verify and automate the security of > + the > SRv6 domain by provisioning filtering rules at the domain boundaries > as described in Section 5 of [RFC8754]. The details of these > potential applications are outside the scope of this document. > @@ -811,7 +909,7 @@ > 5.2.1. SRv6 ERO Validation > > If a PCC does not support the SRv6 PCE Capability and thus cannot > - recognize the SRv6-ERO or SRv6-RRO subobjects. It should respond > + recognize the SRv6-ERO or SRv6-RRO subobjects, it should respond > according to the rules for a malformed object as described in > [RFC5440]. > > @@ -832,6 +930,11 @@ > MUST be 48, otherwise the Length MUST be 64. > > * NT types (1,3, and 5) are not valid for SRv6. > +-- > +jgs: If you take my suggestion to set up a separate registry, you won't > +need the above bullet, because NT types 1, 3, and 5 won't exist in that > +registry. > +-- > > * If T bit is 1, then S bit MUST be zero. > > @@ -963,10 +1066,15 @@ > 7.2. Information and Data Models > > The PCEP YANG module is out of the scope of this document and defined > - in other drafts. An augmented YANG module for SRv6 is also specified > - in another draft that allows for SRv6 capability and MSD > + in other documents. An augmented YANG module for SRv6 is also specified > + in another document that allows for SRv6 capability and MSD > configurations as well as to monitor the SRv6 paths set in the > network. > +-- > +jgs: please provide informative references to the "other documents" and > +"another document" (which I've edited from "other drafts" and "another > +draft"). > +-- > > 7.3. Liveness Detection and Monitoring > > > > <draft-ietf-pce-segment-routing-ipv6-21-00.diff > (1).html><draft-ietf-pce-segment-routing-ipv6-21-00.md> _______________________________________________ Pce mailing list Pce@ietf.org https://www.ietf.org/mailman/listinfo/pce
