Subject: [Pce] draft-ietf-pce-monitoring-01.txt

From: Adrian Farrel <[email-masked]>
Date: Fri, 08 Aug 2008 12:40:10 +0100

Hi,

In Dublin, JP indicated that the authors of this I-D thought it was pretty much 
ready for working group last call, so here is my review as WG chair before we get 
into the business of a last call.

I think there are sections still needing to be filled in.

Cheers,
Adrian

===
idnits (http://tools.ietf.org/tools/idnits/\) shows 1 error and 7 warnings all in 
the references sections.
===
The I-D is currently Standards Track, I am slightly nervous about this given the 
current implementaiton status and the fact that you are defining substantial new 
messages and procedures. Have you considered Experiemental status?

Conversely, I’m worried that this I-D defines the PCE-ID object that is used by 
some instances in BRPC.
===
Abstract
s/where a domain is referred to as/where a domain referrs to/
s/PCE-based environments/PCE-based environments,/
s/states, statistics/states, and statistics/
===
1.  Terminology
Please re-order sections so that Section 1 is the Introduction.
This will mean that you need to expand all of the acrinyjms in the Introduction.

On the other hand...

   AS: Autonomous System.
Not used in the document.

   LSR: Label Switching Router.
Not used in the document.

   TED: Traffic Engineering Database.
Not used in the document.
===
2.  Introduction
s/where a domain is referred to as/where a domain referrs to/
s/states, statistics/states, and statistics/

   This document specifies procedures and extensions to the Path
   Computation Element Protocol (PCEP) ([I-D.ietf-pce-pcep]) in order to
   monitor the path computation chain and gather various performance
   metrics.
I think you need to introduce the concept of a “path computation chain” before 
talking about monitoring it. I would simply move this paragraph to after the next 
paragraph, but even then the term is not clearly explained. There is a good 
paragraph in section 3.1.

s/The aim of this document is to specify/This document specifies/

s/metrics collection can be gathered/metrics can be gathered/
s/one or more PCE(s)/one or more PCEs/

s/collection of a PCE state metric(s)/collection of PCE state metrics/
===
3.  Path Computation Monitoring messages
s/to enforce a PCE/to require a PCE/
===
3.1.  Path Computation Monitoring Request message (PCMonReq)

You need to mention how this message will be handled by a “legacy” PCE that 
does 
not recognise the message. Also how it would be handled by a PCC.

OLD
   Note that this set of objects
   is by all means not limitative and may be extended in further
   revision of this document.
NEW
   Note that this set of objects
   may be extended in future.

OLD
   Example 1: PCC1 requests to check the path computation chain should a
   path computation be requested for a specific TE LSP named T1.
NEW
   Example 1: PCC1 requests to check the path computation chain that
   would be used should it request a path computation a specific TE 
   LSP named T1.

s/PCRep message also comprises a PROC-TIME/PCRep message also carries a PROC-TIME/

Example 3
s/contains a set of PCE-ID objects/contains a sequence of PCE-ID objects/

===
3.2.  Path Monitoring Reply message (PCMonRep)
   If the Monitoring
   object is missing, the receiving PCE MUST send an error message to
   the requesting PCC.

Which error code?

===
4.  Path Computation Monitoring Objects

s/computation request./computation requests./

===
4.1.  MONITORING Object

   The MONITORING object MUST be present within PCMonReq and PCMonRep
   messages ("out of band" monitoring requests) and MAY be carried
   within PCERep and PCReq messages ("in band" monitoring requests).
   There MUST be exactly once instance of the MONITORING object

There is a slight contradiction between MAY and MUST.

   if more
   than one instance of the MONITORING object is present, the recipient
   MUST only process the first instance and ignore other instances.

But now you have a contradiction with the MUST as well.

Perhaps this should read...

   The MONITORING object MUST be present within PCMonReq and PCMonRep
   messages ("out of band" monitoring requests) and MAY be carried
   within PCERep and PCReq messages ("in band" monitoring requests).
   There SHOULD NOT be more than one instance of the MONITORING object:
   if more than one instance of the MONITORING object is present, the 
   recipient MUST process the first instance and MUST ignore other
   instances.
Please indent the bit numbering correctly

   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |       Reserved    |              Flags              |I|C|P|G|L|
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
Very unusual split. 10 bits reserved and 22 bits of flags.

OLD
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                     monitoring-id-number                      |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
NEW
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                     Monitoring-id-number                      |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

s/Flags: 18 bits/Flags: 22 bits/

   The P bit MUST always be set in a PCMonRep message if also
   set in the corresponding PCMonReq message.
No objection, but why? Doesn’t the PROC-TIME object convey the same information?
I note that the L and G bits are not defined in the PCMonRep.
Anyway, you need to talk not just about PCMonReq/Rep, but the use in PCReq/Rep (cf 
C bit).

OLD
   I (Incomplete) - 1 bit: the I bit MUST be set by a PCE that supports
   the PCMonReq message, which does not trigger any policy violation but
   that cannot provide the set of requested performance metrics for
   unspecified reasons.
NEW
   I (Incomplete) - 1 bit: If a PCE supports a received PCMonReq 
   message and that message does not trigger any policy violation, but
   the PCE cannot provide any of the set of requested performance 
   metrics for unspecified reasons, the PCE MUST set the I bit. The I
   bit has no meaning in a request and SHOULD be ignored on receipt.

Question...
   Monitoring-id-number (32 bits).  The monitoring-id-number value
   combined with the source IP address of the PCC and the PCE address
   uniquely identify the monitoring request context.
“the PCE address”?
The request is not specifically targeted at a PCE, or may include a list of PCEs. 
How does this work? Isn’t the PCC ID and the monitoring-id-number sufficient? 
Later 
you say that the same monitoring-id-number can be used for requests sent to 
different PCEs, but this seems to be ambiguous for chains of PCEs. And you say 
“The 
path computation monitoring reply is unambiguously identified by the IP source 
address of the replying PCE” but the replying PCE might not be known by the 
source 
PCC.

Question
   The monitoring-id-
   number MUST be incremented each time a new monitoring is sent to a
   PCE.
Same problem as was raised by the IESG for PCEP. The requirement is just that this 
number allows disambiguation of active requests/responses. Why do you 
require “increment” and by what value should the increment be made?

s/Conversely, different/Conversely, a different/

   No optional TLVs are currently defined.
So, why do we have TLVs? Would it be better to put some of the response objects 
into this object as TLVs? E.g., PROC-TIME, CONGESTION, etc.

===
4.2.  PCE-ID Object
   A set of PCE-ID objects may be inserted within a PCReq or a PCMonReq
   message to specify the PCE for which PCE state metrics are requested
   and in a PCMonRep or a PCRep message to record the IP address of the
   PCE reporting PCE state metrics or that was involved in the path
   computation chain.
So...
If I send a PCReq with a list of PCE-IDs because these are the PCEs I want to 
process the request, can I control the list of PCEs that supply the monitoring 
information? For example, I way want the processing time form just the last PCE of 
a chain of PCEs.

Please align the bit numbers on the figures correctly.

   A PCE MUST use the same IP address as the address used in the PCE-
   ADDRESS sub-TLV defined in [RFC5088] and [RFC5089] should a dynamic
   discovery mechanism be used for PCE discovery.
Should that read “A PCC MUST ...” ? After all, the PCE has no control over the 
PCE-
ID supplied in a request.

===
4.3.  PROC-TIME Object

   The PROC-TIME object MUST be present within a PCMonRep or a PCRep
   message if the P bit of the MONITORING object carried within the
   corresponding PCMonReq or PCReq message is set.
“MUST”? 
Compare with the I bit.
Consider local policy that says “I am not going to answer that”

s/the PROC-TIME object always report/the PROC-TIME object always reports/
s/thus the E bits/thus the E bit/

Please align the bit numbers correctly.

   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |       Reserved                |           Flags               |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                           Processing-time                   |E|
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
Wouldn’t it be more sensible to move the E bit into the Flags field so that all 
time fields are the same format?

   Current-processing-time: This field indicates in milliseconds the
The field is called “Processing-time” in the figure.

===
4.4.  CONGESTION Object
   C (Congestion) - 1 bit: when set, this indicates that PCE is
   congested, in which case the congestion duration may be non nul.
   When cleared this indicates that the PCE is not congested.

   Congestion duration - 16 bits: This field indicates in seconds the
   estimated congestion duration.

Confused! What is the meaning of C-bit set, Congestion Duration = 0?

===
4.5.  TIMESTAMP Object

   A TIMESTAMP object will be specified in a further revision of this
   document

Will you do this before WG last call, or will you delete this section?

===
5.  Multi-destination monitoring

Capitilise the seciton heading, please.

   In a further revision of this document, a new object will be
   specified allowing a PCC or a user to gather PCE state metrics for a
   set of destinations using a single PCMonReq message.

Need to resolve this TBD.

===
7.  Elements of procedure

   I bit processing: as indicated in section Section 4.1, the I bit MUST
   be set by a PCE that supports the PCMonReq message, which does not
   trigger any policy violation but that cannot provide the set of
   required performance metrics for unspecified reasons.  Once set, the
   I bit MUST NOT be changed by a receiving PCE.
Revise for clarity as proposed for section 4.1

   Reception of a PCMonReq message: upon receiving a PCMonReq message:

   1) If the PCE does not support the PCMonReq message, the PCE MUST
   send a PCErr message with Error-type=14 and Error-value=1.

Wait a moment. You are defining a new error code to handle non-support of a new 
feature. Surely you should use an existing error code as see in the PCEP spec. For 
example, error-type=2

===
8.  Manageability Considerations

   To be addressed in a further revision of this document.

Please resolve this TBD

===
9.  To be considered in a further revision of this document

   It might be desirable to modify the format of the PCMonReq and
   PCMonRep messages to support the bundling of multiple performance
   metrics collection for a set of TE LSPs.

Please resolve this TBD

===
10.  IANA Considerations

Need to give better references to the registries defined in the PCEP I-D.

===
11.  Security Considerations

   To be addressed in a further revision of this document.

Hmmm?

_______________________________________________
Pce mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/pce