Subject: [yang-doctors] Yangdoctors early review of draft-ietf-pim-igmp-mld-snooping-yang-05

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Wed, 17 Oct 2018 18:01:26 +0000

Hi Hongji,

I have reviewed rev-05 (revised the existing review), see https://datatracker.ietf.org/doc/review-ietf-pim-igmp-mld-snooping-yang-03-yangdoctors-early-rahman-2018-06-28/
Since no email was sent, I have copied my comments below.

Regards,
Reshad.

YANG Doctor review of draft-ietf-pim-igmp-mld-snooping-yang-05 (by Reshad Rahman)

This is my YD review of -05, in June 2018 I had done an early review of -03.

1 module defined in this draft:

-          [email-masked]<mailto:[email-masked]>

Errors are shown at https://datatracker.ietf.org/doc/draft-ietf-pim-igmp-mld-snooping-yang/ but that seems to be tools related (yang modules can’t be found)

All major issues raised in previous review have been addressed.

Main issues/questions:

  *   igmp-snooping-instances (mld- also) are top level containers, I believe they should be under rt:control-plane-protocol (RFC8349). I should have raised this in the previous review.
  *   Should there be a dependency on draft-ietf-pim-igmp-mld-yang, e.g. should we allow IGMP/MLD snooping configuration only if IGMP/MLD is enabled (leaf “enable”) or supported (feature-mld and feature-igmp)? E.g. I don’t think it makes sense to configure igmp-snooping if igmp is not supported.

Minor comments, suggestions and nits:

  *   Section 2, I don’t see the point of last sentence “This document provides freedom….”.
  *   Section 2.1 has a reference to draft-dsdt-nmda-guidelines which has expired
  *   YANG indentation is off, please correct
  *   Comment “replace with IANA namespace” applies to what line?
  *   In YANG module, s/Refrence/Reference/ (1 instance)
  *   Leaf “type” in snooping-instances, why not have an identity for type (l2vpn and bridge only for now). Right now there’s an enum which is defined twice (for mld and igmp)
  *   Similar comment as above for leaf “host-filter-mode”
  *   Send-query: s/topo/topology/, s/param/parameter/
  *   For all feature definitions, add references (RFC + relevant sections)
  *   Leaf exclude-lite, in description should there an explanation or reference of what exclude lite means?
  *   Grouping statistics-sent-received:

     *   remove -sent-received from name
     *   Description of counters, some have messages and some don’t
     *   For all counters for messages, add a reference for the messages
     *   Counter names, e.g. “pim” is too short, it should be something along the lines of num-pim-messages or num-pim.
     *   In pim description s/pim/PIM/

  *   RPCs clear-xxx-snooping-groups, rename to clear-xxx-snooping-cache (as per description)?
  *   Security Considerations, you should also mention the xxx-snooping-instance leaf nodes under vlan and l2vpn
  *   Examples (Appendix A)

     *   Indentation is off, please correct
     *   Were the examples validated with a tool? You can use yanglib<https://github.com/CESNET/libyang>
     *   bridge-mrouter-interface should be “eth1/1” instead of “1/1”. Likewise for interface, bridge-outgoing-interface
     *   The example is for bridge, there should be an example for l2vpn also

  *   Discrepancy in affiliation of Mahesh (1st page v/s author author’s addresses).

From: Hongji Zhao <[email-masked]>
Date: Thursday, October 11, 2018 at 10:11 PM
To: "Reshad Rahman (rrahman)" <[email-masked]>
Cc: "[email-masked]" <[email-masked]>
Subject: Hi Reshad, I have submitted draft-ietf-pim-igmp-mld-snooping-yang-05 for your comments. Thanks a lot!

Hi Reshad,

I have submitted draft-ietf-pim-igmp-mld-snooping-yang-05 for the new structure. Could you please review again? Thanks a lot !

The IETF datatracker status page for this draft is:

https://datatracker.ietf.org/doc/draft-ietf-pim-igmp-mld-snooping-yang/

Message: 1

Date: Thu, 11 Oct 2018 02:22:27 -0700

From: [email-masked]<mailto:[email-masked]>

To: <[email-masked]<mailto:[email-masked]>>

Cc: [email-masked]<mailto:[email-masked]>

Subject: [pim] I-D Action:

                draft-ietf-pim-igmp-mld-snooping-yang-05.txt

Message-ID: <[email-masked]<mailto:[email-masked]>>

