Re: [ippm] draft-ippm-ioam-data-09 WGLC comments

[Tommy Pauly <[e-mail]>]

> On May 28, 2020, at 6:06 AM, Frank Brockners (fbrockne) <[e-mail]> wrote:
> 
> Hi Martin,
>  
> Thanks a lot for the detailed review. Please see inline (“…FB”).
>  
> From: Martin Duke <[e-mail] <mailto:[e-mail]>> 
> Sent: Mittwoch, 27. Mai 2020 01:24
> To: [e-mail] <mailto:[e-mail]>; [e-mail] <mailto:[e-mail]>
> Subject: draft-ippm-ioam-data-09 WGLC comments
>  
> This document is almost ready. I have a few review comments and a trail of nits.
>  
> Comments:
> 1. It's RFC Ed policy to have no more than 5 authors, and you have 12. Please follow these guidelines <https://www.rfc-editor.org/policy.html#policy.authlist> to trim the list down.
> …FB: The topic was brought up and discussed several times, though there is no easy solution, given that IOAM had indeed many cooks and at least I am not able to find appropriate reasons to prune the list of authors to 5. I’m hoping for some guidance from the WG chairs on how to handle the issue.

We would like to see this changed to an editor model, in which one or two authors are listed as editors, and there is an Authors section in the text below to correctly attribute the contributors. No need to prune anyone out or choose people to fit in the list of 5, but there’s not a need to have the list at the top versus elsewhere in the text.

>  
> 2. Throughout the document, there are many lower-case 'must', 'should', or 'may.' These should be reviewed, and most of them changed to upper-case. Non-upper-case should generally use synonyms like 'might' or 'could'.
> …FB: We’ll review all the lower-case must/should/may statements once more and see whether there is a need to change to uppercase. My current understanding (please correct me if I’m wrong) was that per RFC 8174, the use of lower case must/may/should would be ok – we can of course see whether we can improve the wording to make sure things are not misread. 

It’s also my understanding that a lowercase must/should/etc is technically fine, and not to be interpreted as MUST/SHOULD/etc. However, it is a best practice to use other verbiage to avoid possible confusion. Different reviewers have different opinions on this one =)

Tommy (as co-chair)

> 
> 3. Section 4.2 says that transit nodes "will update at most one of these Option Types". I think you mean "at least one of each of these option types", at least if I'm reading 4.4 correctly.
> …FB: It is indeed “will update at most one of these Option Types” - because for tracing it is either prealloc-update OR incremental-update OR no-operation, i.e. a node will either do nothing or update one of the Option-Types, but never both at the same time. 
> 
> 4. Section 4.4.1 has many normative requirements for the options. What should transit nodes do if these are not followed (e.g. remainingLen is a garbage number)? Perhaps there should be an "error flag" in addition to the Overflow flag?
> …FB: IMHO we can follow typical design principles here, i.e. be loose/tolerant on what you accept, strict on what you send. We can add a sentence that if a node receives a packet with IOAM data fields that it cannot decode/understand, it would simply ignore the IOAM data fields. Having a node figure out whether the IOAM data is really corrupted or not might be hard to achieve (e.g. it could be some future version that the node just does not understand).
>  
> 5. does the pre-allocated option have any sort of initial value the encapsulation node should set all the trace option data to? Would this allow the transit nodes to detect if there was a mistake in RemainingLen? 
> …FB: If we go with the above assumption, that nodes that do not understand the IOAM data fields would simply ignore them and leave the packet untouched, we would not benefit from any pre-set value on allocation. Depending on the implementation, pre-defined values may impose an additional burden on the encap node.
>  
> Nits:
> Sec 3. 
> s/using for example packet/using, for example, packet
> s/but do not have to share/but do not have to, share
> s/data-plane/data planes
>  
> Sec 4.3 s/at average/on average
>  
> Sec 4.4 
> s/hardware and software implementations IOAM/hardware and software implementations, IOAM
> s/data.The/data. The
> s/devices which either/devices with either
>  
> Sec 4.4.1
> suggest the sentence
> 'This bit is set by the network element if there are not enough octets left to
>          record node data, no field is added and the overflow "O-bit"
>          must be set to "1" in the IOAM-Trace-Option header.'
> be rewritten as
> 'If there are not enought octets left to record node data, the network element MUST NOT add a field and MUST set the overflow "O-bit" to "0" in the IOAM-Trace-Option header.'
>  
> s/When set indicates/When set, indicates [multiple times in this section]
>  
> Sec 4.4.2
> This section could use some subheaders for each node data field
>  
> s/hence recommend/and hence RECOMMEND
> s/standard unit/standard units
>  
> Sec 4.5
> Please add a reference for SSSS
>  
> Sec 5.3
> s/run Linux/run the Linux
>  
> Sec 7.8
> RFC 8126 says you are supposed to provide guidelines for the expert reviewer.
>  
> …FB: Thanks for catching all those nits! Greatly appreciated.
>  
> Cheers, Frank
>  
> -- Martin