Re: Yangdoctors last call review of draft-ietf-pim-igmp-mld-yang-07
Xufeng Liu <xufeng.liu.ietf@gmail.com> Sat, 08 September 2018 17:47 UTC
Return-Path: <xufeng.liu.ietf@gmail.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 07438130E14; Sat, 8 Sep 2018 10:47:25 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.998
X-Spam-Level:
X-Spam-Status: No, score=-1.998 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.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 R7RbcfPHW0gj; Sat, 8 Sep 2018 10:47:20 -0700 (PDT)
Received: from mail-io1-xd33.google.com (mail-io1-xd33.google.com [IPv6:2607:f8b0:4864:20::d33]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id F107512F1A2; Sat, 8 Sep 2018 10:47:16 -0700 (PDT)
Received: by mail-io1-xd33.google.com with SMTP id c22-v6so3920307iob.1; Sat, 08 Sep 2018 10:47:16 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=iGSOePv9NBZ14HcXkT9jCTT9w5/pBx5nXrgkneJROhM=; b=jINKjhNak8wNu/FX5Yc8SyADuWFUSOV8G8lrwcClA8ZPxiH+I1FvAMHM2tyPsBJKtM 2Y13GtxRM9GBBzgBh7ouR1z0oFltiN+2fTq+rstZcLTLtm4mlg0Hq3D2uz+gfGdOYcQr NWC/k+vXHoxgRWrD8gXc5FZVHFpbYEhX9JQTM+tAapXHJkDYcBCI+9KhP0rAoIAGh6C7 L4b4JyrnZHUcvw7hA0bIQ8R7tVEkg33NLqFfwfanCNAh8ZDcVZDx2/MFnoUkF0iKWkp0 xKLQNOwAksz/4wotMOSCC0z6Dmx/nie9EwtjWS4Je9Z0xqIilVgpxPxXXfcrMnolsirc UGpA==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=iGSOePv9NBZ14HcXkT9jCTT9w5/pBx5nXrgkneJROhM=; b=CO4VGdQ5aq9iacXWwN/drXnOBEEUiU5rl20C8SEeEXe7edCegLCI8U4KAU0nNgYtbf i7XF3BuIui1m9CziQAPiZD79wFy6k1QR5XA8vNJcS/PAwjFVOVPzFXNLnkzlm2NZfBwq 3TSqHPvs6d7aJGRwhBTBI1DJUZGaaAlUCPh0vyKX3jppQJUqHJN3CZkB1ZKEtQjlfic5 gKzKecMtSfEN97OX9tey3ZjydEhmtR/uC1ZZlW6Qe2mUfwK7uEUnV9YiSKo/kN5NwAvG 4yJnn4FFphj6HMfZD7nNggfY1HEsGStlg9ZTuE6jF/JLtWu4nSzq+vUu+xX8g84Fsl2N kNQQ==
X-Gm-Message-State: APzg51BxNHw1pK9Gt0jGQxTALqKkguI4oTJ8sub+WraqneeH8R1bu6IF D/AtfFz9sMq6+qGhQNgU8MrkGD1T8cGgVenl09pO4a7c
X-Google-Smtp-Source: ANB0VdaVbemxbZhGTHWzl3prirWeA9Hn29LVdXrnnS/aQMcZBEIsF3bnqE+1PGQ37TmZ+UZBLG4WI56hfkSmrwm8viw=
X-Received: by 2002:a6b:e15:: with SMTP id 21-v6mr10682133ioo.149.1536428835923; Sat, 08 Sep 2018 10:47:15 -0700 (PDT)
MIME-Version: 1.0
References: <153417087203.25020.10120277992606371332@ietfa.amsl.com>
In-Reply-To: <153417087203.25020.10120277992606371332@ietfa.amsl.com>
From: Xufeng Liu <xufeng.liu.ietf@gmail.com>
Date: Sat, 08 Sep 2018 13:47:04 -0400
Message-ID: <CAEz6PPT6UFPUdFVm0ACM3vLh9vXt1dtJjtgrbEZ=hiDPmy=Q3g@mail.gmail.com>
Subject: Re: Yangdoctors last call review of draft-ietf-pim-igmp-mld-yang-07
To: janl@tail-f.com
Cc: yang-doctors@ietf.org, ietf <ietf@ietf.org>, pim@ietf.org, draft-ietf-pim-igmp-mld-yang.all@ietf.org
Content-Type: multipart/alternative; boundary="000000000000fcec6205755fb6f0"
Archived-At: <https://mailarchive.ietf.org/arch/msg/ietf/rWYjzVor7LoY2q6krhwUn0gQf8k>
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: Sat, 08 Sep 2018 17:47:25 -0000
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. 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