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

Jan Lindblad <janl@tail-f.com> Fri, 02 November 2018 06:17 UTC

Return-Path: <janl@tail-f.com>
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 F11EB129C6A; Thu, 1 Nov 2018 23:17:42 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.9
X-Spam-Level:
X-Spam-Status: No, score=-1.9 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, SPF_PASS=-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 cY4N8_UDMxop; Thu, 1 Nov 2018 23:17:41 -0700 (PDT)
Received: from mail.tail-f.com (mail.tail-f.com [46.21.102.45]) by ietfa.amsl.com (Postfix) with ESMTP id D6093127B92; Thu, 1 Nov 2018 23:17:40 -0700 (PDT)
Received: from ams3-vpn-dhcp8101.cisco.com (unknown [173.38.220.49]) by mail.tail-f.com (Postfix) with ESMTPSA id B26091AE0389; Fri, 2 Nov 2018 07:17:36 +0100 (CET)
Content-Type: text/plain; charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\))
From: Jan Lindblad <janl@tail-f.com>
In-Reply-To: <166d1751ff0.27ce.9b4188e636579690ba6c69f2c8a0f1fd@labn.net>
Date: Fri, 2 Nov 2018 07:17:34 +0100
Cc: yang-doctors@ietf.org, draft-ietf-teas-yang-te-types.all@ietf.org, ietf@ietf.org, teas@ietf.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <016A1E3B-47F5-4B81-AEAC-FB8FE9B9204A@tail-f.com>
References: <154090780735.15255.3911131220920609603@ietfa.amsl.com> <166d1751ff0.27ce.9b4188e636579690ba6c69f2c8a0f1fd@labn.net>
To: Lou Berger <lberger@labn.net>
X-Mailer: Apple Mail (2.3445.9.1)
Archived-At: <https://mailarchive.ietf.org/arch/msg/teas/JOvScsoGyRedlt1MviAW-ES7TXI>
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: Fri, 02 Nov 2018 06:17:43 -0000

Lou,

> 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.
...
> 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/

Very good. I didn't know if the referencing modules were ready for review, or if there might be additional referencing modules in the works. Anyway, my point is to suggest to bundle the next review of this module with (one of) the modules that use it.

/jan



> 
> 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
>> 
> 
>