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

"Reshad Rahman (rrahman)" <rrahman@cisco.com> Tue, 11 September 2018 15:32 UTC

Return-Path: <rrahman@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 4B968130EA4; Tue, 11 Sep 2018 08:32:18 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -14.51
X-Spam-Level:
X-Spam-Status: No, score=-14.51 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, SPF_PASS=-0.001, T_DKIMWL_WL_HIGH=-0.01, 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 nUuryMt4z-HF; Tue, 11 Sep 2018 08:32:13 -0700 (PDT)
Received: from rcdn-iport-2.cisco.com (rcdn-iport-2.cisco.com [173.37.86.73]) (using TLSv1.2 with cipher DHE-RSA-SEED-SHA (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 63F22130EA1; Tue, 11 Sep 2018 08:32:13 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=55964; q=dns/txt; s=iport; t=1536679933; x=1537889533; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-id:content-transfer-encoding: mime-version; bh=UCZA0z6H/fHWJMwRXXWyP4vALtGGy+aUPntjGnBwvd0=; b=PiBMt3u4QLrbQr67CFOkPPYQNg6WKu61o5ZJ0WHtkDGET27LTI1uQbLq ET1TRkxfVcu1oQRJHBwrrs9E+eaQzfugUjYKP0HOQ1Z28SvHhYwD4XElI h0J47WxYF/otslx6rFeZk6BeTY0FdHonPYEUFgp0SiS6+0bWwtrxuXsCn U=;
X-IronPort-Anti-Spam-Filtered: true
X-IronPort-Anti-Spam-Result: A0CmAgBY35db/4kNJK1SCRkBAQEBAQEBAQEBAQEHAQEBAQGDTmV/KAqDaIgTjCKCDYJZZIUgjV2BdwMLGAsJhEACF4N2ITQYAQIBAQIBAQJtHAyFOAEBAQECAQEBGAkROgsFBwQCAQYCDgMBAgEBAQECAiMDAgICHwYLFAECBggCBAENBRuDBgGBaQMNCA+JbZtLgS6HKA2CT4ELiVwXgUE/gRInH4FOfoJWRQEBgTcUHRAjgkcxggQiAohOhHeNfiMrCQKGOIJ+gzoBgxMXggiMbYwlgRuGMQIRFIElHTiBVXAVOyoBgkEJghwXEYM0hRSFPm8BgRWKfiuBAQGBHQEB
X-IronPort-AV: E=Sophos;i="5.53,360,1531785600"; d="scan'208";a="454013433"
Received: from alln-core-4.cisco.com ([173.36.13.137]) by rcdn-iport-2.cisco.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Sep 2018 15:32:11 +0000
Received: from XCH-RCD-001.cisco.com (xch-rcd-001.cisco.com [173.37.102.11]) by alln-core-4.cisco.com (8.15.2/8.15.2) with ESMTPS id w8BFWAeg005800 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=FAIL); Tue, 11 Sep 2018 15:32:11 GMT
Received: from xch-rcd-005.cisco.com (173.37.102.15) by XCH-RCD-001.cisco.com (173.37.102.11) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Tue, 11 Sep 2018 10:32:10 -0500
Received: from xch-rcd-005.cisco.com ([173.37.102.15]) by XCH-RCD-005.cisco.com ([173.37.102.15]) with mapi id 15.00.1395.000; Tue, 11 Sep 2018 10:32:09 -0500
From: "Reshad Rahman (rrahman)" <rrahman@cisco.com>
To: Martin Bjorklund <mbj@tail-f.com>, "xufeng.liu.ietf@gmail.com" <xufeng.liu.ietf@gmail.com>
CC: "yang-doctors@ietf.org" <yang-doctors@ietf.org>, "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>
Subject: Re: [yang-doctors] Yangdoctors last call review of draft-ietf-pim-igmp-mld-yang-07
Thread-Topic: [yang-doctors] Yangdoctors last call review of draft-ietf-pim-igmp-mld-yang-07
Thread-Index: AQHUMxLL9uMNEGlSi0aJ5xMpk+NSZqTnJZQAgAR6yoD//9N0gA==
Date: Tue, 11 Sep 2018 15:32:09 +0000
Message-ID: <D6D5696E-8E97-4F4B-82A7-FF6DADA6AEBD@cisco.com>
References: <153417087203.25020.10120277992606371332@ietfa.amsl.com> <CAEz6PPT6UFPUdFVm0ACM3vLh9vXt1dtJjtgrbEZ=hiDPmy=Q3g@mail.gmail.com> <20180911.161135.1550814850585221588.mbj@tail-f.com>
In-Reply-To: <20180911.161135.1550814850585221588.mbj@tail-f.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
user-agent: Microsoft-MacOutlook/10.b.0.180311
x-ms-exchange-messagesentrepresentingtype: 1
x-ms-exchange-transport-fromentityheader: Hosted
x-originating-ip: [161.44.212.83]
Content-Type: text/plain; charset="utf-8"
Content-ID: <2A5EA24583484D4599EEDFB012FE51E9@emea.cisco.com>
Content-Transfer-Encoding: base64
MIME-Version: 1.0
X-Outbound-SMTP-Client: 173.37.102.11, xch-rcd-001.cisco.com
X-Outbound-Node: alln-core-4.cisco.com
Archived-At: <https://mailarchive.ietf.org/arch/msg/ietf/rKiKaFPNtZHcx886cK70oVzXa8U>
X-BeenThere: ietf@ietf.org
X-Mailman-Version: 2.1.29
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: Tue, 11 Sep 2018 15:32:18 -0000

+1

On 2018-09-11, 10:11 AM, "yang-doctors on behalf of Martin Bjorklund" <yang-doctors-bounces@ietf.org on behalf of mbj@tail-f.com> wrote:

    Hi,
    
    Xufeng Liu <xufeng.liu.ietf@gmail.com> wrote:
    > Hi Jan,
    > 
    > Thanks for the review and valuable comments.
    > 
    > In regard to item #1, there was a discussion thread among the Yang Doctors,
    > authors of RFC 8349, and Routing Area Yang Architecture Design Team, as
    > attached below.  The discussion occurred during the review of a draft with
    > the same issue as this one.
    
    
    But there was no clear outcome of that discussion.  In fact, it seems
    that most people in that thread argued to NOT do a special augment,
    but instead augement the "control-plane-protocol" list, and state that
    there can only be a single entry of this type.
    
    
    /martin
    
    
    
    > 
    
    > Thanks,
    > - Xufeng
    > 
    > ================================
    > 原始邮件
    > 发件人:XufengLiu <Xufeng_Liu@jabil.com>
    > 收件人:Acee Lindem (acee) <acee@cisco.com>Christian Hopps
    > <chopps@chopps.org>Martin
    > Bjorklund <mbj@tail-f.com>
    > 抄送人:张征00007940;yang-doctors@ietf.org <yang-doctors@ietf.org>
    > 日 期 :2018年02月20日 22:30
    > 主 题 :RE: [yang-doctors] How to restrict to have
    > singlecontrol-plane-protocol instance
    > Using "" as the name is better, but I am not sure that it is good enough.
    > When we use ConfD to translate the model to a command line, if the option
    > "tailf:cli-expose-key-name" is not used, we will have:
    > 
    > edit routing control-plane-protocols control-plane-protocol type msdp name
    > ''"
    > 
    > If the option "tailf:cli-expose-key-name" is used, we will have:
    > 
    > edit routing control-plane-protocols control-plane-protocol msdp ''"
    > 
    > I am pretty sure that we would get a bug report on this, asking what is the
    > purpose to have: name ''", and requesting a suppression on the term, but we
    > do not have a good way to achieve.
    > 
    > As a comparison, the option #3 will give:
    > 
    > edit routing control-plane-protocols msdp
    > 
    > This is the only acceptable solution so far. When a model is not usable by
    > the end-user, other considerations (such as augmentation convenience) will
    > not matter.
    > 
    > Thanks,
    > - Xufeng
    > 
    > > -----Original Message-----
    > > From: Acee Lindem (acee) [mailto:acee@cisco.com]
    > > Sent: Monday, February 19, 2018 1:35 PM
    > > To: Christian Hopps <chopps@chopps.org>; Martin Bjorklund <mbj@tail-f.com>
    > > Cc: Xufeng Liu <Xufeng_Liu@jabil.com>; zhang.zheng@zte.com.cn; yang-
    > > doctors@ietf.org
    > > Subject: Re: [yang-doctors] How to restrict to have single control-plane-
    > > protocol instance
    > >
    > >
    > >
    > > On 2/19/18, 5:02 AM, "Christian Hopps" <chopps@chopps.org> wrote:
    > >
    > >
    > >     Martin Bjorklund <mbj@tail-f.com> writes:
    > >
    > >     > Hi,
    > >     >
    > >     > "Acee Lindem (acee)" <acee@cisco.com> wrote:
    > >     >> All,
    > >     >>
    > >     >> As seems to be the modus operandi for YANG issues, we have 3
    > separate
    > > opinions as to how a protocol only supporting a single instance should be
    > > realized.
    > >     >>
    > >     >>   1. Augment the existing control plane protocols list (RFC
    > 8022BIS)
    > >     >>   and specify in the description text that only a single instance
    > is
    > >     >>   supported.
    > >     >>   2. Augment the existing control plane protocols list (RFC
    > 8022BIS)
    > >     >>   and use a YANG 1.1 must() restriction as discussed by Martin and
    > >     >>   Lada.
    > >     >>   3. Augment the container one level up from the list for singleton
    > >     >>   protocols (suggested by Xufeng).
    > >
    > >     > But I think there was also a proposal to require the single instance
    > >     > to have a well-known name - but maybe this proposal is no longer on
    > >     > the table.
    > >
    > >     I actually liked this solution; however, instead of picking an
    > arbitrary "well-
    > > known" value for name, I would specify the 0 length string instead. I
    > think that
    > > reinforces the idea that this isn't actually a named instance. :)
    > >
    > >        augment "/rt:routing/rt:control-plane-protocols/"
    > >              + "rt:control-plane-protocol" {
    > >           when "derived-from-or-self(rt:type, 'msdp:msdp') and rt:name =
    > ''"  {
    > >           container msdp {
    > >
    > > One benefit of this solution is that it solves Xufeng's issue of what the
    > client uses
    > > as the instance name.
    > >
    > >
    > >     Thanks,
    > >     Chris.
    > >
    > >     >
    > >     >
    > >     > /martin
    > >     >
    > >     >
    > >     >> and #3. For #3, one determent would be that the control plane
    > protocols
    > > are in a location other than where they were originally envisioned and I
    > don't
    > > relish pulling RFC8022BIS off the RFC queue to document.
    > >     >>
    > >     >> Thanks,
    > >     >> Acee
    > >     >>
    > >     >> On 2/15/18, 8:39 AM, "Reshad Rahman (rrahman)" <rrahman@cisco.com>
    > > wrote:
    > >     >>
    > >     >>     Hi Xufeng,
    > >     >>
    > >     >>     I think the intent of 8022bis was to have all protocols under
    > > /rt:routing/rt:control-plane-protocols/rt:control-plane-protocol. I agree
    > that
    > > forcing a name for a single-instance is cumbersome, but I think it is too
    > late to
    > > change tree hierachy organization at this point.
    > >     >>
    > >     >>     I will defer to other YDs and 8022bis authors on this.
    > >     >>
    > >     >>     Regards,
    > >     >>     Reshad.
    > >     >>
    > >     >>     On 2018-02-08, 9:48 AM, "Xufeng Liu" <Xufeng_Liu@jabil.com>
    > wrote:
    > >     >>
    > >     >>         Hi All,
    > >     >>
    > >     >>         I feel that such a solution is still not clean enough to
    > outweigh the
    > > simple augmentation to "/rt:routing/rt:control-plane-protocols/".
    > >     >>
    > >     >>         Some considerations are:
    > >     >>
    > >     >>         - Name management: Neither the operator nor the
    > implementation
    > > wants to deal with the artificial name, whether it is hardcoded,
    > user-configured,
    > > or system-generated. When we implement such singleton protocol, we don't
    > > save a name anywhere.
    > >     >>         - The complexity of validation: The "when" statement is an
    > > unnecessary expense to the user and to the implementation, especially if
    > we
    > > need to check all instances.
    > >     >>         - Data tree query: If the singleton "MSDP" is mixed with
    > other protocol
    > > instances, it is less obvious or harder to search for. Depending on the
    > > implementation, it would be worse if the entire list needs to be iterated..
    > >     >>         - Tree hierarchy  organization: I don't see too big a
    > problem with "all
    > > single-instance protocols under /rt:routing/rt:control-plane-protocols
    > and all
    > > the multi-instance ones under
    > /rt:routing/rt:control-plane-protocols/rt:control-
    > > plane-protocol". If necessary, some of the names can be adjusted.
    > >     >>
    > >     >>         Thanks,
    > >     >>         - Xufeng
    > >     >>
    > >     >>
    > >     >>         > -----Original Message-----
    > >     >>         > From: Reshad Rahman (rrahman) [mailto:rrahman@cisco.com]
    > >     >>         > Sent: Thursday, February 8, 2018 9:41 AM
    > >     >>         > To: Ladislav Lhotka <lhotka@nic.cz>; Martin Bjorklund
    > <mbj@tail-
    > > f.com>;
    > >     >>         > Acee Lindem (acee) <acee@cisco.com>
    > >     >>         > Cc: yang-doctors@ietf.org; zhang.zheng@zte.com.cn;
    > Xufeng Liu
    > >     >>         > <Xufeng_Liu@jabil.com>
    > >     >>         > Subject: Re: [yang-doctors] How to restrict to have
    > single control-
    > > plane-
    > >     >>         > protocol instance
    > >     >>         >
    > >     >>         > Thanks for the suggestions. I agree that hard-coding the
    > name is a
    > > bad idea,
    > >     >>         > glad that a cleaner way of doing this is possible.
    > >     >>         > - We can move the must statement up to restrict max of 1
    > control-
    > > plane-
    > >     >>         > protocol instance of type msdp?
    > >     >>         > - Acee/Lada, should a note be added to section 5.3 of
    > 8022bis
    > > regarding how
    > >     >>         > to enforce single instance? How much of a concern is the
    > > performance
    > >     >>         > impact in this specific case?
    > >     >>         >
    > >     >>         > Regards,
    > >     >>         > Reshad.
    > >     >>         >
    > >     >>         > On 2018-02-08, 7:02 AM, "Ladislav Lhotka" <lhotka@nic.cz>
    > wrote:
    > >     >>         >
    > >     >>         >     On Thu, 2018-02-08 at 12:39 +0100, Martin Bjorklund
    > wrote:
    > >     >>         >     > "Acee Lindem (acee)" <acee@cisco.com> wrote:
    > >     >>         >     > > Hi Lada,
    > >     >>         >     >
    > >     >>         >     > >
    > >     >>         >     >
    > >     >>         >     > > On 2/8/18, 4:42 AM, "yang-doctors on behalf of
    > Ladislav
    > > Lhotka"
    > >     >>         > <yang-docto
    > >     >>         >     > rs-bounces@ietf.org on behalf of lhotka@nic.cz>
    > wrote:
    > >     >>         >     >
    > >     >>         >     > >
    > >     >>         >     >
    > >     >>         >     > >     On Thu, 2018-02-08 at 09:20 +0100, Martin
    > Bjorklund wrote:
    > >     >>         >     >
    > >     >>         >     > >     > Hi,
    > >     >>         >     >
    > >     >>         >     > >     >
    > >     >>         >     >
    > >     >>         >     > >     > "Reshad Rahman (rrahman)" <
    > rrahman@cisco.com> wrote:
    > >     >>         >     >
    > >     >>         >     > >     > > Hi YDs,
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > > MSDP YANG authors want to enforce
    > single-instance of
    > > MSDP
    > >     >>         >     >
    > >     >>         >     > >     > > control-plane protocol. The when
    > “rt:type =
    > > ‘msdp’“ allows
    > >     >>         > multiple
    > >     >>         >     >
    > >     >>         >     > >     > > control-pane-protocol instances as long
    > as they have
    > > different
    > >     >>         >     >
    > >     >>         >     > >     > > rt:name. The only workaround I thought
    > of is to have a
    > > when
    > >     >>         >     > statement
    > >     >>         >     >
    > >     >>         >     > >     > > on the name in the top level container.
    > This would still
    > > multiple
    > >     >>         >     >
    > >     >>         >     > >     > > control-plane-protocol instance of type
    > msdp but
    > > restricts the
    > >     >>         > name
    > >     >>         >     > to
    > >     >>         >     >
    > >     >>         >     > >     > > a fixed name (msdp-protocol in this
    > case) for the top level
    > > msdp
    > >     >>         >     >
    > >     >>         >     > >     > > container to exist. Any suggestions on
    > how to do this
    > > better?
    > >     >>         >     >
    > >     >>         >     > >     >
    > >     >>         >     >
    > >     >>         >     > >     > Hard-coding a name like this is IMO a bad
    > idea.
    > >     >>         >     >
    > >     >>         >     > >     >
    > >     >>         >     >
    > >     >>         >     > >     > Better would be to simply state in text
    > that there MUST
    > > only be one
    > >     >>         >     >
    > >     >>         >     > >     > instance of this type.
    > >     >>         >     >
    > >     >>         >     > >     >
    > >     >>         >     >
    > >     >>         >     > >     > But you can also add a must statement that
    > enforces this:
    > >     >>         >     >
    > >     >>         >     > >     >
    > >     >>         >     >
    > >     >>         >     > >     >    augment
    > "/rt:routing/rt:control-plane-protocols/"
    > >     >>         >     >
    > >     >>         >     > >     >          + "rt:control-plane-protocol" {
    > >     >>         >     >
    > >     >>         >     > >     >       when 'derived-from-or-self(rt:type,
    > "msdp:msdp"'  {
    > >     >>         >     >
    > >     >>         >     > >     >      container msdp {
    > >     >>         >     >
    > >     >>         >     > >     >        must
    > 'count(/rt:routing/rt:control-plane-protocols/'
    > >     >>         >     >
    > >     >>         >     > >     >           + '
    > rt:control-plane-protocol['
    > >     >>         >     >
    > >     >>         >     > >     >           + '
    > derived-from-or-sel(../rt:type, "msdp:msdp")])
    > > <=
    > >     >>         >     > 1'";
    > >     >>         >     >
    > >     >>         >     > >     >
    > >     >>         >     >
    > >     >>         >     > >     >
    > >     >>         >     >
    > >     >>         >     > >     > In general, you should be careful with the
    > usage of "count",
    > > since it
    > >     >>         >     >
    > >     >>         >     > >     > will loop through *all* instances in the
    > list every time.  If
    > > the list
    > >     >>         >     >
    > >     >>         >     > >     > is big, this can have a performance impact..
    > >     >>         >     >
    > >     >>         >     > >
    > >     >>         >     >
    > >     >>         >     > >     Instead of count(), it is possible to use
    > the so-called
    > > Muenchian
    > >     >>         >     > method:
    > >     >>         >     >
    > >     >>         >     > >
    > >     >>         >     >
    > >     >>         >     > >         container msdp {
    > >     >>         >     >
    > >     >>         >     > >           must
    > "not(../preceding-sibling::rt:control-plane-
    > > protocol["
    > >     >>         >     >
    > >     >>         >     > >              + "derived-from-or-self(rt:type,
    > 'msdp:msdp')])";
    > >     >>         >     >
    > >     >>         >     > >           ..
    > >     >>         >     >
    > >     >>         >     > >         }
    > >     >>         >     >
    > >     >>         >     > >
    > >     >>         >     >
    > >     >>         >     > >     It basically states that the
    > control-plane-protocol containing
    > > the
    > >     >>         >     > "msdp"
    > >     >>         >     >
    > >     >>         >     > >     container must not be preceded with a
    > control-plane-
    > > protocol entry
    > >     >>         > of
    > >     >>         >     > the
    > >     >>         >     >
    > >     >>         >     > >     msdp:msdp type (or derived).
    > >     >>         >     >
    > >     >>         >     > >
    > >     >>         >     >
    > >     >>         >     > > This looks like an elegant solution.
    > >     >>         >     >
    > >     >>         >     >
    > >     >>         >     > "elegant" as in "less obvious" ;)  It has the same
    > time complexity
    > > as
    > >     >>         >     > the count() solution.
    > >     >>         >
    > >     >>         >     It should be faster on the average - it has to scan
    > only preceding
    > > siblings of
    > >     >>         >     the MSDP protocol instance whereas count() always
    > has to check
    > > *all*
    > >     >>         > protocol
    > >     >>         >     instances.
    > >     >>         >
    > >     >>         >     It is true though that in XSLT this technique can be
    > made
    > > considerably
    > >     >>         > more
    > >     >>         >     efficient by using indexed keys.
    > >     >>         >
    > >     >>         >     Lada
    > >     >>         >
    > >     >>         >     >
    > >     >>         >     >
    > >     >>         >     > However, since the key for the
    > control-plane-protocol  list is
    > > "type
    > >     >>         >     > name", won't it only work if the previous sibling
    > has a  "name"
    > > that
    > >     >>         >     > is precedes the one being added?
    > >     >>         >     >
    > >     >>         >     > For each list entry that has this container, the
    > expression is
    > >     >>         >     > evaluated.  It will scan all preceding entries and
    > ensure that there
    > >     >>         >     > are none with this type.  So the order of the
    > entries doesn't
    > > matter;
    > >     >>         >     > if there are two with the same type, one of them
    > has to be
    > > before the
    > >     >>         >     > other.
    > >     >>         >     >
    > >     >>         >     >
    > >     >>         >     > /martin
    > >     >>         >     >
    > >     >>         >     >
    > >     >>         >     > >
    > >     >>         >     >
    > >     >>         >     > > Thanks,
    > >     >>         >     >
    > >     >>         >     > > Acee
    > >     >>         >     >
    > >     >>         >     > >
    > >     >>         >     >
    > >     >>         >     > >
    > >     >>         >     >
    > >     >>         >     > >     Lada
    > >     >>         >     >
    > >     >>         >     > >
    > >     >>         >     >
    > >     >>         >     > >     >
    > >     >>         >     >
    > >     >>         >     > >     > Also note that I use derived-from-or-self
    > instead of equality
    > > for the
    > >     >>         >     >
    > >     >>         >     > >     > identity.
    > >     >>         >     >
    > >     >>         >     > >     >
    > >     >>         >     >
    > >     >>         >     > >     >
    > >     >>         >     >
    > >     >>         >     > >     > /martin
    > >     >>         >     >
    > >     >>         >     > >     >
    > >     >>         >     >
    > >     >>         >     > >     >
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > > Regards,
    > >     >>         >     >
    > >     >>         >     > >     > > Reshad.
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > >   augment
    > "/rt:routing/rt:control-plane-protocols/"
    > >     >>         >     >
    > >     >>         >     > >     > >         + "rt:control-plane-protocol" {
    > >     >>         >     >
    > >     >>         >     > >     > >      when "rt:type = ‘msdp’"  {
    > >     >>         >     >
    > >     >>         >     > >     > >       description
    > >     >>         >     >
    > >     >>         >     > >     > >         "….”;
    > >     >>         >     >
    > >     >>         >     > >     > >     }
    > >     >>         >     >
    > >     >>         >     > >     > >     description "….";
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > >     container msdp {
    > >     >>         >     >
    > >     >>         >     > >     > >       when "../rt:name =
    > ‘msdp-protocol’"  {
    > >     >>         >     >
    > >     >>         >     > >     > >         description
    > >     >>         >     >
    > >     >>         >     > >     > >           "….";
    > >     >>         >     >
    > >     >>         >     > >     > >       }
    > >     >>         >     >
    > >     >>         >     > >     > >       description "MSDP top level
    > container.";
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > > From: "Reshad Rahman (rrahman)" <
    > rrahman@cisco.com>
    > >     >>         >     >
    > >     >>         >     > >     > > Date: Monday, February 5, 2018 at 6:25 PM
    > >     >>         >     >
    > >     >>         >     > >     > > To: Xufeng Liu <Xufeng_Liu@jabil.com>,
    > >     >>         > "zhang.zheng@zte.com.cn"
    > >     >>         >     >
    > >     >>         >     > >     > > <zhang.zheng@zte.com.cn>
    > >     >>         >     >
    > >     >>         >     > >     > > Cc: "anish.ietf@gmail.com" <
    > anish.ietf@gmail.com>,
    > > "Mahesh
    > >     >>         > Sivakumar
    > >     >>         >     >
    > >     >>         >     > >     > > (masivaku)" <masivaku@cisco.com>,
    > > "guofeng@huawei.com"
    > >     >>         >     >
    > >     >>         >     > >     > > <guofeng@huawei.com>,
    > > "pete.mcallister@metaswitch.com"
    > >     >>         >     >
    > >     >>         >     > >     > > <pete.mcallister@metaswitch.com>,
    > > "liuyisong@huawei.com"
    > >     >>         >     >
    > >     >>         >     > >     > > <liuyisong@huawei.com>, "
    > xu.benchong@zte.com.cn"
    > >     >>         >     >
    > >     >>         >     > >     > > <xu.benchong@zte.com.cn>,
    > "tanmoy.kundu@alcatel-
    > >     >>         > lucent.com"
    > >     >>         >     >
    > >     >>         >     > >     > > <tanmoy.kundu@alcatel-lucent.com>,
    > >     >>         > "zzhang_ietf@hotmail.com"
    > >     >>         >     >
    > >     >>         >     > >     > > <zzhang_ietf@hotmail.com>, "Acee Lindem
    > (acee)"
    > >     >>         > <acee@cisco.com>
    > >     >>         >     >
    > >     >>         >     > >     > > Subject: Re: Hi all, about the
    > modification of MSDP YANG
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > > Hi Sandy and Xufeng,
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > > I understand that you want only 1 MSDP
    > instance but I
    > > don’t think
    > >     >>         >     > that
    > >     >>         >     >
    > >     >>         >     > >     > > justifies
    > /rt:routing/rt:control-plane-protocols. If we do
    > > that we
    > >     >>         >     >
    > >     >>         >     > >     > > will end up with all single-instance
    > protocols under
    > >     >>         >     >
    > >     >>         >     > >     > > /rt:routing/rt:control-plane-protocols
    > and all the multi-
    > > instance
    > >     >>         >     > ones
    > >     >>         >     >
    > >     >>         >     > >     > > under
    > >     >>         >     >
    > >     >>         >     > >     > >
    > /rt:routing/rt:control-plane-protocols/rt:control-plane-
    > > protocol.
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > > I am not sure what’s the best way to
    > enforce single-
    > > instance, I can
    > >     >>         >     >
    > >     >>         >     > >     > > check with the other YDs on this topic.
    > One way it can be
    > > done is
    > >     >>         > as
    > >     >>         >     >
    > >     >>         >     > >     > > follows (I’ve added the when statement
    > in bold to
    > > existing BFD
    > >     >>         >     > model),
    > >     >>         >     >
    > >     >>         >     > >     > > it enforces that the protocol name is
    > ‘bfdv1’. So multiple
    > >     >>         > instances
    > >     >>         >     >
    > >     >>         >     > >     > > with rt:type=bfd-types:bfdv1 could be
    > created, but only
    > > one of
    > >     >>         > these
    > >     >>         >     >
    > >     >>         >     > >     > > instances can have the bfd container.
    > This is probably not
    > > the
    > >     >>         > best
    > >     >>         >     >
    > >     >>         >     > >     > > way but the point is that IMO protocols
    > have to go under
    > >     >>         >     >
    > >     >>         >     > >     > >
    > /rt:routing/rt:control-plane-protocols/rt:control-plane-
    > > protocol.
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > > Regards,
    > >     >>         >     >
    > >     >>         >     > >     > > Reshad.
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > >   augment
    > "/rt:routing/rt:control-plane-protocols/"
    > >     >>         >     >
    > >     >>         >     > >     > >         + "rt:control-plane-protocol" {
    > >     >>         >     >
    > >     >>         >     > >     > >      when "rt:type = 'bfd-types:bfdv1'"
    > {
    > >     >>         >     >
    > >     >>         >     > >     > >       description
    > >     >>         >     >
    > >     >>         >     > >     > >         "This augmentation is only valid
    > for a control-plane
    > >     >>         >     > protocol
    > >     >>         >     >
    > >     >>         >     > >     > >          instance of BFD (type
    > 'bfdv1').";
    > >     >>         >     >
    > >     >>         >     > >     > >     }
    > >     >>         >     >
    > >     >>         >     > >     > >     description "BFD augmentation.";
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > >     container bfd {
    > >     >>         >     >
    > >     >>         >     > >     > >       when "../rt:name = 'bfdv1'"  {
    > >     >>         >     >
    > >     >>         >     > >     > >         description
    > >     >>         >     >
    > >     >>         >     > >     > >           "This augmentation is only
    > valid for a control-plane
    > >     >>         >     > protocol
    > >     >>         >     >
    > >     >>         >     > >     > >            instance of BFD (type
    > 'bfdv1').";
    > >     >>         >     >
    > >     >>         >     > >     > >       }
    > >     >>         >     >
    > >     >>         >     > >     > >       description "BFD top level
    > container.";
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > > From: Xufeng Liu <Xufeng_Liu@jabil.com>
    > >     >>         >     >
    > >     >>         >     > >     > > Date: Monday, February 5, 2018 at 9:38 AM
    > >     >>         >     >
    > >     >>         >     > >     > > To: "zhang.zheng@zte.com.cn"
    > > <zhang.zheng@zte.com.cn>
    > >     >>         >     >
    > >     >>         >     > >     > > Cc: "Reshad Rahman (rrahman)" <
    > rrahman@cisco.com>,
    > >     >>         >     >
    > >     >>         >     > >     > > "anish.ietf@gmail.com" <
    > anish.ietf@gmail.com>,
    > > "Mahesh
    > >     >>         > Sivakumar
    > >     >>         >     >
    > >     >>         >     > >     > > (masivaku)" <masivaku@cisco.com>,
    > > "guofeng@huawei.com"
    > >     >>         >     >
    > >     >>         >     > >     > > <guofeng@huawei.com>,
    > > "pete.mcallister@metaswitch.com"
    > >     >>         >     >
    > >     >>         >     > >     > > <pete.mcallister@metaswitch.com>,
    > > "liuyisong@huawei.com"
    > >     >>         >     >
    > >     >>         >     > >     > > <liuyisong@huawei.com>, "
    > xu.benchong@zte.com.cn"
    > >     >>         >     >
    > >     >>         >     > >     > > <xu.benchong@zte.com.cn>,
    > "tanmoy.kundu@alcatel-
    > >     >>         > lucent.com"
    > >     >>         >     >
    > >     >>         >     > >     > > <tanmoy.kundu@alcatel-lucent.com>,
    > >     >>         > "zzhang_ietf@hotmail.com"
    > >     >>         >     >
    > >     >>         >     > >     > > <zzhang_ietf@hotmail.com>
    > >     >>         >     >
    > >     >>         >     > >     > > Subject: RE: Hi all, about the
    > modification of MSDP YANG
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > > Hi Sandy,
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > > Thanks for the updates.
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > > In RFC8022bis, the rt:type is defined
    > under
    > >     >>         >     >
    > >     >>         >     > >     > >
    > /rt:routing/rt:control-plane-protocols/rt:control-plane-
    > > protocol.
    > >     >>         > If
    > >     >>         >     >
    > >     >>         >     > >     > > we augment
    > /rt:routing/rt:control-plane-protocols, the
    > > “when”
    > >     >>         >     >
    > >     >>         >     > >     > > statement will not be valid, because it
    > cannot find the
    > > rt:type. I
    > >     >>         >     >
    > >     >>         >     > >     > > don’t think that we need the “when”
    > statement. The
    > > container
    > >     >>         > with
    > >     >>         >     >
    > >     >>         >     > >     > > “presence” will serve the purpose of the
    > identity. We can
    > > simply
    > >     >>         >     > take
    > >     >>         >     >
    > >     >>         >     > >     > > out the “when” statement and the
    > definition of the MSDP
    > > identity.
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > > Thanks,
    > >     >>         >     >
    > >     >>         >     > >     > > - Xufeng
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > > From: zhang.zheng@zte.com.cn
    > > [mailto:zhang.zheng@zte.com.cn]
    > >     >>         >     >
    > >     >>         >     > >     > > Sent: Monday, February 5, 2018 3:36 AM
    > >     >>         >     >
    > >     >>         >     > >     > > To: Xufeng Liu <Xufeng_Liu@jabil.com>
    > >     >>         >     >
    > >     >>         >     > >     > > Cc: rrahman@cisco.com;
    > anish.ietf@gmail.com;
    > >     >>         > masivaku@cisco.com;
    > >     >>         >     >
    > >     >>         >     > >     > > guofeng@huawei.com;
    > > pete.mcallister@metaswitch.com;
    > >     >>         >     >
    > >     >>         >     > >     > > liuyisong@huawei.com;
    > xu.benchong@zte.com.cn;
    > >     >>         >     >
    > >     >>         >     > >     > > tanmoy.kundu@alcatel-lucent.com;
    > > zzhang_ietf@hotmail.com
    > >     >>         >     >
    > >     >>         >     > >     > > Subject: RE: Hi all, about the
    > modification of MSDP YANG
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > > Hi Xufeng and Reshad,
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > > I am sorry for forgetting the point. I
    > updated the YANG
    > > model.
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > > If no one has comments on it I'd like to
    > submit the new
    > > version. :-)
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > > Thanks,
    > >     >>         >     >
    > >     >>         >     > >     > >
    > >     >>         >     >
    > >     >>         >     > >     > > Sandy
    > >     >>         >     >
    > >     >>         >     > >     > > 原始邮件
    > >     >>         >     >
    > >     >>         >     > >     > > 发件人:
    > >     >>         > <Xufeng_Liu@jabil.com<mailto:Xufeng_Liu@jabil.com>>;
    > >     >>
    > ================================
    > 
    > On Mon, Aug 13, 2018 at 10:34 AM Jan Lindblad <janl@tail-f.com> wrote:
    > 
    > > 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.
    > >
    > >
    > >
    _______________________________________________
    yang-doctors mailing list
    yang-doctors@ietf.org
    https://www.ietf.org/mailman/listinfo/yang-doctors