Re: [Teas] Yangdoctors early review of draft-ietf-teas-yang-te-types-01
Lou Berger <lberger@labn.net> Thu, 01 November 2018 22:46 UTC
Return-Path: <lberger@labn.net>
X-Original-To: ietf@ietfa.amsl.com
Delivered-To: ietf@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 4861B12D4E8 for <ietf@ietfa.amsl.com>; Thu, 1 Nov 2018 15:46:57 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.902
X-Spam-Level:
X-Spam-Status: No, score=-1.902 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001, SPF_PASS=-0.001] autolearn=unavailable autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (768-bit key) header.d=labn.net
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 kN5X4hHV2D6G for <ietf@ietfa.amsl.com>; Thu, 1 Nov 2018 15:46:55 -0700 (PDT)
Received: from outbound-ss-579.bluehost.com (outbound-ss-579.bluehost.com [74.220.218.175]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id CB41A129AB8 for <ietf@ietf.org>; Thu, 1 Nov 2018 15:46:55 -0700 (PDT)
Received: from cmgw14.unifiedlayer.com (unknown [10.9.0.14]) by gproxy6.mail.unifiedlayer.com (Postfix) with ESMTP id ECF8C1E06C9 for <ietf@ietf.org>; Thu, 1 Nov 2018 16:46:52 -0600 (MDT)
Received: from box313.bluehost.com ([69.89.31.113]) by cmsmtp with ESMTP id ILjogd1g7vdTuILjogvrBF; Thu, 01 Nov 2018 16:46:52 -0600
X-Authority-Reason: nr=8
DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=labn.net; s=default; h=Content-Transfer-Encoding:Content-Type:MIME-Version:Subject: References:In-Reply-To:Message-ID:Date:CC:To:From:Sender:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=SHozIdOx1tbn3Qt6QxAqCV+J9y9MOZ/ZbPQOlec54F4=; b=S3ayohL7/ibTAuLNO26MKex1WN t91SRfSyc9FdeiZsoSwe508tJBGGTu4RSxpFdo83VS5ZnIrYrxk8xpDOeXFZHVdO0aLhVJdu3NuX5 aFUcKOv7VkMRtwzqdi/yOSZhX;
Received: from mdb2336d0.tmodns.net ([208.54.35.219]:33967 helo=[IPV6:2607:fb90:1812:b1de:0:8:637e:e401]) by box313.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES128-GCM-SHA256:128) (Exim 4.91) (envelope-from <lberger@labn.net>) id 1gILjo-002zlV-C1; Thu, 01 Nov 2018 16:46:52 -0600
From: Lou Berger <lberger@labn.net>
To: Jan Lindblad <janl@tail-f.com>, yang-doctors@ietf.org
CC: draft-ietf-teas-yang-te-types.all@ietf.org, ietf@ietf.org, teas@ietf.org
Date: Thu, 01 Nov 2018 18:46:46 -0400
Message-ID: <166d1751ff0.27ce.9b4188e636579690ba6c69f2c8a0f1fd@labn.net>
In-Reply-To: <154090780735.15255.3911131220920609603@ietfa.amsl.com>
References: <154090780735.15255.3911131220920609603@ietfa.amsl.com>
User-Agent: AquaMail/1.17.0-1318 (build: 101700009)
Subject: Re: [Teas] Yangdoctors early review of draft-ietf-teas-yang-te-types-01
MIME-Version: 1.0
Content-Type: text/plain; format="flowed"; charset="us-ascii"
Content-Transfer-Encoding: 8bit
X-AntiAbuse: This header was added to track abuse, please include it with any abuse report
X-AntiAbuse: Primary Hostname - box313.bluehost.com
X-AntiAbuse: Original Domain - ietf.org
X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12]
X-AntiAbuse: Sender Address Domain - labn.net
X-BWhitelist: no
X-Source-IP: 208.54.35.219
X-Source-L: No
X-Exim-ID: 1gILjo-002zlV-C1
X-Source:
X-Source-Args:
X-Source-Dir:
X-Source-Sender: mdb2336d0.tmodns.net ([IPV6:2607:fb90:1812:b1de:0:8:637e:e401]) [208.54.35.219]:33967
X-Source-Auth: lberger@labn.net
X-Email-Count: 2
X-Source-Cap: bGFibm1vYmk7bGFibm1vYmk7Ym94MzEzLmJsdWVob3N0LmNvbQ==
X-Local-Domain: yes
Archived-At: <https://mailarchive.ietf.org/arch/msg/ietf/Jd2FZS8N7jCExUbQZNu4KKSfcuw>
X-BeenThere: ietf@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: IETF-Discussion <ietf.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/ietf>, <mailto:ietf-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/ietf/>
List-Post: <mailto:ietf@ietf.org>
List-Help: <mailto:ietf-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/ietf>, <mailto:ietf-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 01 Nov 2018 22:46:57 -0000
Jan, Thanks for the quick turnaround on this! I'll wait for the comments to be resolved before submitting for publication. See below for one specific response. ---------- On October 30, 2018 9:57:35 AM Jan Lindblad <janl@tail-f.com> wrote: > Reviewer: Jan Lindblad > Review result: Ready with Issues > > This is my YANG Doctor early review of draft-ietf-teas-yang-te-types, or more > specifically ietf-te-types.yang . I find the module ready with a couple of > issues. I have also noted a few nits. Generally speaking, I'm not sure if > reviewing a -types module with plenty of groupings on its own is the best way > to go about it. The groupings defined here may be used in good or not so good > ways later. You can see how the types/groupings are be used in docs listed on the following page: https://datatracker.ietf.org/doc/draft-ietf-teas-yang-te-types/referencedby/ Thanks again, Lou > > General questions: > > #1: There are many locations in the YANG talking about an "ERO subobject index" > (and once RRO index, record route subobject). What is this, and how is it > supposed to be used? The document is silent on this matter, and I have seen > modules with problems around numeric index leafs much like this earlier. Are > these numbers stable, i.e. remains the same forever? > > #2: There are few leafs (5) with default values given, and none with mandatory. > Probably needs to increase before we get to last call. > > #3: There are several choices in the module that are meant to be augmented with > additional cases. In many instances, this is explicitly spelled out, very good. > If this is meant to happen in all choices, it would be nice to point this out > in every instance. Also, if there are any specific assumptions or > considerations to keep in mind when augmenting in a new technology, please note > that in the description as well. > > Issues and nits: > > #4: Unclear data type > 419: > typedef admin-group { > type binary { > length 4; > > What is the format of this binary? If this is always a 4-byte binary, wouldn't > a numeric type be more user friendly, e.g. uint32? > > #5: identifier in container with optional leafs > 1496: > grouping te-topology-identifier { > > The name suggests this is used as an identifier, but all the leafs are > optional. This is not typical. They are also in a container, precluding them > from being used as list keys. Is that as intended? > > #6: Optional -id leafs again > 1700: > leaf node-id { > leaf link-tp-id { > 1768: > leaf address { > 1783: > leaf node-id { > leaf link-tp-id { > > Leafs that appear to be used as identifiers are optional > > #7: binary length in bits? > > 1731: > leaf as-number { > type binary { > length 16; > 1773: > leaf ip-flags { > type binary { > length 8; > 1805: > leaf label-flags { > type binary { > length 8; > > It appears to me the modeler might have thought the length is given in bits. > The value of length is in bytes, however. > > #8: Must expression copy paste > 1852: > container label-end { > must "not(../label-end/te-label/direction) or " > + "not(te-label/direction) " > + "or ../label-end/te-label/direction = te-label/direction" { > > This must expression appears to have been copied from label-start. In any case, > it always evaluates to true and has no effect. > > #9: Unclear bit field > 1885: > leaf range-bitmap { > type binary; > description > "When there are gaps between label-start and label-end, > this attribute is used to specify the positions > of the used labels."; > } > > Need more information on how to interpret this leaf. Which bits map to what, > and what does the bit field values 0 and 1 indicate? > > #10: Canonical representation > 67: > typedef te-bandwidth { > > The type is based on a string with a pattern allowing hex characters and an > upper or lowercase P. Since the pattern allows multiple representations of the > same underlaying value (0x1p10 presumably means the same as 0x1p0xa and > 0x1P0XA) the question comes up if there is a canonical representation of this > value, e.g. using all lowercase and all hex, or if the string must be > remembered exactly as given by the client. The description could answer this > question. > > #11: Mix of upper and lowercase > The module specifies many enumeration and identity values. Some are all > lowercase. Some are all uppercase. The principle of least astonishment suggests > to pick one and stick with it. YANG recommendations suggest to use all > lowercase when in doubt. > > typedef te-link-direction { > typedef te-label-direction { > typedef te-hop-type { > identity LSP_METRIC_TYPE { > identity LSP_METRIC_RELATIVE { > identity LSP_METRIC_ABSOLUTE { > identity LSP_METRIC_INHERITED { > > Best Regards, > /jan > > > _______________________________________________ > Teas mailing list > Teas@ietf.org > https://www.ietf.org/mailman/listinfo/teas >
- Yangdoctors early review of draft-ietf-teas-yang-… Jan Lindblad
- Re: [Teas] Yangdoctors early review of draft-ietf… Lou Berger
- Re: [Teas] Yangdoctors early review of draft-ietf… Jan Lindblad
- Re: Yangdoctors early review of draft-ietf-teas-y… Tarek Saad (tsaad)
- Re: Yangdoctors early review of draft-ietf-teas-y… Jan Lindblad
- Re: Yangdoctors early review of draft-ietf-teas-y… Tarek Saad (tsaad)
- Re: Yangdoctors early review of draft-ietf-teas-y… Jan Lindblad
- [Teas] not a Yangdoctor review of draft-ietf-teas… tom petch
- Re: [Teas] not a Yangdoctor review of draft-ietf-… Tarek Saad (tsaad)
- Re: [Teas] not a Yangdoctor review of draft-ietf-… tom petch
- Re: [Teas] not a Yangdoctor review of draft-ietf-… Tarek Saad (tsaad)
- Re: [Teas] not a Yangdoctor review of draft-ietf-… tom petch
- Re: [Teas] not a Yangdoctor review of draft-ietf-… Tarek Saad (tsaad)
- RE: [Teas] not a Yangdoctor review of draft-ietf-… Italo Busi
- RE: [Teas] not a Yangdoctor review of draft-ietf-… Ryoo, Jeong-dong