Re: [Pce] WG Last Call for draft-ietf-pce-binding-label-sid-07 (and Code Point Allocation)

[[e-mail]]

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