Re: [yang-doctors] Yangdoctors last call review of draft-ietf-pim-igmp-mld-yang-07
Martin Bjorklund <mbj@tail-f.com> Tue, 11 September 2018 14:11 UTC
Return-Path: <mbj@tail-f.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 C3D62130DF5; Tue, 11 Sep 2018 07:11:41 -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 O8E9wOnrN4X3; Tue, 11 Sep 2018 07:11:38 -0700 (PDT)
Received: from mail.tail-f.com (mail.tail-f.com [46.21.102.45]) by ietfa.amsl.com (Postfix) with ESMTP id CD8B9130DDB; Tue, 11 Sep 2018 07:11:37 -0700 (PDT)
Received: from localhost (h-80-27.A165.priv.bahnhof.se [212.85.80.27]) by mail.tail-f.com (Postfix) with ESMTPSA id F2AAE1AE03F5; Tue, 11 Sep 2018 16:11:35 +0200 (CEST)
Date: Tue, 11 Sep 2018 16:11:35 +0200
Message-Id: <20180911.161135.1550814850585221588.mbj@tail-f.com>
To: xufeng.liu.ietf@gmail.com
Cc: janl@tail-f.com, yang-doctors@ietf.org, ietf@ietf.org, pim@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
From: Martin Bjorklund <mbj@tail-f.com>
In-Reply-To: <CAEz6PPT6UFPUdFVm0ACM3vLh9vXt1dtJjtgrbEZ=hiDPmy=Q3g@mail.gmail.com>
References: <153417087203.25020.10120277992606371332@ietfa.amsl.com> <CAEz6PPT6UFPUdFVm0ACM3vLh9vXt1dtJjtgrbEZ=hiDPmy=Q3g@mail.gmail.com>
X-Mailer: Mew version 6.7 on Emacs 24.5 / Mule 6.0 (HANACHIRUSATO)
Mime-Version: 1.0
Content-Type: Text/Plain; charset="utf-8"
Content-Transfer-Encoding: base64
Archived-At: <https://mailarchive.ietf.org/arch/msg/ietf/G9wFPeN2Tb2oHAyATnbQpg9-aM0>
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 14:11:42 -0000
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. > > > > > >
- Yangdoctors last call review of draft-ietf-pim-ig… Jan Lindblad
- RE: Yangdoctors last call review of draft-ietf-pi… Guofeng
- Re: Yangdoctors last call review of draft-ietf-pi… Xufeng Liu
- Re: Yangdoctors last call review of draft-ietf-pi… Jan Lindblad
- Re: [yang-doctors] Yangdoctors last call review o… Martin Bjorklund
- Re: [yang-doctors] Yangdoctors last call review o… Reshad Rahman (rrahman)
- RE: [pim] Yangdoctors last call review of draft-i… Guofeng