Re: [CCAMP] LSP attribute in ERO Update

["Zhangxian (Xian)" <[e-mail]>]

Hi, Cyril,

   I have reviewed this draft and comments as detailed below.

From the editor’s note embedded in the draft, I reckon some of the issues might have already been discussed or at least thought of before by the authors or within WG, which I may not be aware of the details. If so, it would be good to recap what is the outcome of the relevant discussion(s).

Cheers,
Xian

-----------------review comments starts from here-----------------
Title:
Current one (LSP Attribute in ERO) does not capture the main content of this draft. I suggest two to start with, you may use one of them, write one yourself.
1: RSVP ERO/RRO extensions to support specifying attribute(s) specific to a single hop
2: RSVP extensions to support per-hop attribute specifications in ERO/RRO

Abstract:
It implies RFC5420 does not support report per-hop attributes, but I find the following sentence and section in this RFC:
“[in Abstract]
Additionally, this document defines a way to record the attributes applied to the LSP on a hop-by-hop basis.
”
AND
Section 7 (with RRO attribute subjects defined), especially the following:
“7.3.3.  Reporting Per-Hop Attributes

   To report a per-hop attribute, an LSR sets the appropriate bit in the
   Attributes subobject.

   The requirement to report a per-hop attribute MUST be specified in
   the document that defines the usage of the bit.”

Am I missing anything?

Introduction:
1：
Old：
Several documents have identified the need for attributes that can be
   targeted at specific hops in the path of an LSP, including [RFC6163],
   [I-D.ietf-ccamp-wson-signaling],
   [I-D.dong-ccamp-rsvp-te-mpls-tp-li-lb] or
   [I-D.ali-ccamp-rc-objective-function-metric-bound],.  This document
   provides a generic mechanism for use by these other documents.
New:
The need to specify per-hop attribute(s) in the path of a LSP have been identified in several existing documents, including [RFC6163], [I-D.ietf-ccamp-wson-signaling],    [I-D.dong-ccamp-rsvp-te-mpls-tp-li-lb] and [I-D.ali-ccamp-rc-objective-function-metric-bound]. This document provides a generic mechanism to meet this requirement.

Comment:
1.1: is the first and second reference providing any difference? If not, I would suggest to remove RFC6163 since WSON-signaling cites this draft already.
1.2: w.r.t. the last reference, could you please explain why is it included since I do not see why OF and Metric bounds are tied with one hop, at least not mentioned by the referred document?

2：
Removing Section 1.1 (no content)?

Section 2: Requirement:
1：
“A mechanism similar to LSP attribute defined [RFC5420] should be expressed in ERO and SERO objects.  A new ERO sub-object is defined, containing a list of generic per-Hop attributes.”

This two seems to contradict with each other. The first sentence says it should be used in both ERO and SERO objects. The 2nd says an ERO sub-object. Should the 2nd sentence be replaced with the following?
A new sub-object is defined, containing a list of generic per-hop attribute(s), which can be carried in ERO or SERO objects.

2：
“The mechanism defined in this document limits itself to single HOP attributes, and does not address attributes valid for a LSP section [[CREF1: This can be revised based on feedback --Ed.]]”

Comment: why it should be confined to particular hop? Here, hop means the link or the node? I would prefer a LSP segment to a LSP section.

3:
Comment: This section does not match with what extended in RSVP later in this draft. To be more specific, this section only mentions about the need to specify per-hop attribute, but later in this draft, another RRO subobject is also defined. The update of this section, I reckon, would subject to your answer to my question regarding the relationship of what is defined in this draft and in RFC5420 (RRO_HOP_ATTRIBUTE subobject vs. RRO Attributes subobject), raised up earlier.

Section 3: Specifying Hop Attributes
1:
Old：
The ERO Attributes subobject may be carried in the ERO or SERO object if they are present.
NEW:
The ERO_HOP_ATTRIBUTE subobject may be carried in the ERO or SERO object.
Comment：
1.1: DO not get what “if they are present” intends to say.
2.2: Why subobject title has to have ERO in it?

2:
OLD:
The attributes TLV are encoded as defined in section Section 3.2.
New:
The HOP attributes TLV is encoded as defined in Section 3.2.
Comment:
2.1: The text and the encoding is not aligned, please update either of them.
2.2: After reading the whole draft, it seems to me, this draft does not define a new TLV called “HOP Attibutes TLV”, but rather using what has already been defined by RFC5420? If that is the case, why not cite the RFC directly, but redefine with a different name?

