[Pce] Re: High level PCEP Review

Hi Adrian,

Thanks for the review, very useful comments. I skipped some addressed  
comments.

On Nov 14, 2006, at 1:18 PM, Adrian Farrel wrote:

> Hi,
>
> As discussed, it is time to review PCEP from outside the design   
> team. JP asked (well, OK, he begged :-) that at this review I  
> limit  myself to major protocol issues, and leave other  
> documentation issues to a later review. I have mainly done that,  
> but I have also included a few process-related comments that I  
> think you should
> resolve in the next revision.
>
> Cheers,
> Adrian
>
> ===
> Any reason why the Open, Close, and Keepalive messages do not   
> conform to the naming convention "PCXxxx" of all other messages?

Not really ... We just kept the Open, Close and Keepalive terminology  
as it is widely used in many protocols with the same meaning (other  
messages are PCEP specific).

> ===
> PCErr is missing from section 5.

Right, thanks, a section has been added.

> Indeed, the use of PCNtfy as shown in figure 5 to express   
> cancellation of a request by a PCE would seem to be the correct   
> usage of PCErr.
> I suggest a bit of a rationalisation (and perhaps the creation of  
> a  new message - PCCncl).
> Thus PCNtfy becomes a benign notification message, PCErr is used  
> by  the PCE to report an error (but not a failure), PCCncl is used  
> by  the PCC to cancel a PCReq.
> It is best if we can resist overloading the semantics of the   
> different messages.

This is a valid comment but we're afraid of seeing a message  
inflation ... After PCCncl, we could see PCCong (PCE Congestion) and  
many other ones, thus we'd prefer to keep the PCNtf as a generic  
mechanism to notify of various events. In term of semantic, the PCErr  
is exclusively used to report a protocol error condition. In the  
request cancellation case, the PCE experiencing congestion would  
notify the PCC that it had to cancel some requests.

> ===
> Section 5.2.5 would benefit from a statement that it is your   
> intention that the TCP connection can be re-established and  
> recover  the context of a session without further exchange of Open  
> messages.  I think this is what you intend.

We initially went in that direction but stepped back since this is  
probably not the best approach. Thus, we changed the text to specify  
that a TCP connection failure MUST trigger the PCEP session failure  
(clearing all related states), but that section had not been updated.  
Text updated now, thanks.

> I am not sure it is good, and  would prefer that the session was  
> also re-established with the  exchange of a list of PCReq I-Ds  
> still outstanding OR that a failed  TCP connection automatically  
> cleared the session.

Exactly, we chose the second option, we're in sync.

> Otherwise you  complicate the protocol and the implementation with  
> handling  duplicate request I-Ds and lost requests/responses.

Absolutely (for a very minor benefit).

> ===
> In section 6 you imply that a malformed PCErr will cause the   
> generation of a PCErr. I suggest you need to avoid this behavior  
> to  prevent duelling error messages!

We now differentiate the case of a PCEP message received with a  
missing object (in which case a PCErr message is sent with Error- 
Type=6) and the case of a malformed message that triggers the closing  
of the PCEP session with Reason=4.

> ===
> Can you include a reference to the definition of BNF in section 6
> ===

ISO 14977?

> In 6.1 you need to describe the default settings of the Flags and   
> Reserved fields

Indeed, added.

> ===
> Do you really support 2^32 byte messages? If this is a conscious   
> decision, then maybe OK.

We changed for 65K (16 bits), thus the header has been reduced from 8  
bytes to 4 bytes.

New Header:

0                   1                   2                   3
0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| Ver   |  Flags     |Message-Type|       Message- 
Lenght                  |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Ver: 3 bits
Flags: 5 bits
Message Type: 8 bits
Message Length: 16 bits

> ===
> 6.3 doesn't mention the use of keepalive to detect TCP connection   
> failure. I think it should.

