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

[Hongji Zhao <[e-mail]>]

Hi Reshad,

Thanks a lot for your review.

Regarding tje 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 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) <[e-mail]>
Sent: Thursday, October 18, 2018 2:01 AM
To: Hongji Zhao <[e-mail]>
Cc: [e-mail]; YANG Doctors <[e-mail]>
Subject: Yangdoctors early review of draft-ietf-pim-igmp-mld-snooping-yang-05

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).