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

From: Victoria Pritchard <[email-masked]>
Date: Thu, 6 Apr 2017 22:28:27 +0100

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

---

From: Julien Meuric <[email-masked]>
Date: Fri, 7 Apr 2017 12:15:07 +0200

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 - [email-masked]:
> 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.
>
>
> 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
> Reference link [I-D.ietf-pce-stateful-pce] points to section 3.1, not
> the references section. 
> 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.
>
> "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". 
> 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. 
>
> 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 
> 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.
>
> -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 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.
>
> 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?
>
> 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
> 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. 
>
> 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:
>    PCEs. The PCC will also regain control over a PCE-initiated LSP if
> the PCEP session
>    LSP is considered an "orphan" and the PCC MUST trigger the State
> Timeout Interval 
> 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:
> 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?
>
> 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