I think that we should not mention the use of the PCEP Keepalive to  
detect a TCP failure though.
The Keepalive is really used to detect a PCEP session failure that  
may or may not be triggered by
a TCP failure. Thus, as stated in the document, it is used for PCEP  
session aliveness detection.
Agree ?

> ===
> In 6.4 the End-Points object is described as mandatory, but the  
> BNF  has it optional

Good catch, thanks. Fixed.

> ===
> In 6.7
>  The PCErr message may be sent by a PCC or a PCE in response to a
>  request or in an unsolicited manner.
> "may"?

Reworded: "The PCErr message is either sent by a PCC or a PCE in  
response to a request or in an unsolicited manner"

> ===
> In the objects you define reserved fields as MUST be set to zero.
> I think you need:
> - SHOULD be set to zero
> - MUST be ignored on receipt

Now specified (for all objects):

Reserved: This field MUST be set to zero on transmission and MUST be  
ignored on receipt.

> ===
> In 7.1 it is unclear, but I think the P flag would only have   
> meaning on a request, and the I flag on a response.
> So actually, you only need one flag (or you have some  
> clarification  text to write)

We could use a single bit, which would mean that the flag, say P is  
cleared for an optional object in a PCReq and set for an optional  
object that has been taken into account in a PCRep: so to interpret  
the bit we need to also remember the PCEP message type. Not a problem  
though ... Note that I was tempted to generalize the concept to the  
processing of an object in general (that could be in a PCReq or PCRep  
message) but without strong use case, it is not worth adding  
complexity. Let me know which option you prefer, I've got no  
preference (1 or 2 bits), well a slight preference for keeping two  
bits (easier to interpret).

> ===
> You will also need to explain what it means when a PCRep has the I   
> flag set for an object that had the P flag set on the PCReq. As   
> currently phrased this is an encoding error that would require a   
> PCErr, but that would clearly be a waste of time.

Yes, text has been added "The I flag has no meaning in a PCRep  
message when the P flag had been set in the corresponding PCRep  
message."

> ===
> In fact, why would you ever send a PCErr in response to a PCRep?

There are such cases. For example:

  6          Mandatory Object missing
                Error-value=1: RP object missing

> ===
> At the end of 7.1 you need to explain padding rules

Object are all aligned to 32-bits.

> ===
> Why does the Open need a protocol version number? This is in the   
> message header so we all know what protocol is being used/supported.
> in fact, I think you need to re-write the protocol version support   
> processing and negotiation.

The idea is to not set up a PCEP session if the supported PCEP  
version do not match.

> ===
> Dead timer.
> Should it be "SHOULD be set to zero, and MUST be ignored if the   
> keepalive timer is zero"

Don't you prefer to set it to 0 if the Keepalive is itself set to 0  
(less prone to misinterpretation)?

> ===
> The fields in the Open object are a mess!
> Can you bring the flags up with the version to improve the byte-  
> alignment?
> ===

Yes ! It was fixed in a revision post to rev-03.

> Note that in the message header the version is four bits but in  
> the  Open object it is three bits.
> ===

Fixed, thanks.

> In the Open object
>  SID (PCEP session-ID - 8 bits): specifies a 2 octet unsigned PCEP
>  session number that identifies the current session.
> Are you using bit compression?

Nope.

> ===
> Open object TLVs may be out of the scope of this doc (7.2) but the   
> format needs to be defined here.
> ===

mmm ... It is no different than future objects that may be carried in  
PCEP messages ?

>  When present in an Open message, the OPEN object specifies the
>  proposed PCEP session characteristics.  Upon receiving unacceptable
>  PCEP session characteristics during the PCEP session initialization
>  phase, the receiving PCEP peer (PCE) MAY include a PCEP object   
> within
>  the PCErr message so as to propose alternative session   
> characteristic
>  values.
> Isn't there an issue with duelling Open messages?
> You seem to have an independent Open in each direction, each of   
> which is responded with a PCErr, leading to a new Open in each   
> direction.
> Is it necessary to have this symmetry? After all, the TCP   
> connection is always initiated by the PCC, so can't we also have   
> the Open initiated by the PCC and responded with a PCErr or an  
> Open  by the PCE. Keepalives then follow.