3:
“   Reserved  Reserved, must be set to 0 when the subobject is inserted in the ERO, MUST NOT be changed when a node process the ERO and must be ignored on the node addressed by the preceding ERO subobjects.”
Comment:
Why MUST NOT is capitalized but two “must” are not? Also, not sure what does “must be ignored on the node addressed by the preceding ERO subobjects.” mean, please clarify?

4:
Remove “Attributes TLVs as defined in Section 3.2 .” since it is redundant.

5:
Section 3.2 title does not need to use plural form since it is a definition a the Hop Attributes TLV?

6:
“ERO Attributes carried by the new objects defined in this document are encoded within TLVs.  One or more TLVs may be present in each object. “
Should it be stated as following?
“Hop attributes carried by the new sub-object defined in this document are encoded within TLVs. One or more TLVs may be present in each Hop Attributes sub-object.”

7:
In Section 3.3, sometimes RFC2219 language not used while in other places it is, reason?

8:
The TLV name used in Section 3.3 is not aligned with Section 3.2 (multiple places).

Section 4:
1:
This section should explain what is the relationship of the other subobject defined by RFC5420. If not here, at least somewhere in the draft.

2:
3rd comment for Section 3 also applies in this section.

3:
Why the format is using an ERO subobject format, not a general RRO subobject? What is the use of L bit here in RRO? Please look at RRO Attributes Subobject defined by RFC5420.

4:
[[CREF2:  This is not the most efficient encoding, a more efficient encoding would use a bit field ala RFC5420 --Ed.]]
Comment: This is the option I prefer. Note:
4.1: no new RRO subobject is needed anymore.
4.2: any defined HOP Attributes TLV should also be allocated a corresponding attribute flag in the Attribute Flag TLV.

But I am also thinking about if there is any issue with this alternative. The only issue I can think of is the details (value field) of the Hop Attributes are hiding. If it is additional, the egress/ingress node may not be aware of the details. Thoughts?


Section 5:
1:
By saying “ERO LSP Attribute Subobject”, “RRO LSP Attribute Subobject”, you meant “ERO HOP Attribute Subobject”, “RRO HOP Attribute Subobject”?

2:
By saing “Whether allowed on LSP attribute ERO subobject”, you meant “Whether allowed on ERO HOP attribute subobject”?

3:
I understand the intention is to add a new column to the Attributes TLV Space in http://www.iana.org/assignments/rsvp-te-parameters/rsvp-te-parameters.xhtml, but current text is confusing, would it be possible to write thing like the following, without repeating what is there already?
  Name         Allowed on ERO HOP attribute subobject? (new column)
Attribute Flags          Yes
Service ID               ?
OAM Configuration TLV   ?


Minor：
1：
Old：
A mechanism similar to LSP attribute defined [RFC5420] should be expressed in ERO and SERO objects.
New：
A mechanism similar to specifying LSP attribute as defined in [RFC5420] should be expressed in ERO and SERO objects.

2:
s/Specifying Hop Attribute/Specifying Hop Attributes

3:
Old:
Type  The identifier of the TLV..
New:
Type  The identifier of the TLV. In this document, no TLV is defined.

4:
Old:
…The processed or addition HOP attributes…
New:
…The processed or additional HOP attributes…

5:
Old:
…after the RRO attribute subobject (if present) defined in in [RFC5420].
New:
after the RRO attributes subobject (if present) defined in [RFC5420].

6:
s/ Several ERO_HOP_ATTRIBUTE subobject MAY be present, for each hop./ Several ERO_HOP_ATTRIBUTE subobjects MAY be present, for each hop.
Comment: but they should be of different types??



From: CCAMP [mailto:[e-mail]] On Behalf Of Cyril Margaria
Sent: 2014年7月21日 23:36
To: CCAMP
Subject: [CCAMP] LSP attribute in ERO Update

Dear CCAMPers,
We have updated the LSP attribute in ERO document (http://tools.ietf.org/html/draft-ietf-ccamp-lsp-attribute-ro).
The IANA section has been updated, we have addressed all the comments for this document.
A review from the WG is appreciated, the solution being stable..
Best Regards.
Cyril