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

From: "tom p." <[email-masked]>
Date: Wed, 11 Apr 2018 16:35:55 +0100

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.

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)

"Reusable data structure for count which is used both in the Server
container.";

'both' suggests to me that there should be a second clause.

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

"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.";

"     container session-reflector {
"
same story here

Tom Petch

----- Original Message -----
From: "tom p." <[email-masked]>
To: <[email-masked]>
Cc: <[email-masked]>; <[email-masked]>;
<[email-masked]>
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.
>
> 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.
>
> There is no copyright statement in the YANG module as is required by
> 6087bis (please, IESG, make this an RFC soon:-)
>
> leaf server-start-time {
> includes
>                 The timestamp format follows RFC 1305
> but I see no RFC 1305 in the references of the I-D
>
> leaf reflector-udp-port
> " The default number is
>                   within to the dynamic port range and  .. "
> which is not quite English.
>
> "The new well-known port (862) MAY be used.";
> This was allocated in 2008 which seems to stretch the meaning of 'new'
>
>  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?
>
>  case poisson {
> ...
>    reference
>                   "RFC 2330: Framework for IP Performance Metrics";
> RFC2330 I cannot see in the references for the I-D
>
> The Security Considerations are not as per the current template e.g.
no
> mention of RESTCONF
>
> Tom Petch
>
> ----- Original Message -----
> From: "The IESG" <[email-masked]>
> To: "IETF-Announce" <[email-masked]>
> Cc: <[email-masked]>; <[email-masked]>;
> <[email-masked]>
> Sent: Monday, April 09, 2018 3:57 PM
>
> > The IESG has received a request from the IP Performance Measurement
WG
> (ippm)
> > to consider the following document: - 'Two-Way Active Measurement
> Protocol
> > (TWAMP) Data Model'
> >   <draft-ietf-ippm-twamp-yang-07.txt> as Proposed Standard
> >
> > The IESG plans to make a decision in the next few weeks, and
solicits
> final
> > comments on this action. Please send substantive comments to the
> > [email-masked] mailing lists by 2018-04-27. Exceptionally, comments
may
> be
> > sent to [email-masked] instead. In either case, please retain the
> beginning of
> > the Subject line to allow automated sorting.
> >
> > Abstract
> >
> >
> >    This document specifies a data model for client and server
> >    implementations of the Two-Way Active Measurement Protocol
(TWAMP).
> >    We define the TWAMP data model through Unified Modeling Language
> >    (UML) class diagrams and formally specify it using YANG.
> >
> >
> >
> >
> > The file can be obtained via
> > https://datatracker.ietf.org/doc/draft-ietf-ippm-twamp-yang/
> >
> > IESG discussion can be tracked via
> > https://datatracker.ietf.org/doc/draft-ietf-ippm-twamp-yang/ballot/
> >
> >
> > No IPR declarations have been submitted directly on this I-D.
> >
> >
> >
> >
>

---

From: Mahesh Jethanandani <[email-masked]>
Date: Thu, 12 Apr 2018 17:13:20 -0700

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. <[email-masked]> wrote:
> 

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

> here (happens in two places)

[mj] Ok. Will make the change.

> '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.”

> that it is an exponent and not a count e.g. max-count-exp. This occurs

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

> TWAMP Session-Sender functionality.”;

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

> "     container session-reflector {

[mj] Ditto.

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

>> now an RFC on this, RFC 8340 while RFC8343 s.1.3 is an example of how

[mj] Will add reference to RFC8340.

>> 6087bis (please, IESG, make this an RFC soon:-)

[mj] Ok. Will add it.

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

>> which is not quite English.

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

>> This was allocated in 2008 which seems to stretch the meaning of ‘new'

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

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

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

[mj] Agree. Will update.

Cheers.

>>> No IPR declarations have been submitted directly on this I-D.

Mahesh Jethanandani
[email-masked]

---

From: "MORTON, ALFRED C (AL)" <[email-masked]>
Date: Fri, 13 Apr 2018 00:19:43 +0000

...
> [mj] Al, do you want to comment on this?
[acm] 
Yes, I'll comment.

The UDP well-known port will have a new allocation,
changing from TWAMP-CONTROL to TWAMP-TEST.

Incidentally, the progress of that draft is also a dependency,
but it is ready to ship (a small typo was identified in London,
and the new draft has been available since).

Al

---

From: "tom p." <[email-masked]>
Date: Fri, 13 Apr 2018 12:46:57 +0100

----- Original Message -----
From: "MORTON, ALFRED C (AL)" <[email-masked]>
Sent: Friday, April 13, 2018 1:19 AM

Al

In which case, I think that that draft

 draft-ietf-ippm-port-twamp-test

needs to be a Normative Reference from

draft-ietf-ippm-twamp-yang

and should appear in the description clause, replacing the reference to
'new port'

along with a note up-front to the RFC Editor asking them to replace all
mentions of
draft-ietf-ippm-port-twamp-test
with the RFC number of that I-D when it is allocated.

I find that the use of 'new' is rarely a good idea - road signs near me
talk of a 'new' road layout that is now 10 years old - unless there is a
clear date, explicit or implicit associated with it; and the
reallocation of the use of a port is IMHO a significant change that
needs calling out - the YANG module makes references to RFC 5357 and I
think it is asking too much for users to track down another RFC that
updates RFC 5357 in order to see the change.

Tom Petch

---

From: "tom p." <[email-masked]>
Date: Fri, 13 Apr 2018 12:19:19 +0100

Mahesh

Yes, your responses look good.

Tom Petch

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

Tom,

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

> '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.”

[mj] Ditto.

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

[mj] Ok. Will add it.

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

>> which is not quite English.

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

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

[mj] Will add.

[mj] Agree. Will update.

Cheers.

---

From: "MORTON, ALFRED C (AL)" <[email-masked]>
Date: Fri, 13 Apr 2018 12:58:00 +0000

Tom, see end...
> updates RFC 5357 in order to see the change.

[acm] 
I suggest:
The well-known port (862) allocated in RFC YYYY MAY be used.

RFC Editor: replace RFC YYYY with the RFC number of 
draft-ietf-ippm-port-twamp-test when it is allocated.

---

From: "tom p." <[email-masked]>
Date: Fri, 13 Apr 2018 17:12:05 +0100

Al, see end...

Tom Petch

----- Original Message -----
From: "MORTON, ALFRED C (AL)" <[email-masked]>
Sent: Friday, April 13, 2018 1:58 PM

> > > > >> This was allocated in 2008 which seems to stretch the meaning
of
> > ‘new'

Yes, that is fine for the description clause; RFC YYYY will still need
to be in the References of the I-D, since it appears in the YANG module
and those are the rules for a YANG module, and so will need to appear
somewhere in the body of the text as well, lest it is flagged by nits as
an unused reference (Mahesh is already fixing this for RFC1305 and
RFC2330).

I did check with the RFC Editor and they said that they preferred notes
asking them to do replacements to be up-front, in one place, and not
spread throughout the I-D - hence my comment before about up-front.

Tom Petch

---

From: Mahesh Jethanandani <[email-masked]>
Date: Mon, 16 Apr 2018 07:58:21 -0700

Tom,

On this particular comment ….

> On Apr 12, 2018, at 5:13 PM, Mahesh Jethanandani <[email-masked]> wrote:
> 
>> 
>> 

[[mj]] In addition to removing the presence statement, we decided to change the ‘mandatory true' statement to a ‘default true’ statement. Just removing the presence statement results in the following error:

Validating yang/[email-masked] <mailto:yang/[email-masked]>
yang/[email-masked] <mailto:yang/[email-masked]>:445: error: RFC 6087: 4.9: top-level node twamp must not be mandatory

Cheers.

Mahesh Jethanandani
[email-masked]

---

From: "tom p." <[email-masked]>
Date: Tue, 17 Apr 2018 15:37:41 +0100

Mahesh

Getting there.

I still cannot find a Reference in the I-D for RFC 2330 which appears in
the module.

'max-count' I still find open to confusion - I was hoping that you would
rename the leaf, and not the grouping, since it is the leaf, the data
object, that is an exponent.  Further down, you have

"The default
            max-count value SHOULD be 32768.'
"
which I think wrong since the leaf is and was an exponent.  After
renaming the leaf, not the grouping, I was expecting something like
'The default value of max-count-exponent SHOULD be 15 which corresponds
to a maximum value of 2**15 i.e.32768.'

On the presence/ boolean, and top level nodes being mandatory true, your
solution looks fine (I said to you that my knowledge of YANG was limited
and while this restriction has a faint echo in my memory, I cannot find
the paragraph in RFC6020/RFC7950 that spells this out:-(

I am still thinking about secret key.

Tom Petch

----- Original Message -----
From: "Mahesh Jethanandani" <[email-masked]>
To: "tom p." <[email-masked]>
Cc: <[email-masked]>; <[email-masked]>;
<[email-masked]>; <[email-masked]>
Sent: Monday, April 16, 2018 3:58 PM

Tom,

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

[[mj]] In addition to removing the presence statement, we decided to
change the ‘mandatory true' statement to a ‘default true’ statement.
Just removing the presence statement results in the following error:

Validating yang/[email-masked]
<mailto:yang/[email-masked]>
yang/[email-masked]
<mailto:yang/[email-masked]>:445: error: RFC 6087: 4.9:
top-level node twamp must not be mandatory

Cheers.

Mahesh Jethanandani
[email-masked]

---

From: Mahesh Jethanandani <[email-masked]>
Date: Wed, 18 Apr 2018 15:26:20 -0700

> On Apr 17, 2018, at 7:37 AM, tom p. <[email-masked]> wrote:
> 
> Mahesh
> 
> Getting there.
> 

Added a normative reference to RFC 2330.

> object, that is an exponent.

Renamed the leaf also to max-count-exponent.

>  Further down, you have
> to a maximum value of 2**15 i.e.32768.’

I have added the above text in the model.

Thanks.

> From: "Mahesh Jethanandani" <[email-masked] <mailto:[email-masked]>>
> To: "tom p." <[email-masked] <mailto:[email-masked]>>
> Cc: <[email-masked] <mailto:[email-masked]>>; <[email-masked] <mailto:[email-masked]>>;
> <[email-masked] <mailto:[email-masked]>>; <[email-masked] <mailto:[email-masked]>>
> <mailto:yang/[email-masked] <mailto:yang/[email-masked]>>
> yang/[email-masked] <mailto:yang/[email-masked]>
> <mailto:yang/[email-masked] <mailto:yang/[email-masked]>>:445: error: RFC 6087: 4.9:
> [email-masked] <mailto:[email-masked]>
Mahesh Jethanandani
[email-masked]