We could but introducing such asymmetry also adds complexity to the FSM.

> ===
> 7.3
> Why is the P flag set in an RP object carried in a PCNtf or PCErr?   
> No computation action is required.

Good catch, it now reads: "The P flag of the RP object MUST be set in  
PCReq and PCReq messages and MUST be cleared in PCNtf and PCErr  
messages."

> ===
> Why is there a reserved piece of the RP object? Is there a   
> difference between the unused flags bits and the reserved field?
> ===

Clarified.

> 7.3.1
>  Note that it is not required for a PCE to
>  support the priority field: in this case, it is RECOMMENDED to set
>  the priority field to 0 by the PCC in the RP object.
> Why would it matter what value is set if it is not supported by  
> the  PCE?

Just a recommendation ... if the PCC learns via IGP discovery that  
the PCE does not support the priority field, it
is desirable to have it set to 0. But the information may not be  
available of course, thus the recommendation. Another way to discover  
whether this is supported is for the PCC to set it to <>0, and have  
the PCE reset the field. Note that these are recommendations, not  
required.

> Wouldn't it possibly be helpful if the PCE forwards the request to   
> another PCE?

I think that this should be exclusively triggered by the PCC (after  
discovering the non support of prioritization)
and is application specific but having the PCE redirecting the  
requesting to another PCE itself may lead to
undesirable consequences.

> ===
> 7.3.1
> As described here there is no difference between the R and the F   
> bit. Both are used to recompute a path allowing re-use of  
> resources  already used by the path. Presumably in the case of the  
> F bit, some  information about the failure is also supplied through  
> an  exclusion, but this is not mentioned. The behavior at the PCE  
> for  these two bits would seem to be identical so unless you can  
> give  better separation of the meanings, I suggest you merge the bits.

You're right that the text is not explicit. In the case of the F bit,  
the information can be used by the PCE to exclude some links should  
the updated IGP LSA be received after the path computation request.  
As discussed on the list, let's move that bit to the XRO draft.  
Section removed.

> ===
> 7.3.1
> The description of the O-bit is very confusing.
> I think the problem lies with the definition of "explicit path"
> Loose means that it is acceptable to include loose hops
> You have no term to distinguish the use of abstract nodes that   
> contain more than one network node
> Explicit just means an ERO
> Strict means that only strict hops are allowed

There were some typos/issues in the text indeed ... New Text: "O  
(strict/lOose - 1 bit): when set, in a PCReq message, this indicates  
that a loose path is acceptable. Otherwise, when cleared, this  
indicates to the PCE that a path exclusively made of strict hops is  
required. In a PCRep message, when the O bit is set this indicates  
that the returned path is a loose path, otherwise (the O bit is  
cleared), the returned path is made of strict hops."

> ===
> 7.3.1
> You need to think about how the Request ID is handled when the PCC   
> restarts. Are you requiring the PCC to have persistent memory to   
> avoid re-use of a Request ID? (That is how the text reads!)

Yes, an implementation issue where the PCC may either tear down the  
session upon restart or checkpoints.

> ===
> 7.3.2
> The description of R-bit handling looks broken.
> The previous setting of the O-bit is not important because the PCC   
> is able to use RRO to collect information from the network. Thus,   
> the question is actually about how detailed the RRO is. The point   
> you are making remains (that it is hard to compute a proper make-  
> before-break resource-sharing path if you don't know the previous   
> path. [Those of you looking at the inter-area/AS PCE problem space   
> will recognise this as impossible to solve unless stateful PCEs  
> are  used or the PCE is provided with a way to issue an enquiry to  
> the  ABR/ASBR.]

I don't think that the definition is broken.
Few cases here:
1) Provide the RRO with strict hops and set the R flag to avoid  
double bw booking
2) If the PCC does not know the RRO with all strict hops, provides  
what it knows + set the R flag
In all cases the PCC MUST provide the RRO which may contain loose  
hops (in which case
double bandwidth booking cannot be guaranteed).

