Re: [OPSAWG] Yangdoctors early review of draft-ietf-opsawg-vpn-common-02

mohamed.boucadair@orange.com Thu, 14 January 2021 10:13 UTC

Return-Path: <mohamed.boucadair@orange.com>
X-Original-To: opsawg@ietfa.amsl.com
Delivered-To: opsawg@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 6DFCD3A1634; Thu, 14 Jan 2021 02:13:36 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.118
X-Spam-Level:
X-Spam-Status: No, score=-2.118 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-0.01, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, UNPARSEABLE_RELAY=0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=orange.com
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 6EbRO1ihOwQF; Thu, 14 Jan 2021 02:13:32 -0800 (PST)
Received: from relais-inet.orange.com (relais-inet.orange.com [80.12.66.40]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 47F813A162E; Thu, 14 Jan 2021 02:13:29 -0800 (PST)
Received: from opfedar02.francetelecom.fr (unknown [xx.xx.xx.4]) by opfedar22.francetelecom.fr (ESMTP service) with ESMTP id 4DGgBH6Hnqz2y9M; Thu, 14 Jan 2021 11:13:27 +0100 (CET)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=orange.com; s=ORANGE001; t=1610619207; bh=xPwZ8rVTQ0iFMz3VM0ONovlNwUIKtoWkGmD64zEr0nY=; h=From:To:Subject:Date:Message-ID:Content-Type: Content-Transfer-Encoding:MIME-Version; b=JHwaVWRL2NszviimWemfyLahLgge3wT1eIUxxgRQUl+h6aLlYcXfQRGolWZE74pGF XKmmSpWX5gfbwcc9peqCmrdG3ZahA0y5rNhLGZQsYVUnDmFBckLkqCt864WXggH04Q UWr0M4wg3hEk4S5ww1Kd1PpRvCiX6N02yKwT4UboubedJNX8ICy750DUtpgXnvA3++ xG30XnFC9/tUtdQ65VdE1Quz4vSDIE2LISBj8AaKDgPzJOjFa28gNrHOwOswbX3PJZ LfYGSiXvcKo8dabauRczcGEXSEPCJPBeRyC5VQHvIRzLRRIpE+NhbTacy499QojS1f /P/ije7ahXeTg==
Received: from Exchangemail-eme6.itn.ftgroup (unknown [xx.xx.13.101]) by opfedar02.francetelecom.fr (ESMTP service) with ESMTP id 4DGgBH5B4CzCqkh; Thu, 14 Jan 2021 11:13:27 +0100 (CET)
From: <mohamed.boucadair@orange.com>
To: =?utf-8?B?UmFkZWsgS3JlasSNw60=?= <rkrejci@cesnet.cz>, "yang-doctors@ietf.org" <yang-doctors@ietf.org>
CC: "draft-ietf-opsawg-vpn-common.all@ietf.org" <draft-ietf-opsawg-vpn-common.all@ietf.org>, "opsawg@ietf.org" <opsawg@ietf.org>
Thread-Topic: Yangdoctors early review of draft-ietf-opsawg-vpn-common-02
Thread-Index: AQHW1TdUdpF5/StLZEa6DcccodwT8qonD3oQ
Date: Thu, 14 Jan 2021 10:13:26 +0000
Message-ID: <21141_1610619207_60001947_21141_380_5_787AE7BB302AE849A7480A190F8B9330315B9840@OPEXCAUBMA2.corporate.adroot.infra.ftgroup>
References: <160829366323.14887.4241710682101990781@ietfa.amsl.com>
In-Reply-To: <160829366323.14887.4241710682101990781@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.114.13.245]
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: base64
MIME-Version: 1.0
Archived-At: <https://mailarchive.ietf.org/arch/msg/opsawg/cOsVjFPPDQE8K8Xb3sXT-PLAHx0>
Subject: Re: [OPSAWG] Yangdoctors early review of draft-ietf-opsawg-vpn-common-02
X-BeenThere: opsawg@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: OPSA Working Group Mail List <opsawg.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/opsawg>, <mailto:opsawg-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/opsawg/>
List-Post: <mailto:opsawg@ietf.org>
List-Help: <mailto:opsawg-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/opsawg>, <mailto:opsawg-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 14 Jan 2021 10:13:37 -0000

