Subject: [Pce] Chair's Review of draft-ietf-pce-stateful-pce-11

From: Julien Meuric <[email-masked]>
Date: Fri, 16 Oct 2015 16:40:40 +0200

Dear authors,

To prepare the upcoming move to the IESG, please find below my review of 
the aforementioned I-D (at last!).

_Summary_

Main qualities:
- core specification is clear;
- wording is smooth, with very few typos;
- the manageability and security sections have been included with 
relevant text.
Main issues to point out:
- RFC 2119 keywords (picky me, but so is the IESG: ask JP about 5440);
- a few corner/error cases;
- consistency with existing RFCs (5440, 5886).
I also take the opportunity to remind the WG that including codepoint 
values from existing registries before allocation by the IANA (which can 
be requested early) is a very bad idea, whatever the WG.

_Detailed Comments_
------
Header
---
- The blank line after Ed's name looks strange. If no affiliation is 
wanted, his name may be moved to the bottom of the list, to avoid 
association with one of the listed companies. Otherwise, you may use a 
generic phrase, e.g. "Individual Contributor".
------
Section 1
---
- s/Path Computation Element Protocol/Path Computation Element 
communication Protocol/
------
Section 2
---
- "LSP", properly expended in the introduction, should be added 
somewhere (all the more as IS-IS is mentioned a few times).
- s/Stateful PCE: has/Stateful PCE: a PCE that has/
- s/Passive Stateful PCE: uses/Passive Stateful PCE: a stateful PCE that 
uses/
- s/Active Stateful PCE: is an extension/Active Stateful PCE: an extension/
- s/Delegation: An/Delegation: an/
- s/PCC who owns/PCC which owns/
- s/PCC SHOULD be/PCC should be/
- s/Revocation: An/Revocation: an/

- OLD
Redelegation Timeout Interval: when a PCEP session is terminated, a PCC 
waits for this time period before revoking LSP delegation to a PCE and 
attempting to redelegate LSPs associated with the terminated PCEP 
session to an alternate PCE.
   NEW
Redelegation Timeout Interval: the period of time a PCE waits for, when 
a PCEP session is terminated, before revoking LSP delegation to a PCE 
and attempting to redelegate LSPs associated with the terminated PCEP 
session to an alternate PCE.

- OLD
State Timeout Interval: when a PCEP session is terminated, a PCC waits 
for this time period before flushing LSP state associated with that PCEP 
session and reverting to operator-defined default parameters or behaviors.
   NEW
State Timeout Interval: the period of time a PCE waits for, when a PCEP 
session is terminated, before flushing LSP state associated with that 
PCEP session and reverting to operator-defined default parameters or 
behaviors.

- OLD
Within this document, PCE-PCE communications are described by having the 
requesting PCE fill the role of a PCC. This provides a saving in 
documentation without loss of function.
   NEW
