Re: [ippm] Last Call: <draft-ietf-ippm-twamp-yang-07.txt> (Two-Way Active Measurement Protocol (TWAMP) Data Model) to Proposed Standard

["tom p." <[e-mail]>]

Mahesh

Yes, your responses look good.

Tom Petch


----- Original Message -----
From: "Mahesh Jethanandani" <[e-mail]>
Sent: Friday, April 13, 2018 1:13 AM


Tom,

First of all, thanks for reviewing the document. See my comments inline
with [mj].


> On Apr 11, 2018, at 8:35 AM, tom p. <[e-mail]> wrote:
>
> Some further thoughts on this I-D; I have included the earlier ones
(as
> yet unanswered) below.
>
> typedef test-session-state {
>     .....
>             "Indicates that accepted TWAMP-Test session request.";
>
> something seems to have been omitted.

[mj] Yes. Should be “Indicates an accepted TWAMP-Test session request.”

>
>
> leaf reflector-udp-port {
>               type uint32 {
>                 range "862 | 49152..65535";
> I am puzzled as to why uint32 is used and inet:port-number is not used
> here (happens in two places)

[mj] Ok. Will make the change.

>
>
> "Reusable data structure for count which is used both in the Server
> container.";
>
> 'both' suggests to me that there should be a second clause.

[mj] Yes, made it "Reusable data structure for count, which is used both
in the Server and the Control-Client.”

>
>
> "grouping max-count {
>       leaf max-count {
>         type uint8 {
>           range 10..31;
>         }
>         default 15;"
> contradicts
> " ...The default max-count value SHOULD be 32768.'
> 32768 is outside the range!

>
> I think that your problem here is that other TWAMP documentation, such
> as the RFC that you quote and reference uses 'max-count' to mean a 32
> bit value but here you are reusing the term with a different semantic
> and using it to mean an exponent; rename the object to make it clear
> that it is an exponent and not a count e.g. max-count-exp. This occurs
> in several places.

[mj] Ok. How about we call it 'max-count-exponent’, just to be explicit?

>
> "container session-sender {
>         if-feature session-sender;
>         presence  "Enables TWAMP Session-Sender functionality.";
>         description
>           "Configuration of the TWAMP Session-Sender logical entity";
>         leaf admin-state {
>           type boolean;
>           mandatory true;
>           description
>             "Indicates whether the device is allowed to operate
>              as a TWAMP Session-Sender.";
> "
>
> A presence container is a boolean so I am unclear what it is that the
> admin-state boolean adds here since the presence container "Enables
> TWAMP Session-Sender functionality.”;

[mj] Good catch. Will remove the presence statement.

>
> "     container session-reflector {
> "
> same story here

[mj] Ditto.

>
> Tom Petch
>
> ----- Original Message -----
> From: "tom p." <[e-mail]>
> To: <[e-mail]>
> Cc: <[e-mail]>; <[e-mail]>;
> <[e-mail]>
> Sent: Wednesday, April 11, 2018 12:40 PM
>
>
>> Some mostly administrative points on this I-D
>>
>> [I-D.ietf-ippm-metric-registry] is an Informative Reference so would
> not
>> hold up the production of this as an RFC yet you ask that the text be
>> replaced by the RFC number which implies that you do want it held up.
> I
>> did ask the RFC Editor about this and yes, they would expect to catch
> it
>> but it would seem simpler if this could be a Normative Reference.
You
>> do have the statement up front about replacing the text with the RFC
>> number and yes, the RFC Editor like that.

[mj] I will drop the editor’s note to replace the RFC number for
I-D.ietf-ippm-metric-registry.

>>
>> There is no reference, no legend,  for Tree Diagrams in 5.1 .  There
> is
>> now an RFC on this, RFC 8340 while RFC8343 s.1.3 is an example of how
> to
>> reference this RFC.

[mj] Will add reference to RFC8340.

>>
>> There is no copyright statement in the YANG module as is required by
>> 6087bis (please, IESG, make this an RFC soon:-)

[mj] Ok. Will add it.

>>
>> leaf server-start-time {
>> includes
>>                The timestamp format follows RFC 1305
>> but I see no RFC 1305 in the references of the I-D

[mj] Ok. Will add it in the references, and also in the beginning of
section 5.2. Will do the same for all the other references in the YANG
module.

>>
>> leaf reflector-udp-port
>> " The default number is
>>                  within to the dynamic port range and  .. "
>> which is not quite English.

[mj] Yes. How about “The default number is within the dynamic port range
and …”?

>>
>> "The new well-known port (862) MAY be used.";
>> This was allocated in 2008 which seems to stretch the meaning of
‘new'

[mj] Al, do you want to comment on this?

>>
>> leaf secret-key {
>>             type binary;
>> I wonder about the choice of binary; elsewhere, e.g. RFC8177,
>> hexadecimal is used.
>> Are there, should there be, any length constraints on this key?

[mj] RFC8177 says that not all vendors support the definition of
key-string as a hexadecimal. Just to be safe, I will make it string.

>>
>> case poisson {
>> ...
>>   reference
>>                  "RFC 2330: Framework for IP Performance Metrics";
>> RFC2330 I cannot see in the references for the I-D

[mj] Will add.

>>
>> The Security Considerations are not as per the current template e.g.
> no
>> mention of RESTCONF

[mj] Agree. Will update.

Cheers.

>>
>> Tom Petch
>>
>> ----- Original Message -----
>> From: "The IESG" <[e-mail]>
>> To: "IETF-Announce" <[e-mail]>
>> Cc: <[e-mail]>; <[e-mail]>;
>> <[e-mail]>
>> Sent: Monday, April 09, 2018 3:57 PM