> ===
> 7.3.2
>  If the PCC receives a PCRep message that contains a RP object
>  referring to an unknown Request-ID-Number, the PCC MUST send a PCErr
>  message with Error-Type="Unknown request reference".
> This feels like at most a "MAY". Why is there a requirement to   
> respond?

Because actions are very likely required on the PCE, thus we mandate  
the PCC to respond
so that the PCE detects the issue. Makes sense ?

> ===
> 7.4
> I don't think the description on the inclusion of "failed"   
> constraints in the PCRep message is in agreement with the BNF in   
> section 6.5. The BNF appears to imply that an ERO must be present   
> in order to include, for example, a Metric object.
> [By the way: I continue to believe that this business of reporting   
> the failed constraints is extremely dubious. Where there are two   
> absolute constraints to be satisfied, it is often impossible to   
> state which of them led to the failure to compute a path.]

In that case, the PCE may either to decide to indicate none of them  
or both. But the case where only one constraint is not satisfied is  
by itself a clear motivation for this feature (pretty common  
situation). Not sure to see the issue with the BNF since the path- 
list is optional ?

> ===
> 7.4
> How does a PCC tell the difference between a PCRep that contains   
> the "failed" constraints echoed from the PCReq, and modified   
> constraints that could have been satisfied?

Just by comparing the original values sent by the PCC to the values  
returned by the PCE: if they differ, suggestions are provided by the  
PCE.

> From the text it  appears that you intend the C-bit to make this  
> distinction, but the  description of the C-bt does not say this.

The C bit is set when the PCE indicates the set of non satisfied  
constraints.

> ===
> 7.4
> Wouldn't it be a good idea to put a reason code in the No-Path object?

Yes this was added to a post rev-03 revision (NO-PATH-VECTOR TLV).

> ===
> 7.5
> Can you add a note to say where the end points of the LSP *are*   
> encoded.
> That is, although this PCReq is computing a segment only, it is   
> important (at least for policy) to know the LSP end points as well.

As pointed out in the text "The END-POINTS object is used in a PCReq  
message to specify the source IP address and the destination IP  
address of the path for which a path computation is requested. Note  
that the source and destination addresses specified in the END-POINTS  
object may or may not correspond to the source and destination IP  
address of the TE LSP but rather to a path segment." In the second  
case, the LSP end-points are not provided. If an application requires  
this information (as you said for policy, .. ) it should be defined  
in another ID.

> ===
> 7.6
> First paragraph is hard to parse, but I think it says that the   
> Bandwidth object must always be present in a PCReq! But the second   
> paragraph explains what it means if it is absent.

OK so I reworded it:

"
The BANDWIDTH object is used to specify the requested bandwidth for a  
TE LSP.
If the requested bandwidth is equal to 0, the BANDWIDTH object is  
optional. Conversely, if the requested bandwidth is non equal to 0,  
the PCReq message MUST contain a BANDWIDTH object.
In the case of the reoptimization of a TE LSP, the bandwidth of the  
existing TE LSP MUST also be included in addition to the requested  
bandwidth if and only if the two values differ. Consequently, two  
Object-Type are defined that refer to the requested bandwidth and the  
bandwidth of the TE LSP for which a reoptimization is being performed.
"

> ===
> 7.6
> Can you give a references to the IEEE FP definition.

Sounds pretty well-known, no ?

> ===
> 7.7
> Please add a third metric: hop count

Good idea, added.

> I think you need to allow the Metric object to be present multiple   
> times, and you have this in the text, but it is missing in the BNF.

Good catch. I also fixed the BNF for the PCReq message (same purpose)

> On the other hand, you need to describe how to interpret multiple   
> Metric objects for the same Type setting

