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
_______________________________________________
Pce mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/pce