Within this document, PCEP communications are described through PCC-PCE 
relationship. The PCE architecture also supports the PCE-PCE 
communication, by having the requesting PCE fill the role of a PCC, as 
usual.
------
Section 3
---
- Section 3.1.3 includes 2 paragraphs "Scale and Performance": they 
should either be merged, or the old text version should be dropped.
- s/which prevents interoperability of a PCE/which limits 
interoperability of a stateful PCE/
- s/the same PCEP./the same protocol, i.e., PCEP./
------
Section 4
---
- s/A PCE requests/a PCE requests/
- The reference to draft-sivabalan-pce-disco-stateful makes the reader 
wonder why is is not part of draft-ietf-pce-stateful-pce itself. 
Besides, Table 1 also mentions IS-IS and OSPF PCE-CAP-FLAGS sub-TLV, 
only the allocation section seems to be missing here. Would you talk to 
the authors (some being common to both I-Ds) of the former to consider 
using their material as a contribution? A separate document is quite an 
overhead for allocating 2 bits.
------
Section 5
---
- s/In the PCEP protocol/In PCEP/
- s/The PCEP protocol extensions/The PCEP extensions/
- s/by the operator through/by the operator, e.g., through/
- "PCRpt message can contain": what about "s/can/MAY/"?
- s/ISIS/IS-IS/
- s/The PCEP protocol extensions/The PCEP extensions/
- s/it SHOULD generate a PCErr/it MUST generate a PCErr/  [twice]
- s/it will terminate/it MUST\SHOULD terminate/  [twice]
- s/in this document MAY/in this document may/
- s/SHOULD be generated/MUST be generated/
- s/PCE can still receive/PCE can still accept/
- PLSP-ID is used, while not yet defined nor expended.
- s/include an empty ERO/include an empty RRO/ [Along with RFC 5440 
(section 7.10), the object sent by a PCC to report to a PCE is an RRO: 
let us keep it consistent.]
- s/in its path/for its path/  [or "as its path"?]
- Avoiding "positive acknowledgements for properly received 
synchronization messages" has scalability benefits in normal situations, 
but the PCC is blind and may keep on sending PCRpt to dead processes 
behind up PCEP sessions. Have you consider acknowledgement, possibly 
using a compression mechanism like the one defined later in the I-D?
- When mentioning errors, adding a sentence reminding that RFC 5440 
already defines a set of applicable error codes would be valuable.
- The sentence about "delegation policy" in section 5.5 is not protocol 
specification and should be moved to section 9.1 ("Control Function and 
Policy").
- s/PCE may return/PCE MAY return/
- s/PCE may revoke/PCE MAY revoke/
- s/PCC should react/PCC SHOULD react/  [?]
- In section 5.5.1, it is not clear if an empty LSP Update Request with 
a Delegate flag to 1 is an acceptable way for a PCE to send a delegation 
acknowledgement: to be clarified.
- Section 5.5.1 has a significant break in the middle, it would deserve 
subsections: e.g., "5.5.2.1 Revoking Explicitly" and "5.5.2.2. Revoking 
on Timeout" right before "When a PCC's PCEP session with a PCE 
terminates unexpectedly...".
- s/the PCC SHALL flush/the PCC MUST flush/  [correct, but single 
instance of SHALL in the I-D]
- Current text says "The State Timeout Interval SHOULD be greater than 
or equal to the Redelegation Timeout Interval", but the I-D assumes 
"MUST": if "MUST" is avoided on purpose, then some text should be added 
for cases not falling under that "SHOULD".
- s/SHOULD return the LSP delegation/MUST return the LSP delegation/
- In section 5.5.3, assuming an LSP was delegated, does the reception by 
the PCC of a non empty LSP Update Request with a Delegate Flag to 0 
trigger an error?
- s/to be larger/to be greater/
- s/as recommended/as RECOMMENDED/ or s/as recommended/as MANDATORY/  
[depending on the SHOULD/MUST selected above]
- At the top of page 18, "and [assuming that] PCEs' decisions are the 
same" should be added.
- Again, in section 5.5.5, 4th paragraph, after "there will be no change 
in LSP state", "if similar PCEs' decisions" should be added.
- s/is larger than/is greater than/
- Section 5.6 requires more RFC 2119 keywords, only a few of them are 
suggested below.
- s/For each LSP, it sends/For each LSP, it MAY send/
- s/could not be set up, the PCC sends/could not be set up, the PCC MUST 
send/
- s/it also sends/it SHOULD also send/
- s/A PCC sends each LSP State Report/A PCC MUST send each LSP State Report/
- In section 5.6.2, in case of new LSP, the very first message sent by 
the PCC aims at getting a path. This is clearly the role of a PCReq 
message, and the I-D extends it to support the LSP object including the 
delegation flag (section 7.3). However, Figure 8 treats a new LSP the 
same way as reporting an existing LSP, i.e., using a PCRpt message. In 
this case, there is an overlap between PCReq and PCRpt, which MUST be 
resolved because interoperability is at stake. Documenting the 
delegation of a new LSP deserves some new text and possibly figure, the 
overlapping specification of the PCRpt should be explicitly precluded.
- s/existing LSPs) and LSPs have/existing LSPs). After LSPs have/
- s/For each LSP, it sends/For each LSP, it MAY send/
- s/LSP is up, the PCC sends/LSP is up, the PCC MUST send/
- s/be set up, the PCC sends/be set up, the PCC MUST send/
- s/PCC may choose to compress LSP State/PCC MAY compress LSP State/
- s/A PCC sends each LSP State/A PCC MUST send each LSP state/
- At the end of section 5.6, "SRP-ID-number" is used but not yet 
defined. At least, SRP should be expended.
- s/PCC may choose to compress state/PCC MAY compress state/
- s/the earlier updates/the updates with lower SRP-ID-number/ [Noting 
that it assumes that the value is incrementing, which is documented only 
later, in section 7.2.]
- s/in a separate draft/in a separate document/
- s/Transport/PCEP Sessions/
------
Section 6
---
- The terms "mandatory" and "optional" are introduced, but barely used 
then. It would be better to stick to RFC 2119 keywords (e.g., "REQUIRED" 
and "OPTIONAL") and actually use them for each object.
- The object ordering is defined as mandatory: unfortunately, that would 
be inconsistent with previous PCEP extensions. I suggest to either drop 
that sentence, or at least downgrade to a "SHOULD" as a trade-off.
- s/<ERO><attribute-list>/<RRO><attribute-list>/ [Per RFC 5440, a report 
from PCC to PCE is RRO.]
- The need for the optional xRO is not well documented. Its proper 
naming will depend on the associated use that must be clarified.
- "SRP" is still not defined.
- s/is optional/is OPTIONAL/
- s/the latest one/the one with the highest ID-number/
- s/is mandatory/is REQUIRED/
- The use of the optional xRO is mentioned, but its relationship with 
the RRO (formerly ERO) is not clear. I suspect some assumptions are made 
on the way the ERO/RRO are populated; RFC 5440 only says ERO for 
PCE->PCC and RRO for PCC->PCE.
- s/included by the PCC/inlcuded in PCRpt by the PCC/
- The behavior associated to the resource limit per PCC rather looks 
like a Notifcation than an Error (e.g., in RFC 5440, cancelling a set of 
pending requests relies on PCNtf). Please consider the use of 
Notification instead of Error here.
- s/that the PCE associates/that the PCC associates/
- s/next LSP. If the PCUpd/next LSP.<lineBreak>If the PCUpd/
------
Section 7
---
- s/defined in this document/defined in that (aforementioned) document/
- s/SHOULD always be/MUST be/
- s/one SPR-id-number/one SRP-ID-number/
- s/SRP-ID/SRP-ID-number/  [3 times]
- s/allocated it/allocated to it/
- s/Flags (12 bits):/Flags (12 bits), starting from the least 
significant bit:/
- s/on a PCRpt message/On a PCRpt or PCReq message/
- s/the S flag/The S flag/
- s/in other PCRpt messages/in other messages/
- "PCE SHOULD remove all state" is written 3 times: I wonder about "MUST".
- s/LSP Identifiers/LSP-IDENTIFIERS/  [4 or 5 times]
- Texts associated with object definition are incomplete, they rely too 
much on figures. For example:
     * s/The type of the TLV/The type (16 bits) of the TLV/  [5 times]
     * s/IANA and it has a fixed length of/IANA. The length field is 16 