Text added "There MUST be at most one instance of the METRIC object  
for each metric type. In two or more instances of a METRIC object are  
present for a metric type, only the first instance MUST be considered  
and other instances MUST be ignored."

> ===
> 7.7
> Why do you need the B-bit? The metric value is set to zero with  
> the  same semantic.

This is because the metric-value is used to report the metric of a TE  
LSP for a reoptimization request. Thus, we need the B-bit to indicate  
whether the metric-value corresponds to the metric of the TE LSP to  
be reoptimized or to a bound. That said, I'm realizing that this was  
not written down and the text had to be reworded. New text:

"A METRIC object used to indicate the metric to optimize during the  
path computation MUST have the B-Flag cleared and the T-Flag set to  
the appropriate value. When the path computation relates to the  
reoptimization of an exiting TE LSP (in which case R-Flag of the RP  
object is set) an implementation MAY decide to set the metric-value  
field to the cost of the TE LSP to be reoptimized with regards to a  
specific metric type.)"

I also fixed a few other issues in the paragraph.

> ===
> 7.7
> The text appears to say that only one Metric object may be present   
> with the B-flag cleared. Why?

I think that we should relax that constraint indeed. *That said*, how  
should the PCE interpret the request to optimize a path based on  
Metric-1 and Metric-2 ? Without further indication (hierarchical  
constraint, weighting function, ...) this has no meaning. Proposal:  
we relaxed the constraint but the text required to fully cover those  
aspects will be covered in another ID, as discussed months ago on the  
list. OK ?

> ===
> 7.8 and 7.9 (and everywhere)
> A real nit, but when you write "ERO object" or "RRO object" do you   
> mean EROO and RROO? :-)

;-) Fixed ... sorry for the tautology

> ===
> 7.8
> You say that the ERO is bounded by 3209, 3473 and 3477 which is   
> fine, but how will an MPLS-TE LSR handle an ERO with an explicit   
> label in it? You also allow future extensions to subobjects.
> I think this means that the PCReq must include a way for the   
> requester to limit the subobjects that will be returned.

Do you think that we should limit these objects ? Indeed some ERO sub- 
objects may be defined in the future that might be of interest ?

> ===
> 7.9
> RRO is also used for reporting the path of a failed LSP?

See comment above, this will be covered in the XRO ID.

> ===
> 7.10
> What are optional TLVs for?
> Perhaps to carry the additional attributes as defined in RFC4420?

Yes I added a reference.

> ===
> 7.10
> Why do you need an LSPA object without resource affinities? This  
> is  pointless. It only exists in 3209 for backwards compatibility.

Absolutely ! Text modified to define one object (w/ affinity).

> ===
> 7.10
> What is the meaning of holding priority with respect to path   
> computation?

This can be taken into account by the PCE when computing the path(s).

> ===
> 7.10
> Why are the Flags and Reserved fields different sizes in the two   
> objects?

Only one object now.

> ===
> 7.10
> When computing a path to satisfy a request with the L-bit set, how   
> will the PCE know which links are protected with FRR?

This can be obtained from draft-ietf-isis-link-attr for example, or  
locally available in a database.

> ===
> 7.11
> Good to see the IRO
> Why is there no XRO?
> Exclusion is a standard part of the MPLS-TE MIB (RFC3812 see   
> mplsTunnelHopInclude). If the exclusion can be configured on the   
> headend LSR, I think we need to be able to compute the path.
> Besides, this is a requirement in RC4657.

Covered in draft-oki-pce-pcep-xro-00.txt

> ===
> 7.11
> Why is it necessary to list the subobjects for the IRO but not for   
> the ERO or the RRO?

We listed them since the IRO is a new object by contrast with the ERO  
and RRO for which we re-use existing object formats (we just define a  
PCEP codepoint).

> ===
> 7.12.1
> I urge caution in the use of "simultaneously" in this section.

