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

[Ina Minei <[e-mail]>]

All the changes discussed in this thread have been incorporated in version
13 published yesterday

https://tools.ietf.org/html/draft-ietf-pce-stateful-pce-13

Thank you,

Ina

On Sun, Nov 29, 2015 at 1:21 PM, Ina Minei <[e-mail]> wrote:

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