bit-long and has a fixed value of/  [3 times]
     * s/IANA and it has a variable length/IANA. The length field is 16 
bit-long and has a variable value/  [2 times]
- s/in l following/in the following/
- The paragraph "However, when Gobal Path Protection... (or vice versa)" 
is not IPv6 specific and should be moved to the end of section 7.3.1.
- s/a path's lifetime/an LSP's lifetime/
- It would be nice to elaborate on the reason why the SYMBOLIC-PATH-NAME 
MUST be included and not SHOULD.
- I do not see why SYMBOLIC-PATH-NAME may be included in SRP Object: 
defining the LSP Object as its single place seems enough and much simpler.
------
Section 8
---
- OLD
This document requests that a registry is created to manage the Flags 
field of the LSP object.
   NEW
This document requests that a new sub-registry, named "LSP ObjectFlag 
Field", is created within the "Path Computation Element Protocol (PCEP) 
Numbers" registry to manage the Flag field of the LSP object.

- either s/Error-value=9: ERO Object/Error-value=9: xRO Object/  or add 
an Error-Value for RRO.
- As mentioned above, in Error-Type 19, Error-value 4 should be 
considered in a PCNtf.

- OLD
This document requests that a registry is created to manage the Flags 
field in the STATEFUL-PCE-CAPABILITY TLV in the OPEN object.
    NEW
