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

[Victoria Pritchard <[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