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

[Julien Meuric <[e-mail]>]

Hi Ina,

Following our fruitful discussing in Yokohama, please find [JM]'s note 
below. Just a few items are pending, I hope you will soon find an 
agreement with your co-authors.

WG, if you disagree with the proposed change about discovery bit 
allocation below, please let us know.

Julien


Oct. 26, 2015 - [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?
[JM] A simple way is to add the acronym and its expansion into the 
terminology section, and optionally the forward reference.

> 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?
[JM] Just suggesting a rewording, but my proposal was not clear either; 
"this current I-D" may be enough to clear the ambiguity.

> 3. Can you please review the comments that were not incorporated and 
> let us know if you agree?
[JM] See below.
>
> Thank you,
>
> Ina
>
> On Fri, Oct 16, 2015 at 7:40 AM, Julien Meuric 
> <[e-mail]> wrote:
<snip>
>
>     - s/Active Stateful PCE: is an extension/Active Stateful PCE: an
>     extension/
>
>  ### Replaced as "an Active Stateful PCE that may issue 
> recommendations...
>
[JM] Guessing you actually meant "a Stateful PCE that may...", it is OK.

<snip>
>
>     - 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
[JM] I hope you caught my (double) typo: I was only trying to rephrase, 
the waiting party remaining the PCC.

<snip>
>
>     - 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.
[JM] Looks fine to me.

<snip>
>
>     - 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 
> tosupport 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.
[JM] Along with RFC 5557 and 6006, including discovery bit allocation 
with PCEP extensions in a single document would save much administrative 
processing.

<snip>
>
>     - 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
[JM] As discussed, clarifying that you encode the "intended path" using 
ERO (class 7) and "actual path" using RRO (class 8) will address my 
comments related to xROs.

<snip>
>
>     - 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
>
<snip>
>
>     - 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
<snip>
>
>     - 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.
[JM] As discussed together, I can live with that.

>     - 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
<snip>
>
>     - 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"
  [JM] You got my point. :-)

<snip>
>
>     - 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.
  [JM] Seems addressed. Just to be confirmed when published and to 
confront with draft-ietf-pce-pce-initiated-lsp.

<snip>
>
>     - s/<ERO><attribute-list>/<RRO><attribute-list>/ [Per RFC 5440, a
>     report from PCC to PCE is RRO.]
>
> ### XXX Pending
[JM] As above.

>     - 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?
  [JM] Once added in terminology section, all these are solved.

<snip>
>
>     - 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
[JM] As above.

<snip>
>
>     - 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
[JM] To be updated.

<snip>
>
>     - 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.
[JM] As above: adding "current" may be enough.

<snip>
>
>     - s/on a PCRpt message/On a PCRpt or PCReq message/
>
> ### We don't support delegation through a PCReq message
[JM] Addressed as part of the resolution of the new path delegation.

<snip>
>
>     - "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.
  [JM] I can live with that.

<snip>
>
>     - 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
<snip>
> - As mentioned above, in Error-Type 19, Error-value 4 should be 
> considered in a PCNtf.
> ### XXX Pending
<snip>
>
>     - 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
<snip>