Subject: [pim] Review of draft-ietf-pim-igmp-mld-yang-03

From: Stig Venaas <[email-masked]>
Date: Mon, 12 Jun 2017 11:39:56 -0700

Hi

I reviewed this draft to see if I think it is ready for WGLC. I found
some issues that
might be good to resolve first. Would you be able to address my comments and
publish a new revision soon?

Some of my comments are just questions, but I'm sure at least some changes are
needed.

Many places you have a reference where there is no spacing in front of
the "[". E.g. "IGMPv2[RFC2236]". Please write it as "IGMPv2 [RFC2236]".
Among other things, it means that symlinks will work for the HTML version.

Section 1:
   This document defines a draft YANG data model that can be used to
   configure and manage Internet Group Management Protocol (IGMP) and
   Multicast Listener Discovery (MLD) devices. Currently this model is
   incomplete, but it will support the core IGMP and MLD protocols, as
   well as many other features mentioned in separate IGMP and MLD RFCs.
   Non-core features are defined as optional in the provided data
   model.

Should it still say that it is a draft and that this model is
incomplete? If so, say something about this model is incomplete rather
than using the term "currently".

Section 2.1:
   The representation of some of extension features is not specified in
   this draft of the data model.  This model is being circulated in its
   current form for early oversight and review of the basic hierarchy.

You should rewrite some of this now that it might be published as an
RFC. Maybe "not specified in this document"? Also the second sentence
doesn't make sense any longer.

In the next paragraph you use the term "rpcs". Should it be lower case?
Maybe it should say "RPC definitions", "rpc definitions" or something?

Section 2.2
Also here and other places you probably want to replace the word "draft"
with e.g. "document".

Section 2.3
I think you should write "the ipv4(IGMP)and ipv6(MLD)address" as
"the IPv4 (IGMP) and IPv6 (MLD) address".

Section 3.1:
   Interface-global: IGMP or MLD configuration attributes applicable
   to all interfaces IGMP MLD configuration attributes applied to
   interfaces whose interface level attributes are not existing, with
   same attributes' value for those

I'm having a hard time parsing this text. Can you try to rewrite it?
Similar problem with the corresponding sentence in 3.2.

Further down in this section it says "Our current direction is to
agree to". You probably should rewrite that.

Section 4:
      revision 2017-03-13 {
        description
          "Initial revision.";

Should it say that it is the initial revision?

It might be good to have some text describing the concept with basic and
extended intervals. You've done that many places. In the description it
just says "different value range", which is not that clear.

I have a question about this one in particular
          leaf last-member-query-interval-basic {
            type uint16 {
              range "1..65535";
            }
            units seconds;
            default 1;
            description
              "Last Member Query Interval, which may be tuned to modify the
              leave latency of the network.";
              reference "RFC3376. Sec. 8.8.";
          }
          leaf last-member-query-interval-extended {
            if-feature intf-last-member-query-interval-extended;
            type uint16;
            units seconds;
            default 1;
            description
              "Last Member Query Interval, which may be tuned to modify the
               leave latency of the network.";
             reference "RFC3376. Sec. 8.8.";
          }
        }

Here it seems both the basic and the extended range use uint16, so
they both are the same range (max 65535)? Do you need both basic and
extended here? Is it because vendors cannot specify a custom range if
range is already defined? Same question for query-max-response-time,
basic and extended.

          leaf query-interval-basic {
            type uint16 {
              range "1..31744";
            }
          }
Are there implementations that only support up to 31744?

          leaf robustness-variable-basic {
            type uint8 {
              range "2..7";
            }
There are vendors that allow larger than 2, but not 1?

In the description of group policy for ssm-map it says:

            description
              "Name of the access policy used to filter IGMP
               membership.A device MAY restrict the length
               and value of this name, possibly space and special
               characters are not allowed.";

I don't think it is appropriate to use normative language in the
descriptions. But I may be wrong. Please add a space after the ".".

The grouping interface-config-attributes-igmp-mld is for both IGMP
and MLD, but the leaf descriptions only mention IGMP. Similar to
the above description, you probably should not use "MAY", and a
missing space after ".". Please look for this other placs too.
E.g. the mld interface config grouping.

The "version" leaf accepts versions 1-3, the description says IGMP,
and references IGMP RFCs. But I think you use this for MLD as well.
Would you need to somehow prevent version 3 for MLD?

In many descriptions it says:
              "List of host address that
               joined the multicast group";

Should say "host addresses". Or maybe "hosts". or maybe "List of
addresses of hosts that joined the multicast group".

Makes this plural.
        leaf host-count {
          type uint32;
            description
              "The number of host address.";
        }

        leaf up-time {
          type uint32;
          units seconds;
          description
            "The time after the device created multicast group record.";
        }

Perhaps it would be better to say "Elapsed time since"? The same for
some other places where it says "time after".

A couple of places for "expire" leaf the description is:
            "The time left before multicast group timeout.";

Perhaps it would be better to say "group expires". Or "group times out"?
Or maybe "group state expires"?

I have a question about "rpc clear-igmp-groups" and the same for MLD.
It says:
      rpc clear-igmp-groups {
        if-feature rpc-clear-groups;
        description
          "Clears the specified IGMP cache tables.";
and also:
          leaf group {
            type inet:ipv4-address;
            description
              "Multicast group IPv4 address.
               If it is not specified, all IGMP group tables are
               cleared.";
          }
          leaf source {
            type inet:ipv4-address;
            description
              "Multicast source IPv4 address.
               If it is not specified, all IGMP source-group tables are
               cleared.";
          }

Shouldn't it say "entries" instead of "tables"?

Stig

---

From: Stig Venaas <[email-masked]>
Date: Fri, 23 Jun 2017 14:20:53 -0700

Hi

I don't think I've seen any response for this. Mike and I would like
to see an update of the draft before we issue WGLC. The submission
deadline for the next IETF is in about 10 days.

Stig

---

From: Xufeng Liu <[email-masked]>
Date: Fri, 23 Jun 2017 21:50:50 +0000

Hi Stig,

We are working on it. Should submit an update in a few days.

Thanks,
- Xufeng

-------- Original message --------
From: Stig Venaas <[email-masked]>
Date: 06/23/2017 5:21 PM (GMT-05:00)
To: [email-masked], [email-masked]
Subject: Re: Review of draft-ietf-pim-igmp-mld-yang-03

Hi

Stig