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

[Ina Minei <[e-mail]>]

Thanks a lot for the very useful discussions and for the continued review.
Please see inline %%%, a new version of the draft will be published next
week,

Ina

On Tue, Nov 10, 2015 at 9:48 AM, Julien Meuric <[e-mail]>
wrote:

[snip]

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


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

%%% Done

[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.
>
%%% Yes :-)

[snip]

>
>>
>>
>>
>>     - 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.
>
> %%% I did now :-), fixed.

[snip]

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


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


> <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
>>
> %%%  No, we did not think we needed positive acks because we are using a
TCP session (guaranteed delivery all the way to the PCE).


>>     - When mentioning errors, adding a sentence reminding that RFC
>>     5440 already defines a set of applicable error codes would be
>>     valuable.
>>
>> ### XXX Pending
>>
> %%% Done see text below:
"Error reporting is done using the procedures defined in [RFC5440], and
reusing the applicable error types and error values of [RFC5440] wherever
appropriate. The current document defines new error values  for several
error types to cover failures specific to stateful PCE."
>
>
>> <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
>>
> %%% Yes, it is allowed, modified the text to clarify.


> <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
>>
> %%% No, this is a delegation return. I cleaned up the text, see below.

In order to keep a delegation, a PCE MUST set the Delegate flag to 1
   on each LSP Update Request sent to the PCC.  A PCE that no longer
   wishes to update an LSP's parameters SHOULD return the LSP delegation
   back to the PCC by sending an empty LSP Update Request which has the
   Delegate flag set to 0.  If a PCC receives a non-empty LSP Update
   Request with the Delegate flag set to 0, it MUST treat this as a
   delegation return.



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

%%% Done

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


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

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


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



> <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
>>
> %%% The inclusion in SRP was a historical leftover and is removed.
Regarding why it MUST be included in the LSP object is described in the
draft in section 7.3.2. It is a stable (across PCEP session, PCC restart,
..etc), signaling protocol agnostic, and unique Identifier that identifies
the LSP during its lifetime.


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

%%% 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
>>
> <snip>
>
%%% Done