This document requests that a new sub-registry, named 
"STATEFUL-PCE-CAPABILITY TLVFlag Field", is created within the "Path 
Computation Element Protocol (PCEP) Numbers" registry to manage the Flag 
field in the STATEFUL-PCE-CAPABILITY TLV of the PCEP OPEN object (class 
= 1).

- OLD
This document requests that a registry is created to manage the value of 
the LSP error code field in this TLV.
   NEW
This document requests that a new sub-registry, named "LSP-ERROR-CODE 
TLV Error Code Field", is created within the "Path Computation Element 
Protocol (PCEP) Numbers" registry to manage the LSP Error code field of 
the LSP-ERROR-CODE TLV.
------
Section 10
---
- s/session also result in/session also results in/
- Again in section 10.4, the "resource limit exceed" information 
(Error-value 4 of Error-Type 19) should be considered in a PCNtf.
------
Section 11
---
- Cyril and Ramon might be more than acknowledged since they are 
mentioned twice.
------
Section 12
---
- For this document, I think the RFC 5226 reference is informative, not 
normative (an implementer does not need to read it).
------

Best regards,

Julien

---

From: Ina Minei <[email-masked]>
Date: Mon, 26 Oct 2015 14:10:59 -0700

Julien,

Thank you for the detailed review, please find answers inline below ###.  I
have incorporated the overwhelming majority of the comments, explained the
reason for not incorporating a couple of them, and am still working with
the co-authors on a couple of items marked "pending", which we will close
on shortly.

Two questions and one ask
1. Forward references to SRP object and SRP-ID - there are several in the
comments, though the relevant section is always mentioned. How should such
forward references be addressed?
2. Section 7 - s/defined in this document/defined in that (aforementioned)
document/
The comment was not clear to me. The intention is for the flags to be set
as explained for the new objects we are defining here, can you clarify the
comment?
3. Can you please review the comments that were not incorporated and let us
know if you agree?

Thank you,

Ina

On Fri, Oct 16, 2015 at 7:40 AM, Julien Meuric <[email-masked]>
wrote:

> - the manageability and security sections have been included with relevant
> text.
> values from existing registries before allocation by the IANA (which can be
> requested early) is a very bad idea, whatever the WG.
>
> _Detailed Comments_
> ------
> Header
> ---

### Done

> - s/Path Computation Element Protocol/Path Computation Element

### Done

> ------
> Section 2
> ---
> - "LSP", properly expended in the introduction, should be added somewhere
> (all the more as IS-IS is mentioned a few times).
>
### Added in terminology, with reference to 3031.

> - s/Passive Stateful PCE: uses/Passive Stateful PCE: a stateful PCE that
### Done

> - s/Active Stateful PCE: is an extension/Active Stateful PCE: an extension/
 ### Replaced as "an Active Stateful PCE that may issue recommendations...

> - s/Revocation: An/Revocation: an/
### Done

> attempting to redelegate LSPs associated with the terminated PCEP session
> to an alternate PCE.
>   NEW
> Redelegation Timeout Interval: the period of time a PCE waits for, when a
> PCEP session is terminated, before revoking LSP delegation to a PCE and
> attempting to redelegate LSPs associated with the terminated PCEP session
> to an alternate PCE.
>
### Done

>
> - OLD
> State Timeout Interval: when a PCEP session is terminated, a PCC waits for
> this time period before flushing LSP state associated with that PCEP
> session is terminated, before flushing LSP state associated with that PCEP
### Done

> relationship. The PCE architecture also supports the PCE-PCE communication,
> by having the requesting PCE fill the role of a PCC, as usual.
>
### Done

> ------
> Section 3
> ---
> - Section 3.1.3 includes 2 paragraphs "Scale and Performance": they should
> either be merged, or the old text version should be dropped.
>
### Done, new text below:

 o  Scale & Performance: configuration operations often have
      transactional semantics which are typically heavyweight and often
      require processing of additional configuration portions beyond the
      state being directly acted upon, with corresponding cost in CPU
      cycles, negatively impacting both PCC stability LSP update rate
      capacity.

> - s/which prevents interoperability of a PCE/which limits interoperability
> of a stateful PCE/
### done

> - s/A PCE requests/a PCE requests/
### Done

