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

From: "Frank Brockners (fbrockne)" <[email-masked]>
Date: Fri, 2 Jun 2017 17:11:50 +0000

Hi Joe,

thanks a lot for the detailed review. We'll address your comments in the next revision. Please find a couple of notes and responses inline...

-----Original Message-----
From: ippm [mailto:[email-masked]] On Behalf Of Joe Clarke (jclarke)
Sent: Mittwoch, 31. Mai 2017 23:43
To: [email-masked]
Subject: [ippm] Review of draft-brockners-inband-oam-data-05

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.

...FB: Good point. We'll add this.

===

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.

...FB: Again, a good point which will address in the next revision.

===

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.

...FB: With IOAM being a domain specific feature, the operator will know which trace option would be used, or whether both would be used in parallel. If both would be used in parallel, because a mix of HW and SW forwarders is used, the encap node would put both in. 

===

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.

...FB: Agreed.

===

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).

...FB: Agreed.

===

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

...FB: Those are reserved for future use. We can define them as MSB for now.

===

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.

...FB: Good catch. Thanks!

===

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.

...FB: Again a good catch. What we say in 4.1 ("The number of node data fields recorded and maximum number of node data that can be recorded are written into the option header.") indeed needs to be adjusted, given that we only carry the maximum length (per 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).

...FB: "app_data" is a free form bit-field, which can be interpreted in a domain-specific way. We'll add some verbiage along with an example in the next rev.

===

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.

...FB: Good catch... there were quite a few of those in the -04 version, and it seems that I did not catch all of those in the -05 version.

===

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.

...FB: We'll add POT to the list of abbreviations and provide for a short explanation. 

===

Section 4.3

Same as above for "E2E."

...FB: We'll add E2E to the list of abbreviation.

===

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.

...FB: Agreed that the option to export IOAM data at any node - not just at the edges of a domain, could be useful. We'll note that this is an option in the next rev.

I think that's it for now.

Thanks again for the detailed review! Frank

Joe

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

---

From: Joe Clarke <[email-masked]>
Date: Fri, 2 Jun 2017 14:14:58 -0400

> thanks a lot for the detailed review. We'll address your comments in the next revision. Please find a couple of notes and responses inline...

No problem.  I definitely support the work.

> ...FB: With IOAM being a domain specific feature, the operator will know which trace option would be used, or whether both would be used in parallel. If both would be used in parallel, because a mi…

Okay.  Perhaps some text can be added to make this more explicit?

> ...FB: Those are reserved for future use. We can define them as MSB for now.

What I've learned from reading other reviews is that making it explicit
that these bits are reserved and must be ignored is highly recommended
in I-Ds.  That's why I raised the point.

> Thanks again for the detailed review! Frank

Again, my pleasure.

Joe