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

[Jonathan Hardwick <[e-mail]>]

Hi Victoria

I'm picking up this thread and replying as PCE working group chair, as the authors are unavailable.  We very much appreciate your review, which has been very helpful, and I sincerely apologise for the delay in addressing your comments.

Please see my proposed resolutions inline below, marked with "Jon>".  I have attached a new revision of the draft which, I believe, will resolve all the points you raised.

Best regards
Jon


From: Victoria Pritchard [mailto:[e-mail]]
Sent: 06 April 2017 22:28
To: [e-mail]
Cc: [e-mail]; [e-mail]; [e-mail]
Subject: RtgDir review: draft-ietf-pce-pce-initiated-lsp-09

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 ?

Jon> OK.


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.

Jon> OK.


Section 2
Last paragraph: Routing Backus-Naur Format / Routing Backus-Naur Form, to match the RFC title.

Jon> OK


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"

Jon> OK


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.

Jon> OK, except I don’t think PLSP has an official expansion in draft-ietf-pce-stateful-pce, it’s just a name.

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.

Jon> OK

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.

Jon> I agree and have simplified this section as you suggested.  Here is the new text.

NEW
3.2.  Operation Overview

   This document defines the new I flag in the STATEFUL-PCE-CAPABILITY
   TLV to indicate that the sender supports PCE-initiated LSPs (see
   details in Section 4.1).  A PCC or PCE sets this flag in the Open
   message during the PCEP Initialization Phase to indicate that it
   supports the procedures of this document.

   This document defines a new PCEP message, the LSP Initiate Request
   (PCInitiate) message, which a PCE can send to a PCE to request the
   initiaton or deletion of an LSP.  The decision when to instantiate or
   delete a PCE-initiated LSP is out of the scope of this document.

   The PCE sends a PCInitiate message to the PCC to request the
   initiation of an LSP.  The PCC creates the LSP using the attributes
   communicated by the PCE and local values for any unspecified
   parameters.  The PCC generates an LSP State Report (PCRpt) for the
   LSP, carrying a newly assigned PLSP-ID for the LSP and delegating the
   LSP to the PCE via the Delegate flag in the LSP object.

   The PCE can update the attributes of the LSP by sending subsequent
   PCUpd messages.  Subsequent LSP State Report (PCRpt) and LSP Update
   Request (PCUpd) messages that the PCC and PCE, respectively, send for
   the LSP will carry the PCC-assigned PLSP-ID, which uniquely
   identifies the LSP.  See details in Section 5.3.

   The PCE sends a PCInitiate message to the PCC to request the deletion
   of an LSP.  To indicate a delete operation, this document defines the
   new R flag in the SRP object in the PCInitiate message, as described
   in Section 5.2.  As a result of the deletion request, the PCC removes
   all state related to the LSP and sends a PCRpt for the removed state.
   See details in Section 5.4.


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

Jon> OK

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?

Jon> I’ve made the changes to the text suggested above.  Some drafts do duplicate the TLV figure when adding new fields.  It would probably not have been my personal preference here but I’m inclined to leave it as-is.  I think the text referring the reader to other documents for the U and S bits should stay to make it clear they are already defined elsewhere.

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.

Jon> Thanks, I’ve gone for the following:

·        STATEFUL-PCE-CAPABILITY TLV to match the definition in draft-ietf-pce-stateful-pce

·        “LSP Initiate Request (PCInitiate) Message” abbreviated to “PCInitiate Message”


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.

Jon> OK


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

Jon> I’ve refactored this section to address these comments.

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.

Jon> Kept the diagram as noted above but made the other clarifications.


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"

Jon> OK.  We need to keep the text specializing the draft to RSVP-TE.


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.

Jon> Done

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?

Jon> Fixed the terminology and typo. I think the rest of it reads fine given the general overhaul I did in this section.

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?

Jon> There might be value in adding a capability, but I would prefer to leave it as an enhancement to be done if implementations find it necessary.


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.

Jon> I don’t think this is worth refactoring as there are forward references where needed.


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?

Jon> Fixed the TLV name (well spotted!), the SHOULD (which should be MAY) the stray “the” and the “TLV” being ignored, which should say “LSP”.
Your understanding of how this TLV is used is correct.
The entire LSP is ignored, but multiple LSPs may be requested in a single PCInitiate, so others may still be processed.



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

