Re: [yang-doctors] Yangdoctors early review of draft-ietf-softwire-dslite-yang-02
<mohamed.boucadair@orange.com> Mon, 09 October 2017 07:39 UTC
Return-Path: <mohamed.boucadair@orange.com>
X-Original-To: yang-doctors@ietfa.amsl.com
Delivered-To: yang-doctors@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 246AC134A63; Mon, 9 Oct 2017 00:39:22 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.619
X-Spam-Level:
X-Spam-Status: No, score=-2.619 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-0.01, SPF_PASS=-0.001, UNPARSEABLE_RELAY=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 YVemQ6ogtWFe; Mon, 9 Oct 2017 00:39:19 -0700 (PDT)
Received: from relais-inet.orange.com (mta241.mail.business.static.orange.com [80.12.66.41]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 3F51B134B26; Mon, 9 Oct 2017 00:39:19 -0700 (PDT)
Received: from opfedar00.francetelecom.fr (unknown [xx.xx.xx.11]) by opfedar24.francetelecom.fr (ESMTP service) with ESMTP id 6CD19C046E; Mon, 9 Oct 2017 09:34:06 +0200 (CEST)
Received: from Exchangemail-eme2.itn.ftgroup (unknown [xx.xx.31.69]) by opfedar00.francetelecom.fr (ESMTP service) with ESMTP id 4CA7B18006B; Mon, 9 Oct 2017 09:34:06 +0200 (CEST)
Received: from OPEXCLILMA3.corporate.adroot.infra.ftgroup ([fe80::60a9:abc3:86e6:2541]) by OPEXCLILMA2.corporate.adroot.infra.ftgroup ([fe80::bc1c:ad2f:eda3:8c3d%18]) with mapi id 14.03.0361.001; Mon, 9 Oct 2017 09:34:06 +0200
From: mohamed.boucadair@orange.com
To: Mahesh Jethanandani <mjethanandani@gmail.com>, "yang-doctors@ietf.org" <yang-doctors@ietf.org>
CC: "softwires@ietf.org" <softwires@ietf.org>, "draft-ietf-softwire-dslite-yang.all@ietf.org" <draft-ietf-softwire-dslite-yang.all@ietf.org>
Thread-Topic: Yangdoctors early review of draft-ietf-softwire-dslite-yang-02
Thread-Index: AQHTPwGhj6PXHsGqNUWyvBEfCKn+raLbHGzw
Date: Mon, 09 Oct 2017 07:34:05 +0000
Message-ID: <787AE7BB302AE849A7480A190F8B93300A051070@OPEXCLILMA3.corporate.adroot.infra.ftgroup>
References: <150733543329.11064.7489083325636357552@ietfa.amsl.com>
In-Reply-To: <150733543329.11064.7489083325636357552@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.6]
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: base64
MIME-Version: 1.0
Archived-At: <https://mailarchive.ietf.org/arch/msg/yang-doctors/tsrtpdHHMxbLD9wTFlirJfi_GG4>
Subject: Re: [yang-doctors] Yangdoctors early review of draft-ietf-softwire-dslite-yang-02
X-BeenThere: yang-doctors@ietf.org
X-Mailman-Version: 2.1.22
Precedence: list
List-Id: Email list of the yang-doctors directorate <yang-doctors.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/yang-doctors/>
List-Post: <mailto:yang-doctors@ietf.org>
List-Help: <mailto:yang-doctors-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 09 Oct 2017 07:39:22 -0000
Dear Mahesh, Thank you very much for the detailed review. Much appreciated. It seems that you reviewed -02 of the draft, while the latest version is -06 (https://datatracker.ietf.org/doc/draft-ietf-softwire-dslite-yang/). As you can see below, almost all the comments do not apply for -06. Please see inline. Cheers, Med > -----Message d'origine----- > De : Mahesh Jethanandani [mailto:mjethanandani@gmail.com] > Envoyé : samedi 7 octobre 2017 02:17 > À : yang-doctors@ietf.org > Cc : softwires@ietf.org; draft-ietf-softwire-dslite-yang.all@ietf.org; > ietf@ietf.org > Objet : Yangdoctors early review of draft-ietf-softwire-dslite-yang-02 > > Reviewer: Mahesh Jethanandani > Review result: On the Right Track > > Document reviewed: draft-ietf-softwire-dslite-yang-02. Do not know why I > was > assigned -02 version when I now notice that there are several 03 versions > submitted all the way up to -06. > > Status: On the Right Track. > > I am not an expert in DS-Lite. This review is looking at the draft from a > YANG > perspective. With that said, I have marked it as “On the Right Track” > because > of some of the points discussed below. The authors are encouraged to spend > time > looking at other models for examples on how to write the model, read > RFC6087, > Guidelines for YANG documents. > > Summary: > > This document defines a YANG data model for the DS-Lite Address Family > Transition Router (AFTR) and Basic Bridging BroadBand (B4) elements . > > Overall Comments: > > The draft should refer to YANG 1.1 (RFC 7950) instead of RFC6020. [Med] Can fix this one. > > The draft is not NMDA compliant. It carries a separate -state container > that > needs to be collapsed into one single container. For example, it carries > containers such as aftr-current-config which seems to be carrying state > information for the leafs in dslite-config. [Med] There are no such containers in -06. > > Is it given that a server will implement both AFTR and B4? [Med] No. If not, then > please > define feature statements and use if-feature for each part (AFTR and B4), > so > implementors can choose what part of the model is implemented. > > Section 1.1 Terminology > > What terms are defined in RFC 6333 that are being used in this draft. > Please be > explicit. [Med] Actually, all the terms defined in Section 3 of RFC 6333 are used in this document. I added an explicit pointer to Section 3. > > Section 1.2 Tree Diagram > > Please refer to > https://tools.ietf.org/html/draft-ietf-netmod-yang-tree-diagrams-01 > instead of > defining the nomenclature for tree diagram in the document. [Med] Done. Thanks. > > Section 2. > > s/can:/can [Med] There is no such occurrence in -06. > s/provisioned to the AFTR/provisioned on the AFTR/ [Med] There is no such occurrence in -06. > > The document is very light in the architectural description of the YANG > model > itself. Statements like “model supports enabling one or more instances of > the > AFTR function on a device” are obvious looking at the model. A more > reasonable > description would be why multiple instances need to be supported. The same > is > true for “AFTR instance”. The fact that the instance has been defined as a > list > assure that each instance can be provisioned with dedicated configuration > data, > and maintain its own data table. Again why the multiple instances need to > be > maintained, what purpose do they serve, and the fact that they maintain > their > own data table is not clear. > > The document “assumes [RFC4787] [RFC5382][RFC 5508] are enabled”. What > particular aspects of those documents is the draft assuming? Note, RFC4787 > is > NAT Behavioral Requirements for Unicast UDP, and outlines some seven > behaviors, > all the way from NAT and Port Translation, Filtering, Hairpinning to > Fragmentation of Outgoing Packets. Even if all the behaviors are assumed, > it > would be good to know how they impact this particular model. [Med] What it meant is that all the recommendations are supported + all recommended values are the ones defined in those RFCs. > > Section 3 > > General Comments. > > Please follow [RFC6087], Guidelines for YANG documents, on how to write a > YANG > model. For example, the organization in the model should be IETF Softwire > WG, > not just Softwire WG. > > The indentation in the model is off in a few places (reference is a > keyword > that should be at the same indentation as description in all revision > statements). Most models follow an indentation of 2 characters for every > subsequent depth in the model. I see anywhere between 2 to 10 characters > of > indentation. > [Med] Will double check. Thanks. > As far as naming conventions go, there is little value repeating names in > a > given hierarchy. For example, if the grouping is aftr-parameters, then it > is a > given that parameters inside the grouping are aftr parameters. No need to > say > dslite-aftr-ipv6-address, where both dslite and aftr are redundant. > [Med] Please note that this does not apply for -06. > There is little or no use of must statements to qualify the configuration. > I am > not an expert at DS-Lite, so I cannot suggest where a must statement may > be > helpful to have. But with all the parameters that are optional, there must > be > some relationship between the attributes that could be captured with must > statements. For example, for the leaf max-softwire-per-subscriber, the > leaf is > trying to prevent a misbehaving subscriber. Could that behavior be somehow > captured with a must statement for the resources it can/cannot consume. Or > with > port-presevation-enable and port-parity-preservation-enable? Is there a > relationship between the two variables that could be captured in the > model? > > All references to RFC should be moved under a reference section. [Med] Please check -06. All references are under a reference section. For > example: > OLD: > leaf udp-lifetime { > type uint32; > units "seconds"; > default 120; > description > "UDP inactivity timeout [RFC4787]."; > } > NEW: > leaf udp-lifetime { > type uint32; > units "seconds"; > default 120; > description > "UDP inactivity timeout.“; > reference > “[RFC4787].”; > } > > A note should be prepared for the RFC editor to indicate what revision the > model should finally carry, and what the description/reference statement > in the > model should look like when the document is finally published. See > examples in > other drafts such as draft-ietf-netconf-zerotouch Editorial Note on what > to add. > > Specific Comments > > The name of the file in the line with <CODE BEGINS> is missing the .yang > suffix > in the file. When extracted, the extracted file will be missing the > suffix. > Please add it. This leads to the following error from yanglint. [Med] This was fixed in previous version. > > ietf-dslite@2017-01-03" without file extension - unknown format. > > In grouping port-number, could the port-range be something as simple as > > container port-number { > must “starting-port”; > leaf starting-port { > type inet:port-number; > } > leaf ending-port { > type inet:port-number; > } > } > > where if there is no ending-port it is deemed to be a single port, but if > there > is a ending-port is considered to be a range? [Med] This does not apply for -06. > > The list transport-protocol has one leaf which is the transport-protocol- > id. > Why does the single leaf, which is the key, need to be inside a list? [Med] Idem as above. > > Logging-info has a choice statement for protocol. Is it that there can be > only > one choice for the form of logging? Could it be possible that users might > want > to enable both syslog and ipfix? [Med] Idem as above. > > A pyang compilation of the model with —ietf and —lint option was clean. > > A idnits run of the draft reveals some issues with spacing and references. > The > one related to spacing are parts of the diagram, which confuses idnits. [Med] Will double check those. Thanks. > > Checking nits according to https://www.ietf.org/id-info/checklist : > ------------------------------------------------------------------------ > ---- > > ** There are 5 instances of too long lines in the document, the longest > one > being 3 characters in excess of 72. > > == There are 2 instances of lines with non-RFC6890-compliant IPv4 > addresses > in the document. If these are example addresses, they should be > changed. > > Miscellaneous warnings: > ------------------------------------------------------------------------ > ---- > > == Line 162 has weird spacing: '...ocol-id uin...' > > == Line 207 has weird spacing: '...address ine...' > > == Line 215 has weird spacing: '...address ine...' > > == Line 238 has weird spacing: '...rvation boo...' > > == Line 261 has weird spacing: '...ocol-id uin...' > > == (5 more instances...) > > Checking references for intended status: Proposed Standard > ------------------------------------------------------------------------ > ---- > > (See RFCs 3967 and 4897 for information about using normative > references > to lower-maturity documents in RFCs) > > == Unused Reference: 'RFC7753' is defined on line 1988, but no explicit > reference was found in the text > > == Outdated reference: A later version (-04) exists of > draft-boucadair-pcp-yang-03 > > Summary: 1 error (**), 0 flaws (~~), 9 warnings (==), 1 comment (--). >
- [yang-doctors] Yangdoctors early review of draft-… Mahesh Jethanandani
- Re: [yang-doctors] Yangdoctors early review of dr… mohamed.boucadair
- Re: [yang-doctors] Yangdoctors early review of dr… Mahesh Jethanandani
- Re: [yang-doctors] Yangdoctors early review of dr… mohamed.boucadair
- Re: [yang-doctors] Yangdoctors early review of dr… Mahesh Jethanandani
- Re: [yang-doctors] Yangdoctors early review of dr… mohamed.boucadair
- Re: [yang-doctors] Yangdoctors early review of dr… Benoit Claise