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

["Chengli (Cheng Li)" <[e-mail]>]

Hi Adrian,

Long time no see! Thanks for your valuable comments!  :)

It takes few days to address them, LOL. The latest revision has updated, see
https://raw.githubusercontent.com/dhruvdhody/ietf/master/draft-ietf-pce-binding-label-sid-08.txt
https://github.com/dhruvdhody/ietf/blob/master/draft-ietf-pce-binding-label-sid-08.xml

Diff is attached.
Please see my reply inline.

Respect,
Cheng



-----Original Message-----
From: Adrian Farrel [mailto:[e-mail]] 
Sent: Tuesday, March 23, 2021 6:18 PM
To: [e-mail]; [e-mail]
Cc: [e-mail]
Subject: RE: [Pce] WG Last Call for draft-ietf-pce-binding-label-sid-07 (and Code Point Allocation)

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)"?

---

[Cheng] Removed Segment-ID


Abstract

"Such a binding label/SID"
This is the first use of "label". You need some context.

---

[Cheng] Added - "This document specifies the binding value as an MPLS label or Segment Identifier."


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.


[Cheng] Updated to identify PCC.  

---

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.

---

[Cheng] How about this - 

   o  BT = 0: The binding value is a 20-bit MPLS label value.  The TLV
      is padded to 4-bytes alignment.  The Length MUST be set to 7 and
      first 20 bits are used to encode the MPLS label value.

   o  BT = 1: The binding value is a 32-bit MPLS label stack entry as
      per [RFC3032] with Label, TC [RFC5462], S, and TTL values encoded.
      Note that the receiver MAY choose to override TC, S, and TTL
      values according to its local policy.  The Length MUST be set to 8.





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

---
[Cheng] The idea was to make it consistent with IDR WG I-D [draft-ietf-idr-segment-routing-te-policy]. 
One option could be to move this to the SR-policy-related PCE WG I-D [draft-ietf-pce-segment-routing-policy-cp]. 
I have kept this open for now for further discussion.


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?


[Cheng] They should not be impacted. I have added this - 

   In case of multiple TE-PATH-BINDING TLVs, all
   instances of TE-PATH-BINDING TLVs MUST always be included in the LSP
   object.  In case of an error, a PCErr message MAY include the
   offending TE-PATH-BINDING TLV in the PCEP-ERROR object.


---

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?

---
[Cheng]
The idea was to say that while BT=2 (just IPv6 address) for BSID is allowed, there are older implementations, but BT=3 SHOULD be proffered which requires explicit SID structure and behavior to be explicitly encoded. 
It is not about when no Path Binding TLV is present.
Do you have a better way to put this?



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?

[Cheng] It has been specified that all instances of TE-PATH-BINDING TLVs need to be included and the offending TE-PATH-BINDING TLV can be included in the PCEP-ERROR object. 
Any suggestion to make this clearer?



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

---

[Cheng] Updated


1.

Expand PCE on first use

[Cheng]Done. BTW PCE has a * in https://www.rfc-editor.org/materials/abbrev.expansion.txt

---

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.

---

[Cheng] Updated for the above comments.

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?

---
[Cheng] Updated for the above comments.


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.

---

[Cheng] Added - "The following fields are used to advertise the length of each individual part of the SRv6 SID as defined in [RFC8986]."



4.
s/([RFC5440])/[RFC5440]/
s/message, PCE MUST/message, the PCE MUST/ s/MUST send the PCErr/MUST send a PCErr/

---


[Cheng] Updated


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/

[Cheng]Ignoring, updated as per the below suggestion.


---

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"

---

[Cheng] Updated (ignored the 3rd nit below)


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/

---

[Cheng]Updated


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.

---

[Cheng] No following text :)
I suggest changing it to - "The TE-PATH-BINDING TLV in the LSP object identifies that the allocation is for Binding label/SID."




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"?

[Cheng] Changed the first "it" to "a PCEP peer".



---

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

---
[Cheng] IANA has made the allocation as the I-D is in the RFC-Editor queue. 
Perhaps we can use the value directly as per https://www.iana.org/assignments/pcep/pcep.xhtml#pcep-error-object?



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.

[Cheng]Updated