Hi Adrian, Thanks a lot for your thorough review.
Your comment about draft-ietf-pce-pcep-extension-for-pce-controller is legitimate. Good news: it's in the RFC Editor queue (https://www.rfc-editor.org/current_queue.php#draft-ietf-pce-pcep-extension-for-pce-controller) and associated code points have already been allocated (https://www.iana.org/assignments/pcep/pcep.xhtml#pcecc-capability). Cheers, Dhruv & Julien On 23/03/2021 11:17, Adrian Farrel wrote: > Hi Julien, WG, authors. > > Code point allocation: Is the request for all of the code points in the > document? What about the not-yet-allocated code point from > [I-D.ietf-pce-pcep-extension-for-pce-controller]. This spec can't be > implemented without it. > > WG last call: I have a few questions/issues/nits below. > > Best, > Adrian > > == Questions / Issues == > > Abstract > > What is the difference between "a Binding Segment Identifier (BSID)" and > a "binding Segment-ID (SID)"? > > --- > > Abstract > > "Such a binding label/SID" > This is the first use of "label". You need some context. > > --- > > Abstract > > This document proposes > an approach for reporting binding label/SID to Path Computation > Element (PCE) for supporting PCE-based Traffic Engineering policies. > > Who does the reporting? > Same in Section 1 top of page 4. > > --- > > 3. > > o BT = 0: The binding value is an MPLS label carried in the format > specified in [RFC5462] where only the label value is valid, and > other fields MUST be considered invalid. The Length MUST be set > to 7. > > I don't think that RFC 5462 is the right reference. That document simply > renames a field in the MPLS label stack entry. > > So, are you carrying an MPLS label or an MPLS label stack entry? Either > is possible, although since you're only interested in the label, it > might be more normal to carry just the label value in the least > significant 20 bits of the binding value field. That would be consistent > with a Length of 7. > > But, if you want to carry the full label stack entry (with the other > fields ignored) then perhaps... > o BT = 0: The binding value is an MPLS label carried in an MPLS > label stack entry format as specified in [RFC3032] where only the > label value is valid, and other fields MUST be ignored. The > Length MUST be set to 8. > > This would be more consistent with: > o BT = 1: Similar to the case where BT is 0 except that all the > fields on the MPLS label entry are set on transmission. However, > the receiver MAY choose to override TC, S, and TTL values > according its local policy. The Length MUST be set to 8. > But here you may want a little more clarity as: > o BT = 1: Similar to the case where BT is 0 except that all the > fields of the MPLS label stack entry are set on transmission of > the PCEP message containing the TLV. A PCC receiver of this TLV > in a PCEP message MAY choose to override TC, S, and TTL values > according its local policy. The Length MUST be set to 8. > > But, at the bottom of the section... > For the BT as 0, the 20 bits represent the MPLS > label. For the BT as 1, the 32-bits represent the label stack entry > as per [RFC5462]. > Which is going back on itself (and using the wrong reference). > > Just make a decision on the meaning of BT=0 and make the text clean. > > --- > > 3. > > I'm a bit puzzled as to why this document defines two flags for the > Path Binding TLV Flags field, when this document clearly doesn't use or > depend on them. > > In order to not make [I-D.ietf-spring-segment-routing-policy] a > normative reference, perhaps you should not mention the specific bits, > create the registry (in 11.1.1) as empty, and simply not that > [I-D.ietf-spring-segment-routing-policy] defines some flags. > > (Obviously, [I-D.ietf-spring-segment-routing-policy] will need to make > assignments from the registry.) > > --- > > 4. > > Multiple TE-PATH-BINDING TLVs are allowed to be present in the same > LSP object. This signifies the presence of multiple binding SIDs for > the given LSP. > > If one of these contains a bad value, and a PCErr is sent according to > the previous paragraph, what happens to all the other Binding Values? > Are they used or discarded? > > --- > > 4. > > For SRv6 BSIDs, it is RECOMMENDED to always explicitly specify the > SRv6 Endpoint Behavior and SID Structure in the TE-PATH-BINDING TLV > by setting the BT (Binding Type) to 3, instead of 2. The choice of > interpreting SRv6 Endpoint Behavior and SID Structure when none is > explicitly specified is left up to the implementation. > > I puzzled about the alternative to "RECOMMENDED". I wondered if the > second sentence was meant to offer that guidance, but perhaps it is > intended to only to cover the case when no Path Binding TLV is present. > Two concepts in one paragraph? > > --- > > 4. > > If a PCC wishes to withdraw or modify a previously reported binding > value, it MUST send a PCRpt message without any TE-PATH-BINDING TLV > or with the TE-PATH-BINDING TLV containing the new binding value > respectively. > > If a PCE wishes to modify a previously requested binding value, it > MUST send a PCUpd message with TE-PATH-BINDING TLV containing the new > binding value. The absence of TE-PATH-BINDING TLV in PCUpd message > means that the PCE does not specify a binding value in which case the > binding value allocation is governed by the PCC's local policy. > > How does this work if multiple binding values have been assigned by > using multiple TE-PATH-BINDING TLVs? Suppose, having done that, it is > the intention to withdraw one of the binding values but leave the others > in place. Should the message making the change contain all the TLVs > except the one being withdrawn? > > If a PCC receives a valid binding value from a PCE which is different > than the current binding value, it MUST try to allocate the new > value. If the new binding value is successfully allocated, the PCC > MUST report the new value to the PCE. Otherwise, it MUST send a > PCErr message with Error-Type = TBD2 ("Binding label/SID failure") > and Error Value = TBD4 ("Unable to allocate the specified label/ > SID"). > > And in this failure case, what is the residual state? Is the old value > still in place, or is no value in place? > > > > > > == A whole pile of nits and minor issues == > > Abstract > > s/a BSID to RSVP-TE/a BSID to an RSVP-TE/ > s/or binding Segment-/or a binding Segment-/ > s/to SR Traffic/to an SR Traffic/ > > --- > > Abstract > > "This document proposes". You need to word this to be appropriate for an > RFC. Thus, "This document specifies". > > --- > > Abstract > > s/for supporting/to support/ > > --- > > Requirements Language should move to Section 2. > > --- > > 1. > > Expand PCE on first use > > --- > > 1. > > s/through a network that are/through a network where those paths are/ > s/[RFC8402], Binding/[RFC8402], a Binding/ > s/an Segment Routed/a Segment Routed/ > s/equal to BSID/equal to a BSID/ > s/interface or tunnels/interface or tunnel/ > s/as segments/as a segment/ > s/extension to PCEP that allows/extensions to PCEP that allow/ > s/PCEP extension to setup/PCEP extensions to set up/ > > --- > > 1. Please expand "LSP" on first use > > --- > > 1. > > The PCEP extension to setup and maintain SR-TE > paths is specified in [RFC8664]. > > [RFC8664] provides a mechanism for a network controller (acting as a > PCE) to instantiate candidate paths for an SR Policy onto a head-end > node (acting as a PCC) using PCEP. > > Do you need both of these sentences? > > --- > > 1. > > s/"Binding label/SID has local"/"A binding label/SID has local"/ > s/the following diagram/Figure 1/ (you can use an xref) > s/"In IP/MPLS WAN"/"In the IP/MPLS WAN"/ > s/setup using/set up using/ > > --- > > 1. > > Binding value means either MPLS label or SID > throughout this document. > > This might read better as: > > Throughout this document, the term "binding value" means either an > MPLS label or SID. > > --- > > 2. > > The following are not used in this document and don't need to be in the > list: > LER > LSR > > The following are not used in later sections of this document and don't > need to be in the list: > SRGB > SRLB > > --- > > 3. > > s/in the figure below/in Figure 3/ > s/in ([RFC8231])/in [RFC8231]/ > s/type of this TLV is to be allocated by IANA/type of this TLV is TBD1/ > > --- > > 3. > > For Binding Type (BT), a forward pointer to 11.1.1 would be nice. > > --- > > 3.1. > > Carried as the Binding Value in the TE-PATH-BINDING TLV when the BT > is set to 3. Applicable for SRv6 Binding SIDs > [I-D.ietf-spring-srv6-network-programming]. > > These need to be sentences with a subject. > I think you can update to [RFC8986]. > > --- > > 3.1 > > Figure 4 should be numbered Figure 3. > The text should contain an explicit mention of Figure 3. > The text should mention each of the fields in the figure. > > --- > > 3.1 > Endpoint Behavior: 2 octets. The Endpoint Behavior code point for > this SRv6 SID as defined in section 9.2 of > [I-D.ietf-spring-srv6-network-programming]. > There is no section 9.2 in RFC 8986. You're probably after section 10.2. > But is that the right thing to do? Don't you want to point at the IANA > registry so that you embrace any future values as well? > > --- > > 3.1 > LB Length: 1 octet. SRv6 SID Locator Block length in bits. > > LN Length: 1 octet. SRv6 SID Locator Node length in bits. > > Function Length: 1 octet. SRv6 SID Function length in bits. > > Argument Length: 1 octet. SRv6 SID Arguments length in bits. > > It is unclear what these lengths refer to in the context of this binding > value. Probably you can just provide pointers to where the meaning is > found. > > --- > > 4. > s/([RFC5440])/[RFC5440]/ > s/message, PCE MUST/message, the PCE MUST/ > s/MUST send the PCErr/MUST send a PCErr/ > > --- > > 4. > If a PCE requires a PCC to allocate a specific binding value, it may > do so by sending a PCUpd or PCInitiate message containing a TE-PATH- > BINDING TLV. > In order to avoid confusion, I suggest s/may do/does/ > > --- > > 4. > If a PCE requires a PCC to allocate a specific binding value, it may > do so by sending a PCUpd or PCInitiate message containing a TE-PATH- > BINDING TLV. > > "it may do so" is probably "it instructs the PCC" > > --- > > 4. > > s/receives TE-PATH-BINDING TLV/receives a TE-PATH-BINDING TLV/ > s/other than LSP object/other than an LSP object/ > s/It may do so by sending a PCUpd/It does so by sending a PCUpd/ > > --- > > 5. > > "a PCErr message is sent" Who does the sending? "the receiver"? > > --- > > 6. > > s/for SRv6 SID./for an SRv6 SID./ > > --- > > 7. > > s/This section specify/This section specifies/ > > --- > > 7. > > "allocate the binding label" Do you mean "binding label/SID"? > The rest of the paragraph seems a little inconsistent on that. > Perhaps, also, the section title is wrong. > > --- > > 7. > > s/before PCE could/before the PCE can/ > > --- > > 7. > > The TLV in LSP object identifies what should be > allocated, such as Binding label/SID. > > Suggest to remove this sentence as superfluous with following text. > > --- > > 7. > > OLD > o to let the PCC allocates the binding label/SID, a PCE could set > P=0 and empty TE-PATH-BINDING TLV ( i.e., no binding value is > specified) in the LSP object in PCInitiate/PCUpd message. > NEW > o to let the PCC allocates the binding label/SID, a PCE could set > P=0 and include an empty TE-PATH-BINDING TLV (i.e., no binding > value is specified) in the LSP object in PCInitiate/PCUpd message. > END > > Similarly > > OLD > o a PCC could request that the PCE allocate the binding label/SID by > setting P=1, D=1, and empty TE-PATH-BINDING TLV in PCRpt message. > NEW > o a PCC could request that the PCE allocate the binding label/SID by > setting P=1, D=1, and including an empty TE-PATH-BINDING TLV in > PCRpt message. > END > > --- > > 7. > > Bullets following "Note that,..." should all start with capital letters > so that multi-sentence bullet text is not confused. > > --- > > 7. > > o if both peers have not exchanged the PCECC capabilities as per > [I-D.ietf-pce-pcep-extension-for-pce-controller] and it receives > P=1 in the LSP object, it needs to act as per > [I-D.ietf-pce-pcep-extension-for-pce-controller]: > > What is "it"? > > --- > > 7. > > * Send a PCErr message with Error-Type=19 (Invalid Operation) and > Error-Value=TBD (Attempted PCECC operations when PCECC > capability was not advertised) > > I understand this to mean to use the Error-Value defined in > [I-D.ietf-pce-pcep-extension-for-pce-controller] and marked in that > document as TBD3. > > I suggest you use > > * Send a PCErr message with Error-Type=19 ("Invalid Operation") > and Error-Value=TBD7 ("Attempted PCECC operations when PCECC > capability was not advertised"). [RFC-EDITOR: Please replace > TBD7 with the value assigned by IANA for this Error-Value, > indicated by "TBD3" in [I-D.ietf-pce-pcep-extension-for-pce- > controller].] > > --- > > 7. > PCE would directly allocate the label > from the PCE-controlled label space using P=1 as described above, > whereas PCE would request for the allocation of a specific BSID from > the PCC-controlled label space with P=0 as described in Section 4. > s/PCE would/The PCE can/ (twice) > > --- > > 9. > > s/rouge/rogue/ > s/other LSPs that uses/other LSP that uses/ > > --- > > 11.1.1 > > The two new registries need to have ranges specified so that IANA knows > what new values can be assigned. > > --- > > I think the affiliations of some of your authors and contributors may > be out of date. > >
smime.p7s
Description: S/MIME Cryptographic Signature
_______________________________________________ Pce mailing list [email protected] https://www.ietf.org/mailman/listinfo/pce
