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

[Xufeng Liu <[e-mail]>]

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 <[e-mail]> 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" <[e-mail]>
> >>>>> 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
> >>>> [e-mail]
> >>>> https://www.ietf.org/mailman/listinfo/yang-doctors
> >>>
> >>> --
> >>> Juergen Schoenwaelder           Jacobs University Bremen gGmbH
> >>> Phone: [phone]         Campus Ring 1 | 28759 Bremen | Germany
> >>> Fax:   [phone]         <https://www.jacobs-university.de/>
> >>>
> >>
> >
> > --
> > Juergen Schoenwaelder           Jacobs University Bremen gGmbH
> > Phone: [phone]         Campus Ring 1 | 28759 Bremen | Germany
> > Fax:   [phone]         <https://www.jacobs-university.de/>
> >
>
>