Content-Type: text/plain; charset="utf-8"

A New Internet-Draft is available from the on-line Internet-Drafts directories.

This draft is a work item of the Protocols for IP Multicast WG of the IETF.

        Title           : A Yang Data Model for IGMP and MLD Snooping

        Authors         : Hongji Zhao

                          Xufeng Liu

                          Yisong Liu

                          Mahesh Sivakumar

                          Anish Peter

                Filename        : draft-ietf-pim-igmp-mld-snooping-yang-05.txt

                Pages           : 42

                Date            : 2018-10-11

Abstract:

   This document defines a YANG data model that can be used to configure and manage Internet Group Management Protocol (IGMP) and Multicast Listener Discovery (MLD) Snooping devices. The YANG module in this document conforms to Network Management Datastore Architecture (NMDA).

BR/Hongji
赵宏吉

From: Reshad Rahman (rrahman) <[email-masked]>
Sent: Wednesday, October 10, 2018 8:53 PM
To: Hongji Zhao <[email-masked]>

Cc: YANG Doctors <[email-masked]>; [email-masked]; [email-masked]; Acee Lindem (acee) <[email-masked]>
Subject: Re: [yang-doctors] Hi Reshad, the issues about igmp snooping model are addressed. Thanks a lot!

Hi Hongji,

This structure looks good to me.

Regards,
Reshad.

From: Hongji Zhao <[email-masked]<mailto:[email-masked]>>
Date: Monday, October 8, 2018 at 10:44 PM
To: "Reshad Rahman (rrahman)" <[email-masked]<mailto:[email-masked]>>
Cc: YANG Doctors <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "Acee Lindem (acee)" <[email-masked]<mailto:[email-masked]>>
Subject: RE: [yang-doctors] Hi Reshad, the issues about igmp snooping model are addressed. Thanks a lot!

Hi Reshad & Acee,

I see your points and I plan to move the statistics under the snooping instance as below.   Is it ok?   Thanks a lot!

module: ietf-igmp-mld-snooping
    +--rw igmp-snooping-instances
    |  +--rw igmp-snooping-instance* [name]
    |     +--rw name                                 string
    |     ...
    |     +--ro interfaces
          |        +--ro interface* [name]
          |           +--ro name                   if:interface-ref
                      +--ro statistics
                         +--ro received
                         |  +--ro query?                  yang:counter64
                         |  +--ro membership-report-v1?   yang:counter64
                         |  +--ro membership-report-v2?   yang:counter64
                         |  +--ro membership-report-v3?   yang:counter64
                         |  +--ro leave?                  yang:counter64
                         |  +--ro non-member-leave?       yang:counter64
                         |  +--ro pim?                    yang:counter64
                         +--ro sent
                            +--ro query?                  yang:counter64
                           +--ro membership-report-v1?   yang:counter64
                            +--ro membership-report-v2?   yang:counter64
                            +--ro membership-report-v3?   yang:counter64
                            +--ro leave?                  yang:counter64
                            +--ro non-member-leave?       yang:counter64
                            +--ro pim?                    yang:counter64

BR/Hongji
赵宏吉

---

From: Hongji Zhao <[email-masked]>
Date: Thu, 18 Oct 2018 03:31:59 +0000

Hi Reshad,

Thanks a lot for your review.

Regarding tje main issues/questions:

              ------------------------I think it is ok, and will move igmp-snooping-instances under rt:control-plane-protocol

  *   Should there be a dependency on draft-ietf-pim-igmp-mld-yang, e.g. should we allow IGMP/MLD snooping configuration only if IGMP/MLD is enabled (leaf “enable”) or supported (feature-mld and feature-igmp)? E.g. I don’t think it makes sense to configure igmp-snooping if igmp is not supported.
                           -------------------------I think there is no dependency on draft-ietf-pim-igmp-mld-yang
                                                             IGMP is enabled on L3 router, but IGMP Snooping is enabled on L2 switch.  The switch snoops into IGMP Report and generates the mapping between port and Multicast MAC Address.
                                                             The IGMP Report is sent by host, passes the L2 switch, and to the L3 router.

                                                             So you can configure the IGMP snooping when IGMP is not enabled or supported.

Regarding other comments, I will research them and modify the draft ASAP.   Thanks a lot!

BR/Hongji
赵宏吉

