Re: [pim] Yangdoctors telechat review of draft-ietf-pim-igmp-mld-yang-10

Xufeng Liu <xufeng.liu.ietf@gmail.com> Wed, 13 February 2019 22:19 UTC

Return-Path: <xufeng.liu.ietf@gmail.com>
X-Original-To: pim@ietfa.amsl.com
Delivered-To: pim@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 3A0C3128CF2; Wed, 13 Feb 2019 14:19:07 -0800 (PST)
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, RCVD_IN_DNSWL_NONE=-0.0001, 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 5j7NCnIQHrQX; Wed, 13 Feb 2019 14:19:04 -0800 (PST)
Received: from mail-it1-x12b.google.com (mail-it1-x12b.google.com [IPv6:2607:f8b0:4864:20::12b]) (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 2FCF212785F; Wed, 13 Feb 2019 14:19:04 -0800 (PST)
Received: by mail-it1-x12b.google.com with SMTP id z9so8060769itc.4; Wed, 13 Feb 2019 14:19:04 -0800 (PST)
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=l7vWNUlmDHlxK6qwDt39efZ/ISoQW76ZcTkQZp1Uuwk=; b=qrqy5SR52vIxnQwOcrEBxhZPu9oNRda0l6FvJuuJ5Ivdgq36Qts03FZpGWVl9EP8Fe FSvePE/DaxEaROKS+6QJaU92qyi7gkHxizIUDXddmGFUUQAelwt1zLKUXcdqqorr+L4I Vj6+bKo0p6d2ueUN2xmpozgZAoWH2FWysvaUqCIS7uthqwiMdQvY8H0IYDXEPGp7R1cm nuMNO4MA9PTH0D9Zvd9Sx/+yYQBp2kBEiedTAk9WminPG5feq+vCphcmz4Nf6s5b+779 rTlBWR0ajmLr0s26tHeMjaCSzOmQFOL0ZLrqPE/rIsWaII97K8TzYk/3WglyDnQv2uT7 Givg==
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=l7vWNUlmDHlxK6qwDt39efZ/ISoQW76ZcTkQZp1Uuwk=; b=MVJlz18kqQFThuU49zoxMDqlZhO7aDmnUMT7QwIoiD37ICMYee1mRKp/4+bvI7en+C jEbWJwVH8/9L4sNA+b3XZLYxyR2JnPX/0JE8wmKLVYd8zMButp70+2VtrvDvT1zSe6tl NWSqpKR5OCo2ZQZIe+mpoGCPbzSMBmtAAwyrhlOnjuUz/As9Cao8kTkW7z5SW3uCtCMk F0BuvAUyhjtTDop5Jg8mrbzN3FPmmrvd8vYv9I3bS0aliZTEJOqaxg2EPnCsS7tkIprn a6X+xopNYfed54HbmruzqKA7NVs/tsFl4B+pz98rE3HxpbWi7obY4F590HZ7DQ4L4abG 1GuQ==
X-Gm-Message-State: AHQUAubLE8jnaC+vD0VeQb8Wjl0vNa0t8wmo0KSgrYrlDLGdlm2cnv4J mqAa53RhQuih0pTttQCZCvu0sbowhuXBt68h5+XniX1e
X-Google-Smtp-Source: AHgI3IaFoM3DolCKzqyIww7gA7fJfNcPvflAbcBapyoj7HfmR06OPlO408wWOCK+EaEmvyZ2BT6NrEMYDeeRv8sPJn4=
X-Received: by 2002:a5e:a611:: with SMTP id q17mr261998ioi.17.1550096343183; Wed, 13 Feb 2019 14:19:03 -0800 (PST)
MIME-Version: 1.0
References: <154955530928.5208.12848706122029839234@ietfa.amsl.com> <00d401d4bfc8$273c1380$4001a8c0@gateway.2wire.net> <01887BB2-429A-4230-A278-7577546F93D8@tail-f.com> <050101d4c2cd$2832f760$4001a8c0@gateway.2wire.net>
In-Reply-To: <050101d4c2cd$2832f760$4001a8c0@gateway.2wire.net>
From: Xufeng Liu <xufeng.liu.ietf@gmail.com>
Date: Wed, 13 Feb 2019 17:18:50 -0500
Message-ID: <CAEz6PPTHE7ruwtRY7Zc5ce6DjxQbkAZfQjcgzz+zyZ6Y-KkrKA@mail.gmail.com>
To: tom petch <daedulus@btconnect.com>
Cc: Jan Lindblad <janl@tail-f.com>, "yang-doctors@ietf.org" <yang-doctors@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>
Content-Type: multipart/alternative; boundary="000000000000e736480581cded0f"
Archived-At: <https://mailarchive.ietf.org/arch/msg/pim/NfZwQQh5T2hXQK-Q4DHwy_M3XE4>
Subject: Re: [pim] Yangdoctors telechat review of draft-ietf-pim-igmp-mld-yang-10
X-BeenThere: pim@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Protocol Independent Multicast <pim.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/pim>, <mailto:pim-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/pim/>
List-Post: <mailto:pim@ietf.org>
List-Help: <mailto:pim-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/pim>, <mailto:pim-request@ietf.org?subject=subscribe>
X-List-Received-Date: Wed, 13 Feb 2019 22:19:07 -0000

Hi Tom,

Thanks for pointing out these additional quirks. We will fix them in the
next revision.
Best,
- Xufeng

On Tue, Feb 12, 2019 at 7:20 AM tom petch <daedulus@btconnect.com>; wrote:

> Jan
>
> Thanks for that.  I see grouping as a necessary evil to be used with
> restraint, like subroutines and procedures in code, a restraint that I
> would like to be exercised more:-(
>
> So, I now see how the groupings are used under an augment/when which
> makes them conditional on either IGMP or MLD and so not ambiguous as I
> first thought.
>
> Having sorted that, five more quirks I see for the authors to address.
>
> Introduction
> no mention of NMDA as required by RFC8407
>
>
> /interfaces/interface/interface-name {
>    must "/if:interfaces/if:interface[if:name = current()]/"
>                    + "ip:ipv4" {
> description  "The interface must have IPv4 enabled.";
>
> looks wrong to me.  If you want IPv4 enabled, then you need
> /ip:enabled
> set to true
>
>
>
>             "This augmentation is only valid for a control-plane
>              protocol instance of IGMP (type 'mld').";
>
> looks odd to me when the augment has
>         when "derived-from-or-self(rt:type, 'igmp-mld:mld')" {
>
>
>
>       rpc clear-igmp-groups {
>             description
>               "Name of the IGMP interface.
>                If it is not specified, groups from all interfaces are
>                cleared.";
>
> taken at face value, this clears all MLD groups as well as IGMP groups;
> is this intended?  If not, suggest
>
>                If it is not specified, groups from all IGMP interfaces
> are
>                cleared.";
>
> ditto
>       rpc clear-mld-groups {
>
>
> Tom Petch
>
> ----- Original Message -----
> From: "Jan Lindblad" <janl@tail-f.com>;
> Sent: Saturday, February 09, 2019 7:48 AM
>
> Tom,
>
> > Agree with your comments but I had a more fundamental problem trying
> to
> > understand theYANG module, about the repeated use of IGMP and/or MLD
> as
> > in
>
> As you know I'm not the author of this YANG, but I can explain how it
> works.
>
> >      feature global-admin-enable {
> > Is that one or or the other or both? Or
> >        leaf enable {
> >          type boolean;           default false;
> >          description "true to enable IGMP or MLD in the routing
> > instance;
> > Is that one or the other or both?
> > And so on. I am uncertain how the phrases are meant to be interpreted.
> >
> > The only way I could make sense of this was to assume that an
> > implementation must implement and support both IGMP and MLD; which is
> of
> > course blown out of the water by including separate features for IGMP
> > and MLD.
>
> No, this construct will not force an implementation to do both igmp and
> mld. There are other features to control which protocols to implement
> (feature-igmp, feature-mld). Leaf enable is modeled in a grouping (you
> can think of it as a macro). This grouping is used once in igmp and once
> in mld, so there are two separate enable leafs if both features are
> implemented.
>
> The single global-admin-enable feature does force an implementation that
> has both igmp and mld to support the global enable leaf either for both
> protocols, or for none of them. It's not possible to support global
> enable for one protocol but not the other, when both protocols are
> implemented.
>
> > In  a similar vein, SSM is repeatedly mentioned, never explained,
> never
> > referenced and in a sense, this is another protocol to add to IGMP and
> > MLD
>
> I have no idea what SSM is, and the module does little to help on that
> front. Good point.
>
> /jan
>
> > ----- Original Message -----
> > From: "Jan Lindblad" <janl@tail-f.com>;
> > Sent: Thursday, February 07, 2019 4:01 PM
> >
> >
> >> Reviewer: Jan Lindblad
> >> Review result: Ready with Issues
> >>
> >> This is my YANG-doctor review of draft-ietf-pim-igmp-mld-yang-10. In
> > the
> >> spring, I did an early review of the -02, and in the early fall a
> last
> > call
> >> review of the -07 version. The module has progressed significantly
> > since then.
> >> In particular the two main issues with -07 have been fixed, which is
> > great!
> >> There are still a few things that should be looked at, however.
> >>
> >> Since some of the current issues are the same as addressed in earlier
> > comments,
> >> I will reuse the numbering from the -07 review, and add a few points
> > at the end:
> >>
> >> #3 Top level structures not optional
> >>
> >> If-feature statements have been introduced so that an implementor can
> > choose
> >> whether to implement igmp and/or mld separately. This is very good.
> > RFC 8349,
> >> which the current module augments, recommend that the top level
> > containers be
> >> presence containers. This is currently not the case, and I see no
> > reason why
> >> they aren't.
> >>
> >>   It is also RECOMMENDED that protocol-specific data nodes be
> >>   encapsulated in an appropriately named container with presence.
> > Such
> >>   a container may contain mandatory data nodes that are otherwise
> >>   forbidden at the top level of an augment.
> >>
> >> At this time, there are no mandatory leafs under the top level
> > containers,
> >> which makes this recommendation less of a concern, but if a mandatory
> > leaf is
> >> introduced (e.g. see #9 below), making the top level containers
> > presence
> >> containers will be essential.
> >>
> >> #4 Unclear meaning of optional leaves
> >>
> >> While a lot of default values have been introduced (great!), there
> are
> > still
> >> many optional configuration leafs with no default value, not
> > mandatory, not
> >> self-evident and no description of what absence signifies. A few
> words
> > in the
> >> description might be all it takes to fix this.
> >>
> >> This happens with many leafs, but a couple of examples:
> >>
> >>     leaf max-entries {
> >>       if-feature global-max-entries;
> >>       type uint32;
> >>       description
> >>         "The maximum number of entries in IGMP or MLD.";
> >>     }
> >>
> >> If no max-entries is configured, what is the prescribed behavior?
> Will
> > all
> >> implementations agree that there is no max then? If so, maybe mention
> > that in
> >> the description?
> >>
> >>     leaf source-policy {
> >>       if-feature intf-source-policy;
> >>       type leafref {
> >>         path "/acl:acls/acl:acl/acl:name";
> >>       }
> >>       description
> >>         "Name of the access policy used to filter sources.
> >>          A device can restrict the length
> >>          and value of this name, possibly space and special
> >>          characters are not allowed.";
> >>     }
> >>
> >> If no source policy is specified, does that mean there is no policy?
> >>
> >> There are many more.
> >>
> >> #8 What policy name?
> >>
> >> In leaf ssm-map-group-policy, the description begins with "Name of
> the
> > policy
> >> ....". Does this mean that the policy name can be chosen freely, or
> > does the
> >> policy name entered here have to match some policy name specified
> > somewhere
> >> else?
> >>
> >> If this name can be chosen freely without considering some other
> list,
> > it's all
> >> fine.
> >>
> >>       leaf ssm-map-group-policy {
> >>         type string;
> >>         description
> >>           "Name of the policy used to define ssm-map rules.
> >>            A device can restrict the length
> >>            and value of this name, possibly space and special
> >>            characters are not allowed. ";
> >>
> >> #9 Default for version
> >>
> >> Leaf version is given a default of 2; this sounds excellent. This
> > clarifies
> >> what protocol version to use even if the operator doesn't specify.
> > Note that a
> >> default can never be changed, however, so only do this if 2 will feel
> > like a
> >> good number even 10 years from now.
> >>
> >> If no suitable default value that withstands time can be selected,
> > another
> >> option is to make it mandatory for the client to configure. Note that
> > in such a
> >> case #3 above becomes very important to resolve.
> >>
> >> The authors should
> >>     leaf version {
> >>       type uint8 {
> >>         range "1..3";
> >>       }
> >>       default 2;
> >>
> >> #10 Inaccurate description or expression
> >>
> >> In the interface-name leaf, there is a must statement and description
> > which
> >> contradict each other. The must statement requires that the ipv4
> > container is
> >> configured, while the description says ipv4 should be enabled. It is
> > possible
> >> to configure the ipv4 container but have ipv4/enabled = 'false', so
> > the must
> >> expression only cares whether the ipv4 presence container is
> > configured, not if
> >> it is enabled. Which one reflects the desired behavior?
> >>
> >>           leaf interface-name {
> >>             type if:interface-ref;
> >>             must "/if:interfaces/if:interface[if:name = current()]/"
> >>                + "ip:ipv4" {
> >>               description
> >>                 "The interface must have IPv4 enabled.";
> >>
> >> If ipv4 needs to be enabled, the must expression should be
> >>
> >>             must "/if:interfaces/if:interface[if:name = current()]/"
> >>                + "ip:ipv4/ip:enabled = 'true'" {
> >>
> >> If ipv4 needs to be configured, the description should say "The
> > interface must
> >> have IPv4 configured (but not necessarily enabled)."
> >>
> >> The same logic applies to MLD and ipv6 as well.
>
>