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

[Stig Venaas <[e-mail]>]

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


On Mon, Jun 12, 2017 at 11:39 AM, Stig Venaas <[e-mail]> wrote:
> 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