Re: [Teas] Yangdoctors last call review of draft-ietf-teas-yang-te-topo-08

Robert Wilton <rwilton@cisco.com> Wed, 21 June 2017 09:33 UTC

Return-Path: <rwilton@cisco.com>
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 8B4B3131C9B; Wed, 21 Jun 2017 02:33:49 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -14.502
X-Spam-Level:
X-Spam-Status: No, score=-14.502 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, RCVD_IN_DNSWL_HI=-5, RP_MATCHES_RCVD=-0.001, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001, USER_IN_DEF_DKIM_WL=-7.5] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=cisco.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 kG0U8Hi3dI3O; Wed, 21 Jun 2017 02:33:46 -0700 (PDT)
Received: from aer-iport-1.cisco.com (aer-iport-1.cisco.com [173.38.203.51]) (using TLSv1.2 with cipher DHE-RSA-SEED-SHA (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 95DF4131C9F; Wed, 21 Jun 2017 02:33:41 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=5295; q=dns/txt; s=iport; t=1498037622; x=1499247222; h=subject:to:references:cc:from:message-id:date: mime-version:in-reply-to:content-transfer-encoding; bh=Uh8lzV2yIHZZfxnMoT74xKXwSgWvBg0dGBMqwCreJQQ=; b=Z8ZsqoXqfaaZX9q2HZv21GF/vvsUZwf83DMsu6lk6h9zUMsExr4z6g/a kvpu5T+LPeWlnqrDWt2qNUr8PscgcEKMkuc/ytHiAV6NERaQ89EVdyvjY GHoKfLNGOnYF/KCw/+Fs6FYUh9wmUHRoYlIkK3JQsxUo+waQ9Q2YUTCGk Q=;
X-IronPort-Anti-Spam-Filtered: true
X-IronPort-Anti-Spam-Result: A0AqAQB3PEpZ/xbLJq1dGQEBAQEBAQEBAQEBBwEBAQEBgyuBD4ENg2yKGXOQbCKILI1MghEshXgCgy0YAQIBAQEBAQEBayiFGAEBAQEDIw8BBTMODAQLEQQBAQECAiMDAgIhJQkIBgEMBgIBAYoQAxUQqXGCJoc5DYQWAQEBAQEBAQEBAQEBAQEBAQEBAQEBGAWBC4VigWArC4FigQyCV4UlgkIfAQSeJzuHM4dIhGeLG4Zzi15siEcfOIEKMCEIGxVJhQ0cgWc/NolqAQEB
X-IronPort-AV: E=Sophos;i="5.39,368,1493683200"; d="scan'208";a="695301888"
Received: from aer-iport-nat.cisco.com (HELO aer-core-4.cisco.com) ([173.38.203.22]) by aer-iport-1.cisco.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Jun 2017 09:33:37 +0000
Received: from [10.63.23.55] (dhcp-ensft1-uk-vla370-10-63-23-55.cisco.com [10.63.23.55]) by aer-core-4.cisco.com (8.14.5/8.14.5) with ESMTP id v5L9XaGl021449; Wed, 21 Jun 2017 09:33:36 GMT
Subject: Re: [Teas] Yangdoctors last call review of draft-ietf-teas-yang-te-topo-08
To: Xufeng Liu <Xufeng_Liu@jabil.com>, Mahesh Jethanandani <mjethanandani@gmail.com>, "yang-doctors@ietf.org" <yang-doctors@ietf.org>
References: <149564066257.28529.12761629961042171907@ietfa.amsl.com> <BN3PR0201MB08670E55FFB5470F338E998DF1CD0@BN3PR0201MB0867.namprd02.prod.outlook.com>
Cc: "ietf@ietf.org" <ietf@ietf.org>, "teas@ietf.org" <teas@ietf.org>, "draft-ietf-teas-yang-te-topo.all@ietf.org" <draft-ietf-teas-yang-te-topo.all@ietf.org>
From: Robert Wilton <rwilton@cisco.com>
Message-ID: <5d4541d3-9426-787d-e02b-9c2dbc3a5400@cisco.com>
Date: Wed, 21 Jun 2017 10:33:36 +0100
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <BN3PR0201MB08670E55FFB5470F338E998DF1CD0@BN3PR0201MB0867.namprd02.prod.outlook.com>
Content-Type: text/plain; charset="utf-8"; format="flowed"
Content-Transfer-Encoding: 8bit
Archived-At: <https://mailarchive.ietf.org/arch/msg/ietf/bheDDQC7Em8gVnGQcVrfPMh4iAY>
X-BeenThere: ietf@ietf.org
X-Mailman-Version: 2.1.22
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: Wed, 21 Jun 2017 09:33:50 -0000

Hi Xufeng,

On 12/06/2017 22:28, Xufeng Liu wrote:
> Hi Mahesh,
>
> Thank you much for the review. We have submitted an updated draft (https://tools.ietf.org/html/draft-ietf-teas-yang-te-topo-09) to address these issues. More detailed explanations are put below inline.
>
> If the responses and updates are satisfactory, we are ready for the last call.
>
> Best regards,
> - Xufeng
>
>> -----Original Message-----
>> From: Mahesh Jethanandani [mailto:mjethanandani@gmail.com]
>> Sent: Wednesday, May 24, 2017 11:44 AM
>> To: yang-doctors@ietf.org
>> Cc: ietf@ietf.org; teas@ietf.org; draft-ietf-teas-yang-te-topo.all@ietf.org
>> Subject: Yangdoctors last call review of draft-ietf-teas-yang-te-topo-08
>>
>> Reviewer: Mahesh Jethanandani
>> Review result: Ready with Issues
>>
>> Document reviewed: draft-ietf-teas-yang-te-topo-08
>>
>> Status: Ready with Issues
>>
>> I am not an expert in Traffic Engineering. This review is looking at the draft from
>> a YANG perspective. With that said, I have marked it as “Ready with Issues”
>> because of some of the points discussed below.
>>
>> Summary:
>>
>> This document defines a YANG data model for representing, retrieving and
>> manipulating TE Topologies. The model serves as a base model that other
>> technology specific TE Topology models can augment.
>>
>> Comments:
>>
>> Almost all the containers in the model are presence containers. Is there a reason
>> why they have to be presence containers? Note, that presence containers are
>> containers whose existence itself represents configuration data. What particular
>> configuration data is each container representing in itself?
> [Xufeng] Containers that use “presence” are:
> 	- Container “underlay”
> 	  o  We have changed 13 occurrences of such containers to be not presence container.
> 	- Container “te” under augmentation
> 	  o  To indicate that “TE” is enabled (configuration data)
> 	  o  Also used to do augmentation. The “presence” statement can prevent the mandatory child from affecting augmented base model.
> 	- /nw:networks/nw:network/nw:network-types/te-topology!
> 	  o  A mechanism required by I2RS topology model to specify the topology type.
>
>> It is difficult to co-relate the diagram with the model itself because of different
>> terms being used to define different parts of the model.
>> There is “TE Topology Model” and then there is “Generic TE Topology Model”.
>> Are these one and the same models? If so, a common term for both of them
>> would be helpful.
> [Xufeng] Yes. These two terms are the same. Figure 12, Figure 13, and relevant descriptions have been updated to make the document consistent.
>
>> There is extensive use of groupings in the document. However, not all instances
>> of groupings are used multiple number of times. Where they are not being
>> repeated, it would be better to move the grouping directly where the uses
>> statement resides. Case in point the grouping connectivity-label-restriction-list.
> [Xufeng] We have removed the following groupings
>      te-link-augment
>      te-node-augment
>      te-termination-point-augment
>      te-topologies-augment
>      te-topology-augment
>      te-link-state-underlay-attributes
>      te-node-state-derived-notification
>      te-topology-type
>
> The remaining groupings have been kept so that we can:
> 	- Share the groupings in this model
> 	- Prepare to be shared by a model augmenting this model
> 	- Prevent a grouping or configuration section from being too long
> 	- Improve readability
>
>> The split between config and state containers does not seem to follow any
>> particular pattern.
> [Xufeng] The pattern is clear:
> For each manageable entity (object), there is a config container and state container. The configurable properties go into the config container and state properties go into the state container. Such objects are identified by a list item or a presence container so that the “create”, “delete”, and “modify” operations can be performed on them. The non-presence containers do not represent configuration data so they do not introduce such objects.
>
>> It is neither a top level split, as is the case with existing IETF
>> models,
> [Xufeng] We could not do top level split because the base I2RS network topology model (https://tools.ietf.org/html/draft-ietf-i2rs-yang-network-topo-12) that we augment does not have the top-level split (for its own reasons).
>
>> nor do they follow the OpenConfig style of splitting config and state
>> under each relevant leaf,
> [Xufeng] The pattern is consistent with this style in principle, with some adjustments to fit to our multiple levels of hierarchy.
This is effectively a new forth style of YANG models that is not 
consistent with any of the three existing styles, i.e.:
  - Current IETF config/state split model style
  - NMDA combined config/state tree
  - OpenConfig split config/state containers immediately above the 
config true leaves.

Tooling that it designed to work with OpenConfig models will need 
customization to work with these TE models because the config/state 
containers will not be where the tooling expects them to be.

Thanks,
Rob