Subject: [ippm] Review of draft-brockners-inband-oam-data-05

From: Joe Clarke <[email-masked]>
Date: Wed, 31 May 2017 17:43:07 -0400

Hello, IPPM WG.  I'm not an active IPPM WG participant, so while Brian
recently called for adoption of this document, I will defer a comment there.

I did, however, read the document, and I have some comments to the WG
and authors for consideration.

One overall comment is that while "IOAM" is defined in the terminology
section, it is not often used.  Instead, the full "in-situ OAM" spelling
is preferred.  To me, this gets a bit tiresome to read.  You have
defined the abbreviation, and you should use it.

===

Section 4:

I think it should be noted that nodes can be both in-situ encap/decap
and transit nodes.  That is, a node that adds the container could also
write to it.

===

Section 4.1:

In Section 4 you state that every node need not be an IOAM transit node.
 But here in 4.1 you state that for the tracing option all nodes should
be transit/encap/decap nodes.  While it doesn't say all nodes MUST be
IOAM-aware, I think it would be beneficial if you stated (at least for
operation purposes) what one would expect from a domain that contains
non-IOAM-aware nodes.

===

Since an IOAM deployment MAY choose to support either the software or
hardware optimized tracing options, how would one know what is supported
across all nodes?  It seems like one would need to encapsulate both if
there is no a priori way to learn about this.

===

Your text discussing IOAM tracing options states:

o  Identification of the interface that a packet was received on.

o  Identification of the interface that a packet was sent out on.

Later on you use the terms "ingress" and "egress".  I think it would be
good to use them here as well.

===

Section 4.1.1

You refer to Section 4.1.4 as describing the format of the IOAM data
types, but this is really in 4.1.3, with 4.1.4 being examples of the
data types (same in 4.1.2).

===

You only define two bits for an 8-bit Flags field.  What is expected of
the other six bits?

===

Section 4.1.2

Seems like a missed copy-paste from Section 4.1.1.  You describe Bit 1 as:

Bit 1    When set indicates presence of ingress_if_id and
               egress_if_id in the node data.

And Bit 9 as:

Bit 9    When set indicates presence of ingress_if_id and
               egress_if_id wide in the node data.

I think you should keep what you have in Section 4.1.1:

Bit 1    When set indicates presence of ingress_if_id and
               egress_if_id (short format) in the node data.

Bit 9    When set indicates presence of ingress_if_id and
               egress_if_id in wide format in the node data.

===

With respect to Maximum Length, you say a simple comparison between "Opt
Data Len" and "Max Length" should be enough to decide if data can be
added.  I'm not sure.  In your description in Section 4.1 you state that
the option header will have fields for "number of fields recorded" and
"maximum number".  I don't see how that jibes with Section 4.1.2.

===

Section 4.1.3

You don't have a block figure for "app_data".  Seems odd considering you
do for all other options.  I'm also not clear on what this might be.
You don't really describe it (same thing for wide app data).

===

Your wide ingress and egress interfaces use an overall 8-byte field, but
your "empty" indicator is still 0xFFFFFFFF.  I think this should either
be 0xFFFFFFFFFFFFFFFF or you need to state that EACH 4-byte field needs
to be all-Fs.

===

Section 4.2

You use POT, but you haven't added this to your terminology section
above.  It's clear to me what it means, but I think it's worth
standardizing all terminology.

===

Section 4.3

Same as above for "E2E."

===

Section 5

Above you stated that an IOAM transit node _could_ process IOAM data.  I
think this is useful.  I think it's worth stating in Section 5 that each
IOAM-aware node could choose to export IOAM data.  It need not be at the
edge.

I think that's it for now.

Joe