Hi Radek,

All good points. Many thanks. 

All the comments were addressed in a new revision, except the one related to rt-rd/rd-choice. Distinct names are used to distinguish auto-assigned values vs assigned from a pool. 

FWIW, the new revision can be seen at: 

URL:            https://www.ietf.org/archive/id/draft-ietf-opsawg-vpn-common-03.txt 
Status:         https://datatracker.ietf.org/doc/draft-ietf-opsawg-vpn-common/ 
Htmlized:       https://datatracker.ietf.org/doc/html/draft-ietf-opsawg-vpn-common 
Htmlized:       https://tools.ietf.org/html/draft-ietf-opsawg-vpn-common-03 
Diff:           https://www.ietf.org/rfcdiff?url2=draft-ietf-opsawg-vpn-common-03

Cheers,
Med

> -----Message d'origine-----
> De : Radek Krejčí via Datatracker [mailto:noreply@ietf.org]
> Envoyé : vendredi 18 décembre 2020 13:14
> À : yang-doctors@ietf.org
> Cc : draft-ietf-opsawg-vpn-common.all@ietf.org; opsawg@ietf.org
> Objet : Yangdoctors early review of draft-ietf-opsawg-vpn-common-02
> 
> Reviewer: Radek Krejčí
> Review result: Ready with Issues
> 
> *** draft ***
> 
> Seems well written with a clear statement about its purpose and
> describing the groupings of the module.
> 
> *** module ***
> 
> * layout
> Use pyang's `--yang-canonical` option to unify the order of the
> statements and make the order canonical - e.g. starting the
> groupings with description improves readability of the module. Using
> pyang to print the module would also remove probably forgotten
> comment `//L2xMs`.
> 
> * typo
> - Module's description: s/Section 4.c/Section 4/
> 
> * features
> Almost none of the features is actually used in the module, which
> might be fine, but there are identities referring to the same areas
> as the features, so I believe that these identities should have if-
> feature statement referring to the appropriate feature defined in
> the module.
> 
> * enumeration typedefs
> Since the enumeration cannot be extended, are you really sure, that
> the enumeration types you define are really complete forever? I
> would say that address families, negotiation modes and  control
> modes might need extensions in the modules that will use those
> types. Defining it as identityref with specified identities instead
> of enums would solve the problem.
> 
> * `service-status` and `status-timestamp` groupings Both groupings
> seem to have config false meaning.  But only the service-
> status/status/oper-status container is defined as config false. The
> uses statement doesn't have its own config statement, so if you want
> to place the mentioned groupings into config true data, an extra
> grouping or refine will be required. The common sense of the
> groupings is config false, so define them this way. If there will be
> some exception to make them config true, the uses can refine it in
> such an exceptional case.
> 
> * grouping rt-rd/rd-choice
> There are 2 cases (pool-assigned and full-autoassigned) which seem
> to have a node with the same meaning, but since there would be a
> name collision, they have different names `rd-assign` and `rd-
> assigneed`. I don't think that this is a good solution. If they have
> the same meaning, they should have also the same name. I see 2
> possible solutions: - move the leafs in the cases into a container -
> it adds another level in data, but allows a common name `rd-
> assigned` - move the `rd-assigned` leaf outside the choice. If it
> makes sense, it can be constrained by must/when to the (non)-
> presence of the nodes from the choice. The main point here is that
> the rd-assigned status is then always at the same place.
> 
> * grouping group/
> The grouping seems odd to me, like it misses something or it might
> be just by too generic name. When looking into the `placement-
> constraint` grouping, it seems that it refers to the groupings, but
> since they are separated, it is not possible to refer to the group-
> id explicitly. Is the `group` grouping intended to be instantiated
> standalone, without `placement-constraint`? If not, join them
> together and refer the group-id from target-flavor via leafref.
> 
> 


_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.