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 - pritchar...@gmail.com: > 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 _______________________________________________ Pce mailing list Pce@ietf.org https://www.ietf.org/mailman/listinfo/pce