> wonder why is is not part of draft-ietf-pce-stateful-pce itself. Besides,
> Table 1 also mentions IS-IS and OSPF PCE-CAP-FLAGS sub-TLV, only the
> allocation section seems to be missing here. Would you talk to the authors
> (some being common to both I-Ds) of the former to consider using their
> material as a contribution? A separate document is quite an overhead for
> allocating 2 bits.
>

### I don't think discovery should be part of the base draft. Several
reasons: a) 5440 does not include discovery, b) the base spec is already
quite large, we want to keep only the core function and  c) the discovery
draft is expired. I cleaned up table 1 and added the following text instead
of the reference to the draft:

Old text

[I-D.sivabalan-pce-disco-stateful] defines the extensions needed to
support autodiscovery of stateful PCEs when using OSPF ([RFC5088]) or
IS-IS ([RFC5089]) for PCE discovery.

New text
Similarly to [RFC5440], no assumption is made about the discovery
method used by a PCC to discover a set of PCEs (e.g., via static
configuration or dynamic discovery) and on the algorithm used to
select a PCE.  Extensions needed to support autodiscovery will be
defined in a separate document.

------
> - s/it SHOULD generate a PCErr/it MUST generate a PCErr/  [twice]
### Done

> - s/it will terminate/it MUST\SHOULD terminate/  [twice]
### I replaced with SHOULD

> - s/PCE can still receive/PCE can still accept/
### Done

> - PLSP-ID is used, while not yet defined nor expended.
### Added reference

> (section 7.10), the object sent by a PCC to report to a PCE is an RRO: let
> us keep it consistent.]

### XXX Pending

> - s/in its path/for its path/  [or "as its path"?]
### Done

> but the PCC is blind and may keep on sending PCRpt to dead processes behind
> up PCEP sessions. Have you consider acknowledgement, possibly using a
> compression mechanism like the one defined later in the I-D?
>
### XXX Pending

> already defines a set of applicable error codes would be valuable.
### XXX Pending

