Re: [RTG-DIR] RtgDir review: draft-ietf-pce-pce-initiated-lsp-09

[Julien Meuric <[e-mail]>]

Hi Vicky,

Thank you for your review and this helpful feedback. We'll make sure
they're addressed in the next revision.

Cheers,

Julien


Apr. 06, 2017 - [e-mail]:
> Hello,
>
> I have been selected as the Routing Directorate reviewer for this
> draft. The Routing Directorate seeks to review all routing or
> routing-related drafts as they pass through IETF last call and IESG
> review, and sometimes on special request. The purpose of the review is
> to provide assistance to the Routing ADs. For more information about
> the Routing Directorate, please see
> ​http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir
>
> Although these comments are primarily for the use of the Routing ADs,
> it would be helpful if you could consider them along with any other
> IETF Last Call comments that you receive, and strive to resolve them
> through discussion or by updating the draft.
>
>
> Document: draft-ietf-pce-pce-initiated-lsp-09.txt 
> Reviewer: Victoria Pritchard
> Review Date: 06 April 2017
> IETF LC End Date:  
> Intended Status: Standards Track
>
>
> Summary: 
> I have some minor concerns about this document that I think should be
> resolved before publication.
>
>
> Comments:
> Although I was not very familiar with PCEP, I found the draft for the
> most part easy to understand, but did need to look up some things in
> the referenced documents and was unclear on a couple of small points.
> I have some suggestions that may help improve the draft for other
> readers, and I have some queries which may require clarification in
> the document. However, as most readers will be more familiar with the
> subject, perhaps not all comments will require any action.
>
>
> Major Issues:
> No major issues found.
>
>
> Minor Issues and Nits:
>
> Section 1, 1st paragraph
> Path Control Element / Path Computation Element ?
>
> Section 1, 2nd paragraph
> Stateful pce / Stateful PCE
> Reference link [I-D.ietf-pce-stateful-pce] points to section 3.1, not
> the references section. 
> The 2nd sentence was hard to read, could be split into two sentences.
>
> Section 2
> Last paragraph: Routing Backus-Naur Format / Routing Backus-Naur Form,
> to match the RFC title.
>
> Section 3.1
> At the end of the 1st paragraph, "possible agile software-driven
> network operation" is then repeated in the next paragraph as "A
> possible use case is a software-driven network"
>
> Section 3.2
> The acronyms SRP, PLSP and ERO are used a few times in this section.
> It may well be OK to assume most readers will be familiar with these,
> but would be good to have the expansion here too.
>
> Section 3.2, 3rd paragraph
> SRP-id-number / SRP-ID-number, for consistency
> The sentence beginning "The PCE MAY update" could be moved to a new
> paragraph, to separate it from the text regarding instantiation.
>
> Section 3.2, last paragraph
> Suggest to replace the "and" with a comma in this sentence: 
> "During State Synchronization, a PCC
>    reports the state of its LSPs to the PCE using PCRpt messages and
>    setting the SYNC flag in the LSP Object. "
> "include the Create Flag" / "set the Create Flag" - also the create
> flag has not yet been mentioned.
> Actually I think this overview could be much briefer and simpler.
> There is a lot of detail about objects, flags and options, which is
> explained in later sections but complicates this overview. I think it
> might be good to also summarise here what the extension adds in terms
> of messages and flags, to clearly indicate what's new compared to the
> referenced documents.
>
> Section 4
> After the first sentence, I would rephrase: "First, the Open message
> must include the Stateful PCE Capability TLV, defined in []." Then
> continue to the sentence beginning "A new flag is introduced in this
> TLV, ...".
>
> Section 4.1 
> In the flag bit description, "that the PCE may attempt to instantiate
> LSPs" could be changed to "that the PCE supports instantiating LSPs". 
> Also rather than "in order to support", use "in order to enable".
> Not sure this section is necessary in this form with Figure 1. For
> example, I think the sync-optimizations draft specifies flags in a
> nice way (Section 7 of that draft), suggesting the bit to use without
> a diagram. The description here in section 4.1 could be rolled into
> the main body of Section 4. Also, is there any need to mention the U
> flag or the S flag in this draft?
>
> I noticed a mix and match between terminology of
> "STATEFUL-PCE-CAPABILITY TLV" vs "Stateful PCE Capability" TLV - could
> be made consistent, I'd suggest "Stateful PCE Capability TLV"
> throughout for readability.
> Also the same applies to "Path Computation LSP Initiate Message",
> "Path Computation LSP Initiate Request", "LSP Initiate Message", "LSP
> Initiate Request", "LSP initiation request". Would be nice to see this
> consistent throughout.
>
> Section 5.1
> The 1st paragraph could be simplified by removing text about other
> objects and missing objects, and moving the final two sentences into
> sections 5.3 and 5.4.
> The 2nd paragraph states "The format ... for LSP instantiation", but
> this looks like it applies to deletion too. Suggest to remove "for LSP
> instantiation".
> On first read (although most readers will be familiar already) I would
> have liked to see some mention of what the Common Header is, maybe
> even just a reference to its definition in RFC5440. 
>
> Section 5, final paragraph 
> I would suggest you dont need the 3rd sentence at all, as correlation
> is already mentioned in the 1st sentence in this paragraph.
> Also, is SRP-ID-number incremented when an operation is requested
> *from* the PCE? Or *by* the PCE, or in either direction? Is it clearer
> to say "The PCE increments the current PCEP session's SRP-ID-number
> before including it in the PCInitiate message" (assuming any other
> usage is unchanged from the Stateful PCE draft)? 
>
> Section 5.2 
> This section could be condensed in a similar way as I mentioned before
> regarding the I flag in the Capability TLV in Section 4.1. The text
> could be included at the bottom of section 5.1, and there is no need
> to draw the SRP object. Also no need for the reference as it's already
> in section 5.1. 
> Perhaps also state the alternative case, that if the flag is set to 0,
> it indicates an instantiation request.
>
> Section 5.3, 1st paragraph 
> "LSP instantiation is done by" / "The LSP is instantiated by". 
> "an PCInitiate" / "a PCInitiate"
> Suggest removing the sentence beginning "The LSP is set up"
>
> Section 5.3 in general
> I suggest reorganising this section:
> -First discuss message contents that should be included for
> instantiation, i.e., objects mentioned in the format section above,
> and their contents.
> -Then once you have defined what the PCInitiate should look like, in
> new paragraph(s) talk through checking validity of the PCInitiate
> (non-zero PLSP-ID and missing ERO or SYMBOLIC-PATH-NAME) and discuss
> the error messages.
> -Then use the text describing LSP setup based on the info included.
> -Then discuss the PCRpt. You currently mention PCRpt in a couple of
> places in this section and it would be easier to read if it was in one
> place. For clarity, also state that in the PCRpt, both the Delegate
> and Create flags are in the LSP object.
>
> Section 5.3, 8th paragraph. "The PCEP-ERROR object SHOULD include the RSVP
>    Error Spec TLV (if an ERROR SPEC was returned to the PCC by a
>    downstream node)."
> Is that already covered by the 1st sentence in this paragraph, "relay
> to the PCE errors it encounters"? Could re-phrase to "If an RSVP Error
> Spec TLV was returned to the PCC by a downstream node, it should be
> included in the PCEP-ERROR object in the PCErr message". Also would
> prefer not to use 2 terms "RSVP Error Spec TLV" and "ERROR SPEC".
> suggeseted / suggested
> Is the sentence "After the LSP is set up, errors in RSVP..."
> necessary? By that I mean does that behaviour differ from normal, is
> it particular to this extension?
>
> Section 5.3, paragraphs 8 and 9
> Would you want to inform the PCE of any limits during the capability
> exchange rather than sending an error later and ignoring further PCE
> requests?
>
> Section 5.3.1
> The create flag could be described earlier. As mentioned above,
> Section 4 would be a good place to detail all the bits newly defined
> in this draft, the new message, the new flags. Again, I dont think you
> need to draw a diagram, just describe the flag added and its position
> within the LSP Object.
>
> Section 5.3.2 could be rolled into the description of the create flag
> since the two would be used together. Also, back in Section 3 it said
> you SHOULD include the SPEAKER-IDENTITY-ID TLV, whereas 5.3.2 instead
> uses MAY. SPEAKER-IDENTITY-ID is not actually defined in the
> sync-optimizations draft - assume you mean SPEAKER-ENTITY-ID? Also
> just to make it clear, you are re-using that TLV but this time within
> the LSP Object, and to give the PCE's identity, rather than in the
> OPEN object to give the speaker's identity?
> Also in the final sentence: "the TLV MUST be ignored the and the PCE
> MUST send a PCErr" - there's an extra "the" in the middle, and being
> very fussy, the TLV is not really ignored if you send an error
> message. Also if you do send the error message, is the rest of the
> PCRpt message ignored?
>
> Section 5.4 may benefit from splitting into multiple paragraphs, one
> for each error type, plus another for the final part beginning at
> "Following the removal".
>
> Section 6, 1st paragraph
> "are automatically delegated": suggest this reads "MUST be delegated".
> Automatically might imply you dont need to do anything to make this
> happen.
> If the PCE returns a delegation to the PCC, would the PCC then end up
> sending that PCE a PCRpt with the delegation bit set to zero? The
> first paragraph states that this is an error, but in that case, would
> it be?
>
> As "Redelegation Timeout Interval" and "State Timeout Interval" are
> both terms defined in the Stateful PCE draft, I would try to use the
> exact same terminology and same capitalisation found there throughout. 
>
> Section 6, 3rd paragraph.
> Where it says "In case of PCEP session failure", does that mean
> failure at any point in time, or just a failure after the PCE has
> returned delegation in order to transfer the LSP to a different PCE?
> Also, is the LSP considered an orphan as soon as the initiating PCE
> returns delegation to the PCC? Or only if a PCEP session fails? Having
> these two bits of information in two different paragraphs makes it
> seem like they are separate but I would think they go together? If I
> have interpreted this correctly, I would suggest the following text:
>    A PCE MAY return a delegation to the PCC to allow for LSP transfer
> between
>    PCEs. The PCC will also regain control over a PCE-initiated LSP if
> the PCEP session
>    fails and the Redelegation Timeout Interval timer expires.  In both
> cases, the
>    LSP is considered an "orphan" and the PCC MUST trigger the State
> Timeout Interval 
>    timer for that LSP ([I-D.ietf-pce-stateful-pce]). 
> But, what is not clear to me, is what is happening at this point to
> try to delegate to another PCE? From looking at the Stateful PCE
> draft, I believe the PCC would send a report to 1/any(?) PCE it was
> connected to, setting the delegate flag to 1 to try to get that PCE to
> accept delegation. If I interpreted that correctly, i.e. the PCC
> actively tries to switch delegation to another PCE, it might be worth
> stating that here.
> Assuming a reply comes in from that PCE, with the delegate flag set,
> within the state timeout interval, all is good. However, I was
> slightly concerned by the statement from the Stateful PCE draft that says:
> "If the PCE accepts the LSP Delegation,
>    it MUST set the Delegate flag to 1 when it sends an LSP Update
>    Request for the delegated LSP (note that this may occur at a later
>    time)." 
> I dont know how far in the future "a later time" could be, but if the
> State Timeout Interval expires at the PCC first, wont the LSP get flushed?
> The text also says that to obtain control, a PCE can send a
> PCInitiate. So as an alternative to my initial interpretation, does
> that mean the PCC could advertise the LSP with delegate flag set to
> zero, and wait for a PCE to take control by sending a PCInitiate as
> described?
> This also makes me wonder how/when the initial PCE would decide to
> give up control? Is that in scope for this draft?
>
> Section 6, final paragraph
> The explanation about the timeout sounds like it applies to Stateful
> PCE in general, and therefore may not be necessary to explain in this
> draft? Also, on PCE crash (bearing in mind paragraph 3 above), does
> the Redelegation Timeout Interval occur first, and then the State
> Timeout Interval?
>
> Section 8.1
> Meaning: Initiate. Being really picky, I would like this to match the
> full term used in this draft "Path Computation LSP Initiate Request"
> (or whatever term you settle on - see comment above between Section
> 4.1 and 5.1). This would then match RFC5440's way of doing it for PCReq.
>
>
> Kind regards,
> Victoria