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

[Julien Meuric <[e-mail]>]

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