Re: [Softwires] Yangdoctors last call review of draft-ietf-softwire-yang-06

<mohamed.boucadair@orange.com> Tue, 23 October 2018 05:59 UTC

Return-Path: <mohamed.boucadair@orange.com>
X-Original-To: softwires@ietfa.amsl.com
Delivered-To: softwires@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 911ED130E89; Mon, 22 Oct 2018 22:59:58 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.599
X-Spam-Level:
X-Spam-Status: No, score=-2.599 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RCVD_IN_DNSWL_LOW=-0.7, SPF_PASS=-0.001, UNPARSEABLE_RELAY=0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 0ey0xitbQXXC; Mon, 22 Oct 2018 22:59:56 -0700 (PDT)
Received: from orange.com (mta136.mail.business.static.orange.com [80.12.70.36]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 1798E130E90; Mon, 22 Oct 2018 22:59:56 -0700 (PDT)
Received: from opfednr01.francetelecom.fr (unknown [xx.xx.xx.65]) by opfednr26.francetelecom.fr (ESMTP service) with ESMTP id 42fN5Q46rqz10Vh; Tue, 23 Oct 2018 07:59:54 +0200 (CEST)
Received: from Exchangemail-eme2.itn.ftgroup (unknown [xx.xx.31.19]) by opfednr01.francetelecom.fr (ESMTP service) with ESMTP id 42fN5Q3BvwzDq78; Tue, 23 Oct 2018 07:59:54 +0200 (CEST)
Received: from OPEXCLILMA3.corporate.adroot.infra.ftgroup ([fe80::60a9:abc3:86e6:2541]) by OPEXCLILM44.corporate.adroot.infra.ftgroup ([fe80::b08d:5b75:e92c:a45f%18]) with mapi id 14.03.0415.000; Tue, 23 Oct 2018 07:59:54 +0200
From: mohamed.boucadair@orange.com
To: Martin Björklund <mbj@tail-f.com>, "yang-doctors@ietf.org" <yang-doctors@ietf.org>
CC: "softwires@ietf.org" <softwires@ietf.org>, "draft-ietf-softwire-yang.all@ietf.org" <draft-ietf-softwire-yang.all@ietf.org>, "ietf@ietf.org" <ietf@ietf.org>
Thread-Topic: Yangdoctors last call review of draft-ietf-softwire-yang-06
Thread-Index: AQHUZGV23P0mXi3OdECuF60fHv3ryqUsV0rQ
Date: Tue, 23 Oct 2018 05:59:53 +0000
Message-ID: <787AE7BB302AE849A7480A190F8B93302E0198E5@OPEXCLILMA3.corporate.adroot.infra.ftgroup>
References: <153959399912.4621.9236297320162816916@ietfa.amsl.com>
In-Reply-To: <153959399912.4621.9236297320162816916@ietfa.amsl.com>
Accept-Language: fr-FR, en-US
Content-Language: fr-FR
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-originating-ip: [10.168.234.4]
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: base64
MIME-Version: 1.0
Archived-At: <https://mailarchive.ietf.org/arch/msg/softwires/emmtVsHeGjnE9hDgKiBEJ0ocXB4>
Subject: Re: [Softwires] Yangdoctors last call review of draft-ietf-softwire-yang-06
X-BeenThere: softwires@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: softwires wg discussion list <softwires.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/softwires>, <mailto:softwires-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/softwires/>
List-Post: <mailto:softwires@ietf.org>
List-Help: <mailto:softwires-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/softwires>, <mailto:softwires-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 23 Oct 2018 05:59:59 -0000

Hi Martin,

Thank you for the review. 

We released a new revision -08 to address your comments. We will double check the formatting and issue another iteration if needed. 

Please see inline.

Cheers,
Med

> -----Message d'origine-----
> De : Martin Björklund [mailto:mbj@tail-f.com]
> Envoyé : lundi 15 octobre 2018 11:00
> À : yang-doctors@ietf.org
> Cc : softwires@ietf.org; draft-ietf-softwire-yang.all@ietf.org; ietf@ietf.org
> Objet : Yangdoctors last call review of draft-ietf-softwire-yang-06
> 
> Reviewer: Martin Björklund
> Review result: Ready with Issues
> 
> This is my YANG doctor review of draft-ietf-softwire-yang-06.  The
> review focuses on YANG-specifics only, since I am not familiar with
> the technology that is modelled.
> 
> o  I would like to see all Tom Petch's comments in his three replies
>    to the IETF LC for this document addressed.  Especially his comment
>    about using ianatf:tunnel.
> 

[Med] This is fixed in -07. A new tunnel-type parameter is defined to handle this. 

> 
> o  All three YANG modules should follow the common template for IETF
>    YANG modules found in
>    https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis.
> 
>    Specifically, fix the authors list and copyright text.
> 

[Med] Done. 

> 
> o  The term "instance" is used to mean - I think - the "device".

[Med] It is used to mean function rather than device. A device may enable multiple instances of the same function. 

  I
>    didn't find this term in the RFCs 7596, 7597 or 7599.  I suggest
>    you use some other term, since "instance" is quite generic, and is
>    often used to refer to instances of YANG data nodes.
> 
>    Also, does the term "instance" mean the same thing in
>    "algo-instance"?  And in "br-instances"?  "bind-instance"?
> 
> 
[Med] algo-instance means an instance of type algorithm. br-instances denotes a set of br instances, and bind-instance means an instance of type binding. 

> 
> o  In ietf-softwire-common:
> 
>   grouping algorithm-instance {
>     description
>       "Indicates that the instance supports the MAP-E and MAP-T
>       function. The instance advertises the MAP-E/MAP-T feature
>       through the capability exchange mechanism when a NETCONF
>       session is established.";
> 
>   This description does not seem right.  A grouping can't indicate
>   anything.  Also, what is "the MAP-E/MAP-T feature"?
> 

[Med] Fixed.

> 
> o  In ietf-softwire-ce:
> 
>   A similar description is found here:
> 
>         container algo-instances {
>           description
>             "Indicates that the instances supports the MAP-E and MAP-T
>             function. The instances advertise the MAP-E/MAP-T
>             feature through the capability exchange mechanism
>             when a NETCONF session is established.";
> 
>   same comments apply.
> 

[Med] Fixed. 

> 
> o  In ietf-softwire-common:
> 
>     container algo-versioning {
>       description "algorithm's version";
>       leaf version {
>         type uint64;
>         description "Incremental version number for the algorithm";
>       }
>       leaf date {
>         type yang:date-and-time;
>         description "Timestamp to the algorithm";
>       }
>     }
> 
>   Maybe these descriptions are crystal clear to someone who knows the
>   technology.  If so, perhaps add a reference to help the rest of us?
> 

[Med] This is used for logging purposes. A reference to RFC7422 is added. 

> 
> o  In general, it would help if the definitions had "reference"
>    statements to the relevant sections in the underlying RFCs.
> 
> 
> o  In ietf-softwire-ce:
> 
>   notification softwire-ce-event {
>     if-feature binding;
>     description "CE notification";
>     leaf ce-binding-ipv6-addr-change {
>       type inet:ipv6-address;
>       mandatory true;
>       description
>         "If the CE's binding IPv6 address changes for any reason,
>         it should notify the NETCONF client.";
>     }
> 
>   Perhaps rewrite the description to:
> 
>     This notification is generated whenever the CE's binding IPv6
>     address changes for any reason.
> 

[Med] OK.

> 
> o  In ietf-softwire-br:
> 
>   You have:
> 
>             leaf softwire-num-max
>             leaf softwires-payload-mtu
>             leaf softwire-path-mru
> 
>    Any reason it isn't "softwire-payload-mtu"?
> 

[Med] No. Fixed already in -07.

> 
> o  In ietf-softwire-br:
> 
>           list bind-instance {
>             key "id";
>             [...]
>             container binding-table-versioning { ... }
>             leaf id { ... }
> 
>   I suggest you put the leaf "id" before the container; it is common
>   practise to put the key leafs first.
> 

[Med] Fixed. 

>   Also, the leaf id has the description:
> 
>       description "An instance identifier.";
> 
>   This again can be confused with YANG's "instance-identifier".
> 

[Med] chnaged to:  

              "A binding instance identifier.

               This identifier can be automatically assigned
              or explicitly configured.";

>   Also, it seems each "instance" has a numerical id (the key), but
>   also a name (a string).  Maybe there are protocol-reasons to do
>   this, but if not, did you consider using the "name" as key instead?
> 

[Med] id/name is inspired from how NAT instances are managed (see RFC7659). The name is optional. 

> 
> o  In ietf-softwire-br:
> 
>   notification softwire-binding-instance-event {
>     if-feature binding;
>     description "Notifications for binding instance.";
> 
>   notification softwire-algorithm-instance-event {
>     if-feature algorithm;
>     description "Notifications for algorithmic instance.";
> 
> 
>   I suggest that the descriptions are updated to describe when these
>   notifications are generated.
> 

[Med] Done.  

> 
> o  Appendix A
> 
>   The examples have:
> 
>     <softwire-config xmlns="urn:ietf:params:xml:ns:yang:ietf-softwire-br">
> 
>   which isn't present in the module.  Maybe a leftover?
> 

[Med] Ooops. We used to have that before moving to an nmda-compliant module.

> 
> o  Formatting
> 
>   To save the RFC editor some cycles, I suggest you format the modules
>   consistently wrt. whitespaces/newlines and indentation.  One option
>   is to run:
> 
>     pyang -f yang --keep-comments <filename>
> 
>   and then manually fix the long leafref paths that pyang can't handle
>   properly at the moment.
> 

[Med] Will double check this one. 

> 
> /martin
>