Re: [pim] Yangdoctors last call review of draft-ietf-pim-igmp-mld-yang-07

Guofeng <guofeng@huawei.com> Thu, 16 August 2018 02:49 UTC

Return-Path: <guofeng@huawei.com>
X-Original-To: pim@ietfa.amsl.com
Delivered-To: pim@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id C4205130EC7; Wed, 15 Aug 2018 19:49:57 -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, SPF_PASS=-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 8ygVRCV59MHu; Wed, 15 Aug 2018 19:49:56 -0700 (PDT)
Received: from huawei.com (lhrrgout.huawei.com [185.176.76.210]) (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 0D66212F1A2; Wed, 15 Aug 2018 19:49:56 -0700 (PDT)
Received: from lhreml705-cah.china.huawei.com (unknown [172.18.7.106]) by Forcepoint Email with ESMTP id 4DC179359C877; Thu, 16 Aug 2018 03:49:52 +0100 (IST)
Received: from DGGEMM422-HUB.china.huawei.com (10.1.198.39) by lhreml705-cah.china.huawei.com (10.201.108.46) with Microsoft SMTP Server (TLS) id 14.3.399.0; Thu, 16 Aug 2018 03:49:52 +0100
Received: from DGGEMM527-MBX.china.huawei.com ([169.254.6.217]) by dggemm422-hub.china.huawei.com ([10.1.198.39]) with mapi id 14.03.0399.000; Thu, 16 Aug 2018 10:49:50 +0800
From: Guofeng <guofeng@huawei.com>
To: Jan Lindblad <janl@tail-f.com>, "yang-doctors@ietf.org" <yang-doctors@ietf.org>
CC: "ietf@ietf.org" <ietf@ietf.org>, "pim@ietf.org" <pim@ietf.org>, "draft-ietf-pim-igmp-mld-yang.all@ietf.org" <draft-ietf-pim-igmp-mld-yang.all@ietf.org>
Thread-Topic: Yangdoctors last call review of draft-ietf-pim-igmp-mld-yang-07
Thread-Index: AQHUMxLGMeqvxFBTqEOtoXQkyfOjI6TBq0uw
Date: Thu, 16 Aug 2018 02:49:49 +0000
Message-ID: <26C188D59156FB48A93A72ACF12DE0A5B1859613@DGGEMM527-MBX.china.huawei.com>
References: <153417087203.25020.10120277992606371332@ietfa.amsl.com>
In-Reply-To: <153417087203.25020.10120277992606371332@ietfa.amsl.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-originating-ip: [10.111.201.94]
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: base64
MIME-Version: 1.0
X-CFilter-Loop: Reflected
Archived-At: <https://mailarchive.ietf.org/arch/msg/pim/mWo3xUuEbEyWnw3U5ca38rNktUc>
Subject: Re: [pim] Yangdoctors last call review of draft-ietf-pim-igmp-mld-yang-07
X-BeenThere: pim@ietf.org
X-Mailman-Version: 2.1.27
Precedence: list
List-Id: Protocol Independent Multicast <pim.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/pim>, <mailto:pim-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/pim/>
List-Post: <mailto:pim@ietf.org>
List-Help: <mailto:pim-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/pim>, <mailto:pim-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 16 Aug 2018 02:49:58 -0000

Hi Jan,

Thanks for your review, we (Mcast Design Team) will discuss your review comments and update the draft.

Feng

-----Original Message-----
From: Jan Lindblad [mailto:janl@tail-f.com] 
Sent: Monday, August 13, 2018 10:35 PM
To: yang-doctors@ietf.org
Cc: ietf@ietf.org; pim@ietf.org; draft-ietf-pim-igmp-mld-yang.all@ietf.org
Subject: Yangdoctors last call review of draft-ietf-pim-igmp-mld-yang-07

Reviewer: Jan Lindblad
Review result: On the Right Track

This is my YANG-doctor review of draft-ietf-pim-igmp-mld-yang-07. In the spring, I did an early review of the -02 version.

Most of the comments from the earlier review are still valid. In some ways the document has progressed since -02, in many it has not, and in a few it has deteriorated. In my judgement, the document is not ready for last call. Many fundamentally important questions are still unresolved. Here are my review comments in rough falling order of importance.

#1 Improper augment of /rt:routing/rt:control-plane-protocols

Quoted from section 3.1:
   This model augments the core routing data model "ietf-routing"
   specified in [RFC8349].  The IGMP model augments "/rt:routing/
   rt:control-plane-protocols" as opposed to augmenting "/rt:routing/
   rt:control-plane-protocols/rt:control-plane-protocol", as the latter
   would allow multiple protocol instances, while the IGMP protocol is
   designed to be enabled or disabled as a single protocol instance on
   a network instance or a logical network element.

The description above, and the actual augment statements in the YANG module violate the principles described in RFC 8349, the ietf-routing.yang module it augments. In RFC 8349, section 5.3.  Control-Plane Protocol, the proper way of augmenting the routing module is described. The fact that this is a singleton protocol instance doesn't change this. Section 5.3 describes singleton cases as well.

#2 Incorrect vendor refinement model

Quoted from section 2.2:
   For the same reason, wide constant ranges (for example, timer
   maximum and minimum) will be used in the model.  It is expected that
   vendors will augment the model with any specific restrictions that
   might be required. Vendors may also extend the features list with
   proprietary extensions.

This is not acceptable. The principle suggested does not foster interoperability and useful standards. It is also not possible to do what the paragraph suggests in YANG. This was pointed out in the -02 review, and a suggestion was given there on how to address the problem.

#3 Top level structures not optional

Quoted from section 2.3:
   The current document contains IGMP and MLD as separate schema
   branches in the structure. The reason for this is to make it easier
   for implementations which may optionally choose to support specific
   address families. And the names of objects may be different between
   the IPv4 (IGMP) and IPv6 (MLD) address families.

This problem was also pointed out in the -02 review. The author suggests that implementing igmp and/or mld is optional. This is not reflected in the YANG module, however. As currently modeled, both are currently mandatory to implement. If-feature is used liberally in the module, and could be used here as well.

#4 Unclear meaning of optional leaves

Quoted from section 3.1:
   Where fields are not genuinely essential to protocol operation, they
   are marked as optional. Some fields will be essential but have a
   default specified, so that they need not be configured explicitly.

In fact, in the current version of the module, every leaf is optional (except keys, which cannot be optional). It is good to see the addition of defaults in many cases, but many unclear cases remain. E.g. leaf /igmp/global/enable is of type boolean. I understand what true and false implies for this leaf. But what does it mean if it is not set at all? Either add a default or describe the meaning in the description. Similarly, if the leaf version is not set on an igmp or mld interface, or on the interface-global level, what does that mean?
Add default. require-router-alert? explicit-tracking? exclude-lite? Many of these are used in NP-containers inheriting all the from the root, which makes the use of mandatory highly discouraged in the current form. If the RFC 8349 augmentation principles are followed, the concern around mandatory falls, and some leafs with no sensible default could be marked mandatory instead.

#5 All optional state

All state data is optional, which means a conforming server could very well decide not to implement it. E.g. discontinuity-time is optional. Should a manager count on this being available? A situation where every leaf is optional is as nice and flexible for server implementors as it is frustrating and complicated for manager implementors to consume. A YANG model is an API contract and should consider the needs of both sides. The way this has been designed reveals that no representation for the consumer side of this model has been involved in the design. I would suggest thinking through what is the most essential state data for a manager, and make some leafs mandatory.

#6 Abundant copy-paste

There is abundant repetition in the YANG module. leaf version is defined 2 times for igmp with identical definitions, and two more for mld with identical definitions. leaf enable is defined once for the interface global-level, and with identical definition on the interface local level. leaf last-member-query-interval, query-interval and half a dozen other leaves are defined twice with identical definitions.

#7 Leaf interface in the rpc clear*groups on line 1124, 1094 has type string.
Should be a leafref? Describe what values are valid. #8 Leaf group-policy, source-policy on line 486, 527, 624, 689: type string. Should be leafref?
Describe what values are valid. #9 Leaf group on line 705, 1101, 1131: Is any
ipv4/6 address ok, or only a multicast address? Model accordingly.