[CCAMP] AD review draft-ietf-ccamp-rsvp-te-mpls-tp-oam-ext

["Adrian Farrel" <[e-mail]>]

Hi,

Thanks to the authors for being patient with this draft. The CCAMP OAM
configuration framework draft and the Ethernet counterpart to this work
have now progressed far enough through the system that it is appropriate
to move this one forward, so I have done my usual AD review. The purpose
of the review is to catch some of the larger issues that might come up
in IETF last call and IESG evaluation. Catching them early helps save
everyone time, and improves the quality of subsequent reviews.

I have a few small editorial issues listed below and a request for
some careful reworking of the IANA section. This is open for
discussion if you have any concerns, but until then I have placed
the document in "Revised I-D Needed" state. When I see a new
revision I will advance the document to IETF last call.

Thanks for the work.

Adrian

====

Loa may want to change his affiliation.

---

Could you please remove the citation from the Abstract. Probably replace
it with "...the OAM Configuration Framework for GMPLS RSVP-TE."

---                                                 

I think you can/should remove the ITU-T cooperation paragraph from the
Abstract and Introduction unless this work has been explicitly worked on
in a shared way by the two bodies.

---

You need to look through the document for unexpanded acronyms. I see
LER
LSR
NMS
LSP
MEP
FMS
G-ACh
PM

---

Section 1 has...

   Pro-active MPLS-TP OAM is performed by four different protocols

but I think you only list 3 (BFD, DM, and LM)

---

The penultimate paragraph of Section 1 comes along a little late, IMHO.

   MPLS-TP describes a profile of MPLS that enables ...

I suggest moving this to be the second paragraph.

---

I suggest moving Section 1.1 to be placed between sections 5 and 6 to
be more consistent with the RFC editor guidelines.

---

3.1.3

   When configuring Fault Management
   Signals, two options are possible, default configuration is enabled
   by setting the respective flags in the "OAM Function Flags sub-TLV",
   the default settings MAY be customized by including the "MPLS OAM FMS
   sub-TLV".

It took me a long time to parse the two options in this paragraph: once
I saw it, it was obvious.  I also think "the respective flags" is not
enough information. I suggest something like...

   When configuring Fault Management
   Signals, an implementation can enable the default configuration by
   setting the FMS flag in the "OAM Function Flags sub-TLV".  If an
   implementation wishes to modify the default configuration it includes
   a "MPLS OAM FMS sub-TLV".

---

You can safely remove some of the IANA allocation discussion from 
Section 3.2 and leave it to the Section 5 where it is also stated.

---

Section 3.2

   Length: indicates the total length including sub-TLVs.

You need to be clear whether "total length" includes the T and L.
This applies to the definitions of the sub-TLVs as well.

---

Section 3.2

   The following MPLS OAM specific sub-TLVs MAY be included in the the
   "MPLS OAM Configuration sub-TLV":

You omit to say (but you imply) that it is legal to have zero sub-TLVs
present. Please clarify.

Also s/the the/the/

---

Section 3.2

What happens if a sub-TLV is included and the appropriate lag was not 
set?

What happens if multiple copies of a sub-TLV are present?

What ordering of sub-TLVs is required?

What happens if a mandatory sub-TLV (according to flag settings etc.) is
not present?

---

Section 3.2

   Moreover, if the CV Flag is set, the CC flag SHOULD be set as well.

It would make sense for this and the following paragraphs to be in their
own subsection.

Please use the flag names as stated in [OAM-CONF-FWK]

I think that this "SHOULD" applies specifically to MPLS-TP usage. Can 
you make that clear. Can you also explain in the text why this is not a
"MUST".

---

There is an alignment error in the figure in 3.3.1

---

Section 3.3.1
Shouldn't there be a reference to a BFD spec for Local Discriminator?

---

Section 3.3.2

   Acceptable Min. Asynchronous RX interval: If the S (symmetric) flag
   is set in the "BFD Configuration sub-TLV", this field MUST be equal
   to "Acceptable Min. Asynchronous TX interval" and has no additional
   meaning with respect to the one described for "Acceptable Min.
   Asynchronous TX interval".

If it MUST be equal, you have to handle it being different with an 
error case. But since the field has no meaning in this case, why not
say it MUST be set equal and MUST be ignored on receipt?

---

Section 3.4

   In case the values need to be different than the default ones the
   "Performance Monitoring sub-TLV" MAY include the following sub-TLVs:

s/MAY include/includes/

---

Section 3.4.2

   Type: 2,"MPLS OAM PM Loss sub-TLV".

A victim of block copying, I think.

---

The IANA considerations and use of codepoints in the document need some
work.

Section 5 is reasonably clear, but it would help if you could break it 
into subsections, one for each action.
- MPLS OAM Configuration Sub-TLV
- MPLS OAM Type
- New RSVP-TE OAM Configuration registry
- New BFD Configuration Type registry
- New Performance Monitoring Type registry
- New RSVP-TE error codes

It is really important to name the registries you are asking IANA to
allocate from. At least one of the registries does not exist yet, so
you need to state "[OAM-CONF-FWK] requests the creation of..."

For allocations from existing registries, do you *really* need the 
values you have specified, or were you trying to be helpful?  Please 
replace in this section and in the text...
- MPLS OAM  TBA1  (was 2)
- MPLS OAM Configuration Sub-TLV  TBA2  (was 33)
   - You should request allocation from the Technology-specific range

For existing registries, please build the table fragment here to 
include all of the table columns defined in the registry.  There will
be at least a "Reference" column.

For each of the new sub-registries, please:
- note that they are sub-registries
- note that the parent registry is created by [OAM-CONF-FWK]
- add a column for "Reference" and populate it with [This.I-D]
- replace "Reserved" with "Unassigned" for the higher values
- state explicitly what the top of the range is
- state what the allocation policy is for new values with a reference
  to RFC 5226

For the error codes and error value sub-codes, it would be more helpful
if you could:
- indicate that the "values" to be assigned are "error value sub-codes"
- list the new sub-codes one per line
- leave out "OAM Problem/" from each one

---

The error value sub-code "Mismatch of BFD Authentication Key ID" in the
IANA section is referred to as "Mismatch of BFD Authentication ID" in 
Section 3.3.3 and Section 4.

---

It would be nice to add some material discussing the management impact
of this work. Obviously it is all about OAM, but there are a number of
things that need to be configurable, and a few more things that need to
be visible to a management station.