Early Review of draft-ietf-pim-igmp-mld-snooping-yang-03
review-ietf-pim-igmp-mld-snooping-yang-03-yangdoctors-early-rahman-2018-06-28-00

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:
-       ietf-igmp-mld-snooping@2018-10-11.yang

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: 1.      remove
-sent-received from name 2.      Description of counters, some have messages
and some don’t 3.      For all counters for messages, add a reference for the
messages 4.      Counter names, e.g. “pim” is too short, it should be something
along the lines of num-pim-messages or num-pim. 5.      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) 1.      Indentation is off, please correct
2.      Were the examples validated with a tool? You can use yanglib 3.     
bridge-mrouter-interface should be “eth1/1” instead of “1/1”. Likewise for
interface, bridge-outgoing-interface 4.      The example is for bridge, there
should be an example for l2vpn also -       Discrepancy in affiliation of
Mahesh (1st page v/s author’s addresses).