Subject: [Pce] Shepherd's Review of draft-ietf-pce-lsp-setup-type-06

From: Julien Meuric <[email-masked]>
Date: Tue, 21 Nov 2017 18:08:16 +0100

Hi,

Thank you for this new version of the I-D, it has greatly improved and
clarifies former loose zones. Please find my review below.

------
Abstract
---
- s/traffic engineering paths (TE paths)/Traffic Engineering paths (TE
paths)/
- I wonder about the expansion of "TE path" above: why not "Engineered"
instead of "Engineering"? (This is global to the I-D, and beyond... RFC
Editor's list includes both.)
- s/label switched paths (LSPs)/Label Switched Paths (LSPs)/
------
Status
---
- "https://" was introduced in -05, but has now disappeared.
------
1. Introduction
---
- s/Path Computation Element Protocol/Path Computation Element
communication Protocol/

- OLD
path setup type needs to be either explicitly indicated or implied in
the appropriate PCEP messages (when necessary)...
   NEW
path setup type needs to be explicitly indicated in the appropriate PCEP
messages, unless RSVP-TE type is meant (omission implying this type)...
------
3. Path Setup Type Capability TLV
---
- s/Initialization phase/initialization phase/
- I though the discussion on the list was about using a bitmap to
identify supported PSTs: any reason why it is now a list of raw octets?
- The definition of length and padding mixes the words "octet" and
"bytes", depending on the field (probably to due to text coming from RFC
5440). Consistency would be welcome (the comment appears to be
applicable beyond this section).
------
4. Path Setup Type TLV
---
- Figure 2 explicitly includes the codepoint for the "Type" (28), the
"Length" field should be treated similarly (4).
- The last sentence of section 4 puzzles me. If the PATH-SETUP-TYPE TLV
is not supported, the PCEP peer cares little if it was "recognized" or
not. If both sub-cases are commonly handled by ignoring, an
implementation always including the RSVP-TE PST will be able to
interwork with an implementation knowing about the TLV without actually
supporting it; the current text turns this situation into an error.
(Note also that RFC 5440 does not distinguish unrecognized and
unsupported in TLV processing rules.)
- In case my previous comment does not fly, 3 more nits:
  * s/recognizes the TLV but does not support the TLV/recognizes the TLV
but does not support its use/
  * s/send PCErr/send a PCErr message/
  * Error-Type is 2, would not 4 fit better?
------
5. Operation
---
- s/Initialization phase/initialization phase/
- s/MUST infer/MUST consider/  [explicit => nothing to infer]
- The text says multiple times "unless the intended PST is RSVP-TE, in
which case it MAY omit the PATH-SETUP-TYPE TLV". This is inconsistent
with section 4: "It is RECOMMENDED that a PCEP speaker omits the TLV if
the PST is RSVP-TE." Please choose between MAY and SHOULD, and align.
------

Cheers,

Julien

---

From: Jonathan Hardwick <[email-masked]>
Date: Tue, 21 Nov 2017 18:02:12 +0000

Hi Julien

Many thanks for the speedy review!  Please see a few answers below, marked with [Jon].  (All other comments are accepted.)
I will hold the document mark-ups until WGLC ends.

Cheers
Jon

-----Original Message-----
From: Julien Meuric [mailto:[email-masked]] 
Sent: 21 November 2017 17:08
To: [email-masked]
Cc: [email-masked]
Subject: Shepherd's Review of draft-ietf-pce-lsp-setup-type-06

Hi,

Thank you for this new version of the I-D, it has greatly improved and clarifies former loose zones. Please find my review below.

------
Abstract
---
- s/traffic engineering paths (TE paths)/Traffic Engineering paths (TE paths)/
- I wonder about the expansion of "TE path" above: why not "Engineered"
instead of "Engineering"? (This is global to the I-D, and beyond... RFC Editor's list includes both.)
- s/label switched paths (LSPs)/Label Switched Paths (LSPs)/
------
Status
---
- "https://" was introduced in -05, but has now disappeared.
------
1. Introduction
---
- s/Path Computation Element Protocol/Path Computation Element communication Protocol/

- OLD
path setup type needs to be either explicitly indicated or implied in the appropriate PCEP messages (when necessary)...
   NEW
path setup type needs to be explicitly indicated in the appropriate PCEP messages, unless RSVP-TE type is meant (omission implying this type)...
------
3. Path Setup Type Capability TLV
---
- s/Initialization phase/initialization phase/
- I though the discussion on the list was about using a bitmap to identify supported PSTs: any reason why it is now a list of raw octets?

[Jon] We did discuss a bit field on the list.  The PST field is an 8-bit value, so a naive implementation of a bit field would use 32 bytes.  This seems wasteful given we only have two values defined so far (possibly 3 with the up-coming IPv6-SR draft).  I then looked at some schemes to shorten the bit field, e.g. by truncating it, but the result seemed more complicated to encode than just listing the path setup types.  (I also didn't much like having a PST known both by a value and by a bit field position.)  So I opted to list the PSTs.  IMO it's not a problem given the low number of PSTs we expect (could be a case of famous last words!)  What do you think of this tradeoff?

- The definition of length and padding mixes the words "octet" and "bytes", depending on the field (probably to due to text coming from RFC 5440). Consistency would be welcome (the comment appears to be applicable beyond this section).

[Jon] Let's standardize on bytes, per RFC 5440.

------
4. Path Setup Type TLV
---
- Figure 2 explicitly includes the codepoint for the "Type" (28), the "Length" field should be treated similarly (4).
- The last sentence of section 4 puzzles me. If the PATH-SETUP-TYPE TLV is not supported, the PCEP peer cares little if it was "recognized" or not. If both sub-cases are commonly handled by ignoring, an implementation always including the RSVP-TE PST will be able to interwork with an implementation knowing about the TLV without actually supporting it; the current text turns this situation into an error.
(Note also that RFC 5440 does not distinguish unrecognized and unsupported in TLV processing rules.)

[Jon] I think you are right. The same also applies to the final sentence of section 3, I believe.

- In case my previous comment does not fly, 3 more nits:
  * s/recognizes the TLV but does not support the TLV/recognizes the TLV but does not support its use/
  * s/send PCErr/send a PCErr message/
  * Error-Type is 2, would not 4 fit better?
------
5. Operation
---
- s/Initialization phase/initialization phase/
- s/MUST infer/MUST consider/  [explicit => nothing to infer]
- The text says multiple times "unless the intended PST is RSVP-TE, in which case it MAY omit the PATH-SETUP-TYPE TLV". This is inconsistent with section 4: "It is RECOMMENDED that a PCEP speaker omits the TLV if the PST is RSVP-TE." Please choose between MAY and SHOULD, and align.

[Jon] I think our intent is MAY, so we can reword the text in section 4 to "A PCEP speaker MAY omit the TLV if the PST is RSVP-TE."

------

Cheers,

Julien

---

From: Julien Meuric <[email-masked]>
Date: Thu, 23 Nov 2017 15:05:12 +0100

Hi Jon,

You are welcome. All open issues are about to get closed. Please see
[JM] below.

Thanks,

Julien

> [Jon] We did discuss a bit field on the list.  The PST field is an 8-bit value, so a naive implementation of a bit field would use 32 bytes.  This seems wasteful given we only have two values defin…

[JM] I do not have any strong opinion on this. If noone objects, it is
reasonable to go that way from where we stand.
Have you considered a more explicit figure, e.g. the following?

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|     PST#1     |     PST#2     |           Padding             |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
//                       (variable size)                       //
|                                                               |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

> [Jon] Let's standardize on bytes, per RFC 5440.

[JM] OK

> [Jon] I think you are right. The same also applies to the final sentence of section 3, I believe.

[JM] I agree. Good catch!

> [Jon] I think our intent is MAY, so we can reword the text in section 4 to "A PCEP speaker MAY omit the TLV if the PST is RSVP-TE."

[JM] OK