> specification and should be moved to section 9.1 ("Control Function and
### Done. Good point, moved.

> - s/PCC should react/PCC SHOULD react/  [?]
### Done

> - In section 5.5.1, it is not clear if an empty LSP Update Request with a
> Delegate flag to 1 is an acceptable way for a PCE to send a delegation
### XXX Pending

> subsections: e.g., "5.5.2.1 Revoking Explicitly" and "5.5.2.2. Revoking on
> Timeout" right before "When a PCC's PCEP session with a PCE terminates
> unexpectedly...".
>
### Done

> - s/the PCC SHALL flush/the PCC MUST flush/  [correct, but single instance
> of SHALL in the I-D]
>
### Done

> - Current text says "The State Timeout Interval SHOULD be greater than or
> equal to the Redelegation Timeout Interval", but the I-D assumes "MUST": if
> "MUST" is avoided on purpose, then some text should be added for cases not
> falling under that "SHOULD".
>
### Done

> - s/SHOULD return the LSP delegation/MUST return the LSP delegation/
### This should remain SHOULD. The nice way to do it is to return it
explicitly, but it may choose to wait until the next update and return the
delegation then by not setting the delegate flag.

> the PCC of a non empty LSP Update Request with a Delegate Flag to 0 trigger
> an error?
>
### XXX Pending

> - s/to be larger/to be greater/
### Done

> [depending on the SHOULD/MUST selected above]
### Done

- At the top of page 18, "and [assuming that] PCEs' decisions are the same"
> should be added.
### The comment is not clear - I am assuming you refer to the text
"assuming that the state timeout... ' on page 17, changed to: " and
assuming that the primary and redundant PCEs take similar decisions, this"

> in LSP state", "if similar PCEs' decisions" should be added.
### Done "and assuming that the standby PCE takes similar decisions as the
failed PCE"

> - s/A PCC sends each LSP State Report/A PCC MUST send each LSP State
### Done

> - In section 5.6.2, in case of new LSP, the very first message sent by the
> PCC aims at getting a path. This is clearly the role of a PCReq message,
> and the I-D extends it to support the LSP object including the delegation
> flag (section 7.3). However, Figure 8 treats a new LSP the same way as
> reporting an existing LSP, i.e., using a PCRpt message. In this case, there
> is an overlap between PCReq and PCRpt, which MUST be resolved because
> interoperability is at stake. Documenting the delegation of a new LSP
> deserves some new text and possibly figure, the overlapping specification
> of the PCRpt should be explicitly precluded.
>
### This is historical confusion in the figure, from the time when
initiation was rolled up in the same document. I modified the figure so it
is clear this is for updating parameters only.

> - s/A PCC sends each LSP State/A PCC MUST send each LSP state/
### Done

> - At the end of section 5.6, "SRP-ID-number" is used but not yet defined.
> At least, SRP should be expended.
>
### It was referenced and explained earlier in 5.6.

> - s/the earlier updates/the updates with lower SRP-ID-number/ [Noting that
> it assumes that the value is incrementing, which is documented only later,
> in section 7.2.]
>
### Added reference

> - s/Transport/PCEP Sessions/
### Done

> that sentence, or at least downgrade to a "SHOULD" as a trade-off.
### Done.

> - s/<ERO><attribute-list>/<RRO><attribute-list>/ [Per RFC 5440, a report
### XXX Pending

> - The need for the optional xRO is not well documented. Its proper naming
> will depend on the associated use that must be clarified.
### Isn't it enough to reference the section in which it is defined?

> - s/is mandatory/is REQUIRED/
### Done

> - The use of the optional xRO is mentioned, but its relationship with the
> RRO (formerly ERO) is not clear. I suspect some assumptions are made on the
> way the ERO/RRO are populated; RFC 5440 only says ERO for PCE->PCC and RRO
> for PCC->PCE.
>
### XXX Pending

> - s/included by the PCC/inlcuded in PCRpt by the PCC/
### Done

> - The behavior associated to the resource limit per PCC rather looks like
> a Notifcation than an Error (e.g., in RFC 5440, cancelling a set of pending
> requests relies on PCNtf). Please consider the use of Notification instead
> of Error here.
>
### XXX Pending

> - s/next LSP. If the PCUpd/next LSP.<lineBreak>If the PCUpd/
### Done

> - s/defined in this document/defined in that (aforementioned) document/
### Unclear comment, the intention was for the flags to be set as explained
for the new objects in this document, not for anything in 5440.

> - s/Flags (12 bits):/Flags (12 bits), starting from the least significant
> bit:/
>
### Done

> - s/on a PCRpt message/On a PCRpt or PCReq message/
### We don't support delegation through a PCReq message

> - s/in other PCRpt messages/in other messages/
### Done

> - "PCE SHOULD remove all state" is written 3 times: I wonder about "MUST".
### SHOULD is the correct operation, the PCE is free to choose to keep
state around for as long as it wishes. The 3 removal statements refer to
different scenarios.

> - s/LSP Identifiers/LSP-IDENTIFIERS/  [4 or 5 times]
### Done

> bit-long and has a variable value/  [2 times]
### Done

### Done

- The paragraph "However, when Gobal Path Protection... (or vice versa)" is
> not IPv6 specific and should be moved to the end of section 7.3.1.
>
### Done. Also added a MUST keyword

> - s/a path's lifetime/an LSP's lifetime/
### Done

> defining the LSP Object as its single place seems enough and much simpler.

### XXX  Pending

> Numbers" registry to manage the Flag field of the LSP object.
### Done

>
> - either s/Error-value=9: ERO Object/Error-value=9: xRO Object/  or add an
> Error-Value for RRO.
>
### Done

> - As mentioned above, in Error-Type 19, Error-value 4 should be considered
> in a PCNtf.
>
### XXX Pending

> field in the STATEFUL-PCE-CAPABILITY TLV of the PCEP OPEN object (class =
> 1).
>
### Done

> This document requests that a new sub-registry, named "LSP-ERROR-CODE TLV
> Error Code Field", is created within the "Path Computation Element Protocol
> (PCEP) Numbers" registry to manage the LSP Error code field of the
> LSP-ERROR-CODE TLV.
>
### Done

> - s/session also result in/session also results in/
### Done

> (Error-value 4 of Error-Type 19) should be considered in a PCNtf.
### XXX Pending

> Section 11
> ---
> - Cyril and Ramon might be more than acknowledged since they are mentioned
> twice.
>
### Done :-)

> normative (an implementer does not need to read it).
### done