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

[Ina Minei <[e-mail]>]

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 <[e-mail]>
wrote:

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

### Done


> ------
> Section 1
> ---
> - s/Path Computation Element Protocol/Path Computation Element
> communication Protocol/
>

### 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/Stateful PCE: has/Stateful PCE: a PCE that has/
> - s/Passive Stateful PCE: uses/Passive Stateful PCE: a stateful PCE that
> uses/
>
### Done

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

- s/Delegation: An/Delegation: an/
> - s/PCC who owns/PCC which owns/
> - s/PCC SHOULD be/PCC should be/
> - s/Revocation: An/Revocation: an/
>
### Done

>
> - 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.
>
### 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 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.
>
### Done


> - 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.
>
### 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/
> - s/the same PCEP./the same protocol, i.e., PCEP./
>
### done



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


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

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

------
> 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]
>
### Done


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


> - 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/
>
### Done


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


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

### XXX Pending


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


> - 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?
>
### XXX Pending




> - When mentioning errors, adding a sentence reminding that RFC 5440
> already defines a set of applicable error codes would be valuable.
>
### XXX Pending



> - 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").
>
### Done. Good point, moved.


> - s/PCE may return/PCE MAY return/
> - s/PCE may revoke/PCE MAY revoke/
> - 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
> acknowledgement: to be clarified.
>
### XXX Pending


> - 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...".
>
### 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.


> - 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?
>
### XXX Pending


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

- s/as recommended/as RECOMMENDED/ or s/as recommended/as MANDATORY/
> [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"


> - Again, in section 5.5.5, 4th paragraph, after "there will be no change
> 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/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/
>
### 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/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/
>
### 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/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.]
>
### Added reference


> - s/in a separate draft/in a separate document/
> - s/Transport/PCEP Sessions/
>
### Done

> ------
> 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.
>
### Done.


> - s/<ERO><attribute-list>/<RRO><attribute-list>/ [Per RFC 5440, a report
> from PCC to PCE is RRO.]
>
### 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.
> - "SRP" is still not defined.
>
### Isn't it enough to reference the section in which it is defined?


> - s/is optional/is OPTIONAL/
> - s/the latest one/the one with the highest ID-number/
> - 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/that the PCE associates/that the PCC associates/
> - s/next LSP. If the PCUpd/next LSP.<lineBreak>If the PCUpd/
> ------
>
### Done

> Section 7
> ---
> - 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/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:/
>
### Done


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


> - s/the S flag/The S flag/
> - 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


> - 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]
>
### Done

- s/in l following/in the following/
>
### 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


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

### XXX  Pending

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

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

>
> - 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.
>
### Done


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


> - Again in section 10.4, the "resource limit exceed" information
> (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 :-)

> ------
> Section 12
> ---
> - For this document, I think the RFC 5226 reference is informative, not
> normative (an implementer does not need to read it).
>
### done


> ------
>
> Best regards,
>
> Julien
>
>