[Teas] Yangdoctors early review of draft-ietf-teas-yang-te-21

Radek Krejčí via Datatracker <noreply@ietf.org> Thu, 13 June 2019 07:49 UTC

Return-Path: <noreply@ietf.org>
X-Original-To: teas@ietf.org
Delivered-To: teas@ietfa.amsl.com
Received: from ietfa.amsl.com (localhost [IPv6:::1]) by ietfa.amsl.com (Postfix) with ESMTP id E16451200E6; Thu, 13 Jun 2019 00:49:28 -0700 (PDT)
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
From: =?utf-8?q?Radek_Krej=C4=8D=C3=AD_via_Datatracker?= <noreply@ietf.org>
To: <yang-doctors@ietf.org>
Cc: draft-ietf-teas-yang-te.all@ietf.org, teas@ietf.org
X-Test-IDTracker: no
X-IETF-IDTracker: 6.97.0
Auto-Submitted: auto-generated
Precedence: bulk
Reply-To: =?utf-8?b?UmFkZWsgS3JlasSNw60=?= <rkrejci@cesnet.cz>
Message-ID: <156041216883.12414.4305444153384125389@ietfa.amsl.com>
Date: Thu, 13 Jun 2019 00:49:28 -0700
Archived-At: <https://mailarchive.ietf.org/arch/msg/teas/VjIOXZMJ2fF9fli23P_K6bFv-gg>
Subject: [Teas] Yangdoctors early review of draft-ietf-teas-yang-te-21
X-BeenThere: teas@ietf.org
X-Mailman-Version: 2.1.29
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, 13 Jun 2019 07:49:29 -0000

Reviewer: Radek Krejčí
Review result: On the Right Track

This is my YANG doctor review of draft-ietf-teas-yang-te-21 containing 2 YANG
modules: - ietf-te@2019-04-09.yang - ietf-te-device@2019-04-09.yang The draft
and both modules follows NMDA.

Since I'm not expert in the area, the review focuses on YANG-specifics.

Draft text
----------
1. The second paragraph in 3.2  - items of the list are not properly formatted.

Both YANG modules
-----------------

2. Some of the multiline strings (descriptions) are not correctly formatted,
there is:

     "some
     text";

   instead of

     "some
      text";

   Also better consistency in writing description as a sentence (with capital
   first letter and full stop at the end) or not would be nice.

3. Remove WG Chairs from contact information (see
https://tools.ietf.org/html/rfc8407#appendix-B)

ietf-te@2019-04-09.yang
-----------------------

4. grouping path-properties:
path-properties/path-route-objects/path-computed-route-objects
   - The list is defined as ordered-by user, but as a config false item
   (inherited from path-route-objects), the ordered-by statement is ignored
   (RFC 7950: 7.7.7.).

5. grouping p2p-path-properties-common :path-computation-server
   grouping p2p-path-properties-common :use-path-computation
   - Use derived-from-or-self() in when-stmt expression instead of comparing
   string values (RFC 8407: 4.6.2.).

6. There is a number of single-instantiated groupings:
   - p2p-reverse-path-properties
   - p2p-primary-reverse-path-properties
   - p2p-secondary-reverse-path-properties
   - p2p-primary-path-properties
   - p2p-secondary-path-properties
   - hierarchical-link-properties
   - protection-restoration-properties-state
   - ... (mybe the most of all that groupings)
   While it is a good mechanism to allow repeated instantiation of a group of
   statements, it can produce kind of "spaghetti code" - with such a number of
   groupings, it is quite challenging for human readers to follow the schema.
   And one of the YANG advantages is its readability. Please consider, if there
   is some real possibility to reuse groupings somewhere outside the schema
   itself. If yes, then ok - readability is the price for reuseability, but
   otherwise you actually don't get anything for poor readability.

7. grouping p2p-reverse-path-properties: named-path-constraint
   grouping p2p-path-properties: named-path-constraint
   - Use absolute leafref path, in this case you probably don't want to refer
   to something relative to the grouping (or something in the grouping), but
   specifically to /te/globals/... using relative path in this case really
   complicates ability to reuse the grouping somewhere else. Maybe in some
   cases this can be also an indicator of a grouping which is actually not
   supposed to be a separate grouping. - Similarly in the following nodes:
   grouping p2p-dependency-tunnels-properties:
   dependency-tunnels/dependency-tunnel/name grouping
   p2p-path-candidate-secondary-path-config: secondary-path ...

8. grouping protection-restoration-properties-state: lockout-of-normal
   grouping protection-restoration-properties-state: freeze
   - Weird formatting of the descriptions with the first and last empty lines.

9. grouping protection-restoration-properties: protection/hold-off-time
   grouping protection-restoration-properties: restoration/hold-of-time
   - Is there any compatibility reason to have 'milli-seconds' units? If not,
   change it to milliseconds.

10. grouping tunnel-p2p-associations-properties: association-objects(-extended)
    - As mentioned, I'm not an expert in the area, but what is the
    global-source (in contrast to source) and why is it a key of the lists? I
    didn't find it mentioned in RFC4872, so the reference is wrong. - Use lower
    case for the node identifiers (ID, extended-ID - other *-id identifiers in
    the document are lowercase)

11. RPCs and Notifications
    - From the descriptions, it is unclear what exactly the RPCs are supposed
    to do and about what the Notifications are supposed to notify. The
    description "TE tunnels RPC nodes" is completely useless.

ietf-te-device@2019-04-09.yang
------------------------------

12. grouping tunnel-device-config
    - It is not very clear why this grouping is actually defined, why other
    modules should import and instantiate this single-leaf grouping? At least
    the description must be improved. - Similarly, what are the use cases for
    interfaces-rpc and (empty) interfaces-notif (as 11)?

13. grouping te-admin-groups-config:
admin-group-type/named-admin-groups/named-admin-groups/named-admin-group
    grouping te-srlgs-config: srlg-type/named-srlgs/named-srlgs/named-srlg
    - Same as 7.

14. grouping te-srlgs-config: srlg-type/value-srlgs/values/value
    - The range actually covers whole range of the type itself.