Re: [Teas] Yangdoctors early review of draft-ietf-teas-yang-te-types-01

Lou Berger <lberger@labn.net> Thu, 01 November 2018 23:12 UTC

Return-Path: <lberger@labn.net>
X-Original-To: teas@ietfa.amsl.com
Delivered-To: teas@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 6112D12D4EA for <teas@ietfa.amsl.com>; Thu, 1 Nov 2018 16:12:09 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.901
X-Spam-Level:
X-Spam-Status: No, score=-1.901 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, 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 reePO9u9iFnB for <teas@ietfa.amsl.com>; Thu, 1 Nov 2018 16:12:06 -0700 (PDT)
Received: from gproxy8-pub.mail.unifiedlayer.com (gproxy8-pub.mail.unifiedlayer.com [67.222.33.93]) (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 AC2FD12D4E8 for <teas@ietf.org>; Thu, 1 Nov 2018 16:12:06 -0700 (PDT)
Received: from cmgw11.unifiedlayer.com (unknown [10.9.0.11]) by gproxy8.mail.unifiedlayer.com (Postfix) with ESMTP id DA9311AB314 for <teas@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 ILjogQQ22d20TILjogLdvZ; 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)
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: 1
X-Source-Cap: bGFibm1vYmk7bGFibm1vYmk7Ym94MzEzLmJsdWVob3N0LmNvbQ==
X-Local-Domain: yes
Archived-At: <https://mailarchive.ietf.org/arch/msg/teas/lTJgcrTo_h7gB_VuqmG4Zn13d1g>
Subject: Re: [Teas] Yangdoctors early review of draft-ietf-teas-yang-te-types-01
X-BeenThere: teas@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Traffic Engineering Architecture and Signaling working group discussion list <teas.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/teas>, <mailto:teas-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/teas/>
List-Post: <mailto:teas@ietf.org>
List-Help: <mailto:teas-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/teas>, <mailto:teas-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 01 Nov 2018 23:12:09 -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
>