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

["Reshad Rahman (rrahman)" <[e-mail]>]

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:

-          [e-mail]<mailto:[e-mail]>


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 <[e-mail]>
Date: Thursday, October 11, 2018 at 10:11 PM
To: "Reshad Rahman (rrahman)" <[e-mail]>
Cc: "[e-mail]" <[e-mail]>
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: [e-mail]<mailto:[e-mail]>

To: <[e-mail]<mailto:[e-mail]>>

Cc: [e-mail]<mailto:[e-mail]>

Subject: [pim] I-D Action:

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

Message-ID: <[e-mail]<mailto:[e-mail]>>

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) <[e-mail]>
Sent: Wednesday, October 10, 2018 8:53 PM
To: Hongji Zhao <[e-mail]>

Cc: YANG Doctors <[e-mail]>; [e-mail]; [e-mail]; Acee Lindem (acee) <[e-mail]>
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 <[e-mail]<mailto:[e-mail]>>
Date: Monday, October 8, 2018 at 10:44 PM
To: "Reshad Rahman (rrahman)" <[e-mail]<mailto:[e-mail]>>
Cc: YANG Doctors <[e-mail]<mailto:[e-mail]>>, "[e-mail]<mailto:[e-mail]>" <[e-mail]<mailto:[e-mail]>>, "[e-mail]<mailto:[e-mail]>" <[e-mail]<mailto:[e-mail]>>, "Acee Lindem (acee)" <[e-mail]<mailto:[e-mail]>>
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
赵宏吉