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

[Robert Varga <[e-mail]>]

Hello,

please find my comments on the pending items.

On 10/26/2015 10:10 PM, Ina Minei wrote:
> 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] <mailto:[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_
>
>

[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

In this case PCRpt differs from PCReq. The PCE needs to know the ERO 
object for each LSP, as may have been pushed by a different PCE. 
Reporting RRO is not sufficient, as that contains the effective LSP 
path, e.g. with loose hops expanded by the PCC. That is why ERO is a 
mandatory object in PCRpt (as part of intended_path), hence we specify 
an empty object for the end-of-sync marker.
>
>     - 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

The association between a PCEP session and PCE processes is something 
which I would consider an internal PCE detail, and it should be covered 
by the next sentence (e.g. raise PCErr 20/1).

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

I agree, this is an extension, so implementations should reuse RFC5440 
errors when appropriate.

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

It is not, as that would be seen as a request to modify the LSP setup to 
empty. Such an acknowledgement would have to include full configuration 
as previously reported -- which would be handled as a normal update.

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

It could, if we want to be strict about it, but it does not really have 
an impact on protocol operation: delegate=0 should kick in first, which 
means the PCC can safely discard any extra payload.
>
>     - s/<ERO><attribute-list>/<RRO><attribute-list>/ [Per RFC 5440, a
>     report from PCC to PCE is RRO.]
>
> ### XXX Pending
>
>     - 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

The idea here is that ERO contains the path as pushed by the PCE. It may 
contain loose hops, which the PCC can expand as it sees fit. The RRO 
contains the effective path the LSP is currently taking, e.g. any loose 
hops are resolved. Since a backup PCE is not required to share state 
with the primary PCE, and there is no way to derive ERO from RRO, the 
PCEP session needs to communicate both, so a backup PCE can pick up the 
previous PCE's policy decision as well as the current LSP path.
>
>     - 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

Current wording is based on the assumption that the PCE has to have a 
consistent point-in-time view of the PCC's state. In this regard a PCRpt 
of a new LSP which exceeds PCE implementation-internal limit on the 
number of LSPs it supports would break that assumption, hence we chose 
PCErr. This makes it consistent with what would happen if that LSP is 
reported during initial state resynchronization.

>     - 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 MUST is there to maintain a single global identifier for the LSP. 
PLSP-ID is then used as a shorthand. I do not recollect the exact 
reasoning as to why the TLV can be in SRP, as the placement and 
semantics of that TLV has changed quite a bit over the past couple of 
years. If I were to venture a guess, I think it was retrofitted to allow 
the PCE to update the symbolic path name.

Thanks,
Robert