Jon> Done.


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.

Jon> OK


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?

Jon> Correct – I’ve fixed the text.


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.

Jon> Done


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?

Jon> Failure at any point in time.

Also, is the LSP considered an orphan as soon as the initiating PCE returns delegation to the PCC?

Jon> Yes, this was the intention.

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

Jon> Looks good – I’ve used this text with a few changes.


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.

Jon> Yes, the PCC is allowed to do that if the Redelegation Interval Timer expires.

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?

Jon> The PCE implementation needs to acknowledge soon enough that the State Interval Timer does not expire – but that is also true of the base draft, so is not really appropriate to discuss in this draft.

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?

Jon> You are correct, both the PCRpt and PCInitiate mechanisms could be used.

This also makes me wonder how/when the initial PCE would decide to give up control? Is that in scope for this draft?

Jon> The draft says “to allow LSP transfer between PCEs.”  This might be done for a variety of reasons – I don’t think the draft needs to list them.


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?

Jon> I think it’s worth keeping.

Also, on PCE crash (bearing in mind paragraph 3 above), does the Redelegation Timeout Interval occur first, and then the State Timeout Interval?

Jon> Yes, that’s correct.

Jon> I did a significant rewrite of section 6 as a result of your comments.  Here is the new text in its entirety.

NEW
6.  LSP Delegation and Cleanup

   The PCC MUST delegate PCE-initiated LSPs to the PCE upon
   instantiation.  The PCC MUST set the delegation bit to 1 in the PCRpt
   that includes the assigned PLSP-ID.

   The PCC MUST NOT revoke the delegation for a PCE-initiated LSP on an
   active PCEP session.  Therefore, all PCRpt messages from the PCC to
   the PCE that owns the delegation MUST have the delegation bit set to
   1.  If the PCE that owns the delegation receives a PCRpt message with
   the delegation bit set to 0 then it MUST send a PCErr message with
   Error-type=19 ("Invalid Operation") and Error-value=7 ("Delegation
   for PCE-initiated LSP cannot be revoked").  The PCE MAY further react
   by closing the session.

   Control over a PCE-initiated LSP can revert to the PCC in two ways.
   A PCE MAY return a delegation to the PCC to allow for LSP transfer
   between PCEs.  Alternatively, the PCC gains control an LSP if the
   PCEP session that it was delegated on fails and the Redelegation
   Timeout Interval timer expires.  In both cases, the LSP becomes an
   orphan until the expiration of the State Timeout Interval timer
   ([I-D.ietf-pce-stateful-pce]).

   The PCC MAY attempt to redelegate an orphaned LSP by following the
   procedures of [I-D.ietf-pce-stateful-pce].  Alternatively, if the
   orphaned LSP was PCE-initiated, then a PCE MAY obtain control over
   it, as follows.

   A PCE (either the original or one of its backups) sends a PCInitiate
   message, including just the SRP and LSP objects, and carrying the
   PLSP-ID of the LSP it wants to take control of.  If the PCC receives
   a PCInitiate message with a PLSP-ID pointing to an orphaned PCE-
   initiated LSP, then it MUST redelegate that LSP to the PCE.  Any
   other non-zero PLSP-ID MUST result in the generation of a PCErr
   message using the rules described in Section 5.4.  The State Timeout
   Interval timer for the LSP is stopped upon the redelegation.  After
   obtaining control of the LSP, the PCE may remove it using the
   procedures described in this document.

   The State Timeout Interval timer ensures that a PCE crash does not
   result in automatic and immediate disruption for the services using
   PCE-initiated LSPs.  PCE-initiated LSPs are not removed immediately
   upon PCE failure.  Instead, they are cleaned up on the expiration of
   this timer.  This allows for network cleanup without manual
   intervention.  The PCC SHOULD support removal of PCE-initiated LSPs
   as one of the behaviors applied on expiration of the State Timeout
   Interval timer.  The behavior SHOULD be picked based on local policy,
   and can result either in LSP removal, or in reverting to operator-
   defined default parameters.



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.

Jon> Done


Kind regards,
Victoria