Agree since this can never be true ;-) Reworded: "Independent versus  
dependent path computation requests: path computation requests are  
said to be independent if they are not related to each other.  
Conversely a set of dependent path computation requests is such that  
their computations cannot be performed independently of each other (a  
typical example of dependent requests is the computation of a set of  
diverse paths)."

> ===
> 7.12.1
> At the end of this section, you suddenly introduce the term   
> "bundle". I think this is supposed to refer to the presence of   
> multiple requests in a single PCReq message. Note that the SVEC  
> can  apply across messages. It would, perhaps be better to explain  
> that  requests in the same message are not necessarily associated  
> by an  SVEC, and that requests in separate messages may be  
> associated by  an SVEC.

This is detailed in 7.12.3.

> ===
> 7.12
> You are missing text to describe the use of the SVEC on a PCRep   
> (allowed according to 6.5).
> It feels like you would only use SVEC in the case of an RP, but  
> the  BNF has them as independent.
> Please clarify

BNF has been fixed, there's no reason to have the SVEC in PCRep, thanks.

> ===
> 7.12.2
> You have MUST NOT for each of your diversity characteristics.
> I think you also need "desired" so that you can get "as much   
> diversity as possible".
> How will you handle the overlap between (for example) node and  
> link  diversity? Just needs some text to explain.

Yes, this is a topic that we discussed a few times, we could define a  
"desired" bit along with various constraint relaxation strategy but  
this would lead to adding much complexity. So we decided to keep the  
simple rule here and add more functionality in a separate ID. The  
overlap case is quite simple here: in this example, link two LSP are  
link *and* node diverse they cannot have any link/node in common.  
Node diverse => link diverse, not the opposite of course.

> ===
> 7.12
> Is it possible to end up with a nasty mesh of synchronized   
> requests? Are there any rules to resolve this?

Good implementation ;-) (thus the timer-based approach suggested),  
but this is implementation-specific.

> ===
> 10
> The state machine is quite hard to parse for reviewing purposes.  
> It  would be good to at least name the arrows on the figure. My   
> personal preference is to draw the FSM as a grid because this  
> shows  up missing state/event combinations far more easily.
>
> Is the OpenRetry counter just a substate of OpenWait?
>
> The description of OpenRetry is at odds with the error used (1:5).   
> The error says a second unacceptable Open has been received, but   
> the text says that OpenRetry is incremented if the Open is  
> anything  except malformed. Can I suggest that the use of a  
> separate state  and not a substate would help you resolve this type  
> of issue.
>
> LocalOK also looks like a substate. Ditto RemoteOK. Please don't   
> use substates, they are evil!

YEs, they are not sub-state but variables.

> The flip flop between OpenWait and  KeepWait caused by these two  
> variables and the negotiation of  parameters is ghastly.
>
> I think that what the state machine shows us is that we do not  
> need  independent Open messages in each direction. Anyway, if we  
> *do*  need them, we need correct states to manage them.

We'll need a piece of paper ... In prague ..

> ===
> 10
>  If a malformed PCEP message is received or the TCP connection fails,
>  the systems sends a PCEP CLOSE message, the system releases the PCEP
>  resources for that PCEP peer, closes the TCP connection and moves to
>  the Idle State.
> This seems to be in contradiction with text in the main body of  
> the  draft and with the existence of error code 6.

Yes we decided to close the TCP connection upon receiving a malformed  
packet.
Not sure which section you're referring to here ?

> ===
> App A
> Is this appendix just present to help you track the requirements   
> you still need to satisfy, or do you intend to leave some   
> requirements unanswered?

This was requested a few IETF meetings ago to keep track of the  
requirements to be satisfied. All unanswered MUST requirements will  
be listed there. Text updated.

>
> BTW, I don't think this is a full list. Route exclusions is not   
> listed.

Reference added.

>
> What are you doing to ensure that the list is complete?
> ===
> I think Kenji's email address may have changed
> Same for Jean-Louis

Thanks!

JP.