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

Xufeng Liu <xufeng.liu.ietf@gmail.com> Tue, 12 February 2019 00:01 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 6BB9F12941A; Mon, 11 Feb 2019 16:01:02 -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 RCavAUYf5ILL; Mon, 11 Feb 2019 16:00:58 -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 83F3212426A; Mon, 11 Feb 2019 16:00:58 -0800 (PST)
Received: by mail-it1-x12b.google.com with SMTP id g85so2862952ita.3; Mon, 11 Feb 2019 16:00:58 -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=8ZtHPdlfnfqjSNL17gF8yM0PFLePbShZwtsbt9G7qH8=; b=V2BbDo+sQwxL5vN/rFX9dN+EMyCqAf7kFvw9fsN6m4A5JDNathdW2ODkkr41pyEJj3 zAPeRGVxMCdtU2g7sSjMPvaXUiEKZnO7On8KxlYm8YOjB+cA2WTGkZ+RlnYFQpzDAPNT AEOXR6jelvxstQpUdzg2Ar5QgxtIukSG2F0NHdrC5flLj1tDWz7CExnDLV/Jcv/icjg4 dHl7dnRfyZhZGQoH8WHb1DIAUT64QmbAz3a85XswG2BTlimXU3j642uBRyKkNinF+WwU tcnDCesHgYY7i/YeSv9Iq+dMLU0pjGtg2BIbfWefAgkjZ4sLXZ7tZoaVjinsmIOr4qu7 oi9w==
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=8ZtHPdlfnfqjSNL17gF8yM0PFLePbShZwtsbt9G7qH8=; b=Lbyrgjlz2Beit70iEcRdl9w8V5+m26OQlLui/o6X+C25y8CllQBccnh2vsi2E/uiCa h/8CZHKI1rOmgbK3Q8MLcuP5GUHvo8yPPQ5c14L1UZ8o7k9d6FZJtqkhtd6092L62NT0 LiePOfsEBOMHq32nisJBcks5h9JVMRWzYsaoVFa+O6jXUuZw4ks5hzoCkULOkhYiJWMs exqAzG3rRXfVOgRp2nEUDlAOqJcPWPBxKQ9sBxdNNRmFVTVHKaItituYBY6dP8Eg8udC o1serRe4N6suYENTiM6PoDO/frbdsm4Mfr+V+9OEtnaXL9fQGr0A7pCDznOYPOh6XSWe 0YXg==
X-Gm-Message-State: AHQUAuZR9PTXFodGhkO+FzxV1NMjNnBULqPARk3v1ccKxjx3ejJYtvH+ Y88huglKPoVXAos2zzcD3ugs87LAq069TATq4dE=
X-Google-Smtp-Source: AHgI3Ia+6vQaTdnb/MtiwgsXy3427yqriFxendU/MM5tTcURaI+b979yhl+3mMOFBTyiQ6ahdAMJFJQgdaMmmZCEwjc=
X-Received: by 2002:a24:9686:: with SMTP id z128mr463415itd.97.1549929657459; Mon, 11 Feb 2019 16:00:57 -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> <20190209075246.6gqnbmsrxvle5nx4@anna.jacobs.jacobs-university.de> <FECB8944-1B7E-4EB3-8BCE-16E413F31849@tail-f.com> <20190209085847.ovasmvidonjxdimy@anna.jacobs.jacobs-university.de> <F77DB351-48E7-46A5-9BFD-65AF41B431B4@tail-f.com>
In-Reply-To: <F77DB351-48E7-46A5-9BFD-65AF41B431B4@tail-f.com>
From: Xufeng Liu <xufeng.liu.ietf@gmail.com>
Date: Mon, 11 Feb 2019 19:00:45 -0500
Message-ID: <CAEz6PPRrA4sNm+Wg9gNzBxtTiLEEkkwE1V=2tGCUhXq2AMzKaA@mail.gmail.com>
To: Jan Lindblad <janl@tail-f.com>
Cc: Juergen Schoenwaelder <j.schoenwaelder@jacobs-university.de>, tom petch <daedulus@btconnect.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="000000000000a8e7040581a71e7c"
Archived-At: <https://mailarchive.ietf.org/arch/msg/pim/GhiCy0RQOuArdahBk54fIEH1Wg0>
Subject: Re: [pim] [yang-doctors] 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: Tue, 12 Feb 2019 00:01:03 -0000

Hi Tom,

The intended usage of the feature igmp, mld, and global-admin-enable is as
what Jan explained. Various combinations of these features provide a
certain level of flexibility.

Hi Jan,

The node "control-plane-protocol" where the augmentation is applied is a
list entry, which can already achieve what a presence container can do. I
don't think that we need to make igmp and mld presence containers in this
case. The mandatory leaves are already possible because of the list
statement. Right?

Thanks,
- Xufeng

On Sat, Feb 9, 2019 at 6:13 AM Jan Lindblad <janl@tail-f.com>; wrote:

> > If there are individual enable leafs, then the combined enable leaf is
> > (i) not needed and (ii) very easy to implement and clearly not
> > justifying a feature.
>
> There is one global enable leaf for igmp, another for mld. Both are
> conditioned on the same feature. Currently the containers igmp and mld are
> not presence containers, so these global switches may have some use.
> Presence containers would be better (as pointed out in my review), but that
> does not preclude enable leafs.
>
> /jan
>
>
> > On Sat, Feb 09, 2019 at 09:41:20AM +0100, Jan Lindblad wrote:
> >> J├╝rgen,
> >>
> >>> They have a feature to control a global enable leaf???
> >>
> >> That's right. It's called "global-admin-enable" and is used for nothing
> else. But I guess features are cheap.
> >>
> >> /jan
> >>
> >>
> >>
> >>> On Sat, Feb 09, 2019 at 08:48:22AM +0100, Jan Lindblad wrote:
> >>>> 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.
> >>>>>>
> >>>>>
> >>>>
> >>>> _______________________________________________
> >>>> yang-doctors mailing list
> >>>> yang-doctors@ietf.org
> >>>> https://www.ietf.org/mailman/listinfo/yang-doctors
> >>>
> >>> --
> >>> Juergen Schoenwaelder           Jacobs University Bremen gGmbH
> >>> Phone: +49 421 200 3587         Campus Ring 1 | 28759 Bremen | Germany
> >>> Fax:   +49 421 200 3103         <https://www.jacobs-university.de/>
> >>>
> >>
> >
> > --
> > Juergen Schoenwaelder           Jacobs University Bremen gGmbH
> > Phone: +49 421 200 3587         Campus Ring 1 | 28759 Bremen | Germany
> > Fax:   +49 421 200 3103         <https://www.jacobs-university.de/>
> >
>
>