From: Reshad Rahman (rrahman) <[email-masked]>
Sent: Thursday, October 18, 2018 2:01 AM
To: Hongji Zhao <[email-masked]>
Cc: [email-masked]; YANG Doctors <[email-masked]>
Subject: Yangdoctors early review of draft-ietf-pim-igmp-mld-snooping-yang-05

Hi Hongji,

Regards,
Reshad.

  *   [email-masked]<mailto:[email-masked]>

Main issues/questions:

---

From: Hongji Zhao <[email-masked]>
Date: Thu, 18 Oct 2018 03:44:28 +0000

Hi Reshad,

                                                             So you can configure the IGMP snooping when IGMP is not enabled or supported.
                                                             In the current implementation of IGMP snooping, both Cisco and Huawei don’t need IGMP enabled.   (https://www.cisco.com/c/en/us/td/docs/routers/7600/ios/15S/configuration/guide/7600_15_0s_book/snooigmp.html)

BR/Hongji
赵宏吉

Hi Hongji,

…
…

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Thu, 18 Oct 2018 18:01:16 +0000

Hi Hongji,

Regarding no dependency on igmp-mld-yang, you are correct (I didn’t think this one through).

Regards,
Reshad.

From: Hongji Zhao <[email-masked]>
Date: Wednesday, October 17, 2018 at 11:44 PM
To: "Reshad Rahman (rrahman)" <[email-masked]>
Cc: "[email-masked]" <[email-masked]>, YANG Doctors <[email-masked]>
Subject: RE: Yangdoctors early review of draft-ietf-pim-igmp-mld-snooping-yang-05

Hi Reshad,

BR/Hongji
赵宏吉

Hi Hongji,

---

From: Hongji Zhao <[email-masked]>
Date: Tue, 23 Oct 2018 02:16:43 +0000

Hi Reshad,

Regarding the main issues/questions:

  *     igmp-snooping-instances (mld- also) are top level containers, I believe they should be under rt:control-plane-protocol (RFC8349). I should have raised this in the previous review.
        ----I would like to change the structure like this. What do you think of that?
augment /rt:routing/rt:control-plane-protocols/rt:control-plane-protocol:
                    +--rw igmp-snooping-instance* [name]
                             +--rw name                                 string
                        ...
                    +--rw mld-snooping-instance* [name]
                               +--rw name                                 string
                ...

Minor comments, suggestions and nits:
-              Leaf exclude-lite, in description should there an explanation or reference of what exclude lite means?
            ------------- I would like to change the description like this. What do you think of that?
                     leaf exclude-lite {
                               …
                               description
                                 "Track the Lightweight IGMPv3 and MLDv2 protocol report";
                               reference "RFC5790";
                     }

-              Grouping statistics-sent-received:
4.            Counter names, e.g. “pim” is too short, it should be something along the lines of num-pim-messages or num-pim.
-------------Is  pim-hello ok? Here we just want to count the pim hello packet which is related to multicast router interface.
              +--ro statistics
    |              +--ro received
    |              |  +--ro query?                  yang:counter64
    |              |  +--ro membership-report-v1?   yang:counter64
    |              |  +--ro membership-report-v2?   yang:counter64
    |              |  +--ro membership-report-v3?   yang:counter64
    |              |  +--ro leave?                  yang:counter64
    |              |  +--ro non-member-leave?       yang:counter64
    |              |  +--ro pim-hello?              yang:counter64

-              RPCs clear-xxx-snooping-groups, rename to clear-xxx-snooping-cache (as per description)?
              ----------------what about modify the description? I think clear-xxx-snooping-groups is better.
                                rpc clear-igmp-snooping-groups {
                                 if-feature rpc-clear-groups;
                                  description
                                   "Clear the specified IGMP snooping group.";
  input {
…

-              Examples (Appendix A)
2.            Were the examples validated with a tool? You can use yanglib
-------------- I used json-yang-master to validate the examples, but didn’t use libyang.
                How to use libyang? Do you mean the yanglint?  Do you have some example about it?

Regarding other comments I have addressed and I will update the draft together with the comments above.  Thanks a lot!

BR/Hongji
赵宏吉

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Tue, 23 Oct 2018 20:59:52 +0000

Hi Hongji,

Inline.

Date: Monday, October 22, 2018 at 10:16 PM

Hi Reshad,

<RR> Looks good to me. So you’ll 2 new identities, 1 for MLD instance and 1 for IGMP snooping?

<RR> Good with me.

<RR> I would personally have used a more explicit name such as num-pim-hello (likewise for other counters), because pim-hello does not clearly indicate that it’s a counter. But I don’t feel strongly enough about this to object to what you propose above.

 <RR> I’ll defer to PIM WG to decide whether groups is the appropriate term.

-              Examples (Appendix A)
2.            Were the examples validated with a tool? You can use yanglib
-------------- I used json-yang-master to validate the examples, but didn’t use libyang.
                How to use libyang? Do you mean the yanglint?  Do you have some example about it?
<RR> If you used a tool to validate the examples, then good enough, you don’t need to use libyang.

Thanks for putting up with my reviews ☺

Regards,
Reshad.

BR/Hongji
赵宏吉

---

From: Hongji Zhao <[email-masked]>
Date: Thu, 25 Oct 2018 04:04:07 +0000

Hi Reshad,

  1.  I have 2 new identities , 1 for MLD instance and 1 for IGMP snooping. Is it ok?

identity igmp-snooping {
    base rt:control-plane-protocol;
    description
      "IGMP snooping protocol";
  }
augment "/rt:routing/rt:control-plane-protocols/rt:control-plane-protocol" {
    container igmp-snooping-instance {
                  when "derived-from-or-self(../type, 'ims:igmp-snooping')" {
        description
          "This container is only valid for IGMP snooping protocol.";
      }

      leaf name {
        type string;
        description
          "Name of the igmp-snooping-instance";
      }
    …
}

  1.  I have added the prefix “num-” to the statistics
|        +--ro statistics
    |           +--ro received
    |           |  +--ro num-query?                  yang:counter64
    |           |  +--ro num-membership-report-v1?   yang:counter64
    |           |  +--ro num-membership-report-v2?   yang:counter64
    |           |  +--ro num-membership-report-v3?   yang:counter64
    |           |  +--ro num-leave?                  yang:counter64
   |           |  +--ro num-non-member-leave?       yang:counter64
    |           |  +--ro num-pim-hello?              yang:counter64

BR/Hongji
赵宏吉

From: Reshad Rahman (rrahman) <[email-masked]>
Sent: Wednesday, October 24, 2018 5:00 AM
To: Hongji Zhao <[email-masked]>
Cc: [email-masked]; YANG Doctors <[email-masked]>
Subject: Re: Yangdoctors early review of draft-ietf-pim-igmp-mld-snooping-yang-05

Hi Hongji,

Inline.

Cc: "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, YANG Doctors <[email-masked]<mailto:[email-masked]>>

Hi Reshad,

-              Examples (Appendix A)

Regards,
Reshad.

BR/Hongji
赵宏吉

---

From: Hongji Zhao <[email-masked]>
Date: Thu, 25 Oct 2018 04:06:12 +0000

Hi Reshad,

identity igmp-snooping {

      leaf name {
        type string;
        description

BR/Hongji
赵宏吉

From: Reshad Rahman (rrahman) <[email-masked]<mailto:[email-masked]>>
Sent: Wednesday, October 24, 2018 5:00 AM
To: Hongji Zhao <[email-masked]<mailto:[email-masked]>>
Cc: [email-masked]<mailto:[email-masked]>; YANG Doctors <[email-masked]<mailto:[email-masked]>>
Subject: Re: Yangdoctors early review of draft-ietf-pim-igmp-mld-snooping-yang-05

Hi Hongji,

Inline.

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Thu, 25 Oct 2018 15:56:26 +0000

Hi Hongji,

Yes having the 2 identities is good.

You don’t need to have a leaf name because control-plane-protocol already has that.

       container control-plane-protocols {
         description
           "Support for control-plane protocol instances.";
         list control-plane-protocol {
           key "type name";
           description
             "Each entry contains a control-plane protocol instance.";
           leaf type {
             type identityref {
               base control-plane-protocol;
             }
             description
               "Type of the control-plane protocol -- an identity
                derived from the 'control-plane-protocol'
                base identity.";
           }
           leaf name {
             type string;
             description
               "An arbitrary name of the control-plane protocol
                instance.";
           }

Regards,
Reshad.

Date: Thursday, October 25, 2018 at 12:04 AM

Hi Reshad,

identity igmp-snooping {

      leaf name {
        type string;
        description

BR/Hongji
赵宏吉

Hi Hongji,

Inline.

Hi Reshad,

-              Examples (Appendix A)

Regards,
Reshad.

BR/Hongji
赵宏吉