Subject: Re: [ippm] Review of TWAMP YANG model

From: Ladislav Lhotka <[email-masked]>
Date: Wed, 16 Dec 2015 13:50:31 +0100

> On 15 Dec 2015, at 14:34, Reshad Rahman (rrahman) <[email-masked]> wrote:
> 
> Regarding the top-level containers, is this something the yangvalidator
> tool should catch?

Yes, I think so, I already filed an issue:

https://github.com/mbj4668/pyang/issues/190

Lada

> 
> *** Module "ietf-twamp"
>   - Top-level containers "twamp-session-sender" and
>     "twamp-session-reflector" should probably be with presence
>     because otherwise they are mandatory, which is not allowed per
>     RFC 6087, sec. 4.9.
> 
> 
> 
> Regards,
> Reshad.
> 
> 
> 
> On 2015-12-14, 8:47 AM, "Ladislav Lhotka" <[email-masked]> wrote:
> 
>> Hi Mahesh,
>> 
>> here are my comments to draft-cmzrjp-ippm-twamp-yang-02, I am sorry it
>> took me so long.
>> 
>> Overall, I think the I-D and the module are well organized. I am not a
>> TWAMP expert, so I cannot evaluate how the data model fits the
>> domain-specific needs. From the YANG point of view, I discovered one
>> problem with the module and several validity issues with the example
>> instance documents. I also have a couple of suggestions regarding the
>> module. Details follow.
>> 
>> It is useful to always validate, e.g. using pyang and yang2dsdl, the
>> module *and* all example instance data. I have a skeleton project on
>> GitHub where these (and other) tasks are automated through a Makefile:
>> 
>> https://github.com/llhotka/YANG-I-D
>> 
>> Best regards, Lada
>> 
>> ** draft-cmzrjp-ippm-twamp-yang-02
>> *** Module "ietf-twamp"
>>  - Top-level containers "twamp-session-sender" and
>>    "twamp-session-reflector" should probably be with presence
>>    because otherwise they are mandatory, which is not allowed per
>>    RFC 6087, sec. 4.9.
>>  - The "reference" statement should contain "RFC XXXX: Two-Way
>>    Active Measurement Protocol (TWAMP) Data Model" instead of
>>    "draft-cmzrjp-ippm-twamp-yang".
>>  - Ranges for uint32 such as "1024..4294967295" should be written
>>    "1024..max" - the latter is IMO clearer.
>> 
>> *** Instance data examples
>> **** control-client
>>   - boolean value is "true", not "True"
>> **** control-client2
>>   - missing mandatory leaf "client-admin-state"
>> **** server
>>   - boolean value is "true", not "True"
>> **** server2
>>   - missing mandatory leaf "server-admin-state"
>> **** session-sender
>>   - missing mandatory leaf "session-sender-admin-state"
>>   - comment "read-only" should be written (twice) as XML comment, i.e.
>>     <!-- read-only-->
>>   - "packet-distribution" is a "choice" node, so it should not
>>     appear in instance data.
>> **** session-reflector
>>   - missing mandatory leaf "session-reflector-admin-state"
>>   - key elements of the "twamp-reflector-test-session" list should
>>     go first in each list entry and in the same order as they appear
>>     in the "key" statement, see RFC 6020, sec. 7.8.5. For example:
>>       <twamp-reflector-test-session>
>>         <sender-ip>10.1.1.1</sender-ip>
>>         <sender-udp-port>4000</sender-udp-port>
>>         <reflector-ip>10.1.1.2</reflector-ip>
>>         <reflector-udp-port>5000</reflector-udp-port>
>>         <sid>1232</sid>
>>         ...
>>       </twamp-reflector-test-session>
>> 
>> 
>> 
>> Mahesh Jethanandani <[email-masked]> writes:
>> 
>>> Lada,
>>> 
>>> I am following up on our discussion in Yokohama, and in particular my
>>> request to see if you could review the TWAMP YANG model.
>>> 
>>> The team has done a exceptional job of explaining the YANG model using
>>> both a UML diagram and the YANG tree diagram, that you might find
>>> helpful as you review the approach.
>>> 
>>> https://tools.ietf.org/html/draft-cmzrjp-ippm-twamp-yang-02
>>> <https://tools.ietf.org/html/draft-cmzrjp-ippm-twamp-yang-02>
>>> 
>>> Thanks in advance.
>>> 
>>> Mahesh Jethanandani
>>> [email-masked]
>>> 
>>> 
>>> 
>>> 
>>> 
>> 
>> -- 
>> Ladislav Lhotka, CZ.NIC Labs
>> PGP Key ID: E74E8C0C
> 

--
Ladislav Lhotka, CZ.NIC Labs
PGP Key ID: E74E8C0C