Early Review of draft-ietf-pim-igmp-mld-snooping-yang-05
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:
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.
- 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).