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

[Reshad Rahman <[e-mail]>]

Reviewer: Reshad Rahman
Review result: On the Right Track

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

1 module defined in this draft:
- [e-mail]

No YANG validation errors or warnings (from yang and yanglint).

1 example are provided in this draft.

Major issues perceived:
1)      The YANG model has a new container+list for bridges and
“l2vpn-instances”.  Why not augment l2vpn-instance (from
draft-ietf-bess-l2vpn-yang)? If all L2 features end up adding their own lists
for “l2vpn-instances” this will be messy and there’ll be no easy way to look at
all the configuration relevant to an l2vpn-instance, it’ll have to be done
feature by feature. 2)      If:interface is augmented and has the name of the
l2vpn-instance. This config seems redundant since under l2vpn-instance
(draft-ietf-bess-l2vpn-yang) there is already an interface-ref for AC (Access
Circuit). Why not augment the L2VPN endpoint or AC? 3)      There doesn’t seem
to be the capability to enable IGMP/MLD snooping on a subset of ACs or PW (i.e.
not on the full l2vpn-instance)? 4)      I thought Bridge related YANG models
belong to IEEE. But if we have to do the model for bridges in this draft, why
not augment IEEE YANG models e.g.  ieee802-dot1q-bridge.yang (same comment as
for l2vpn-instance)?

There might be good reasons to justify the way the YANG model has been done,
but if that's the case IMO there needs to be text which justifies the design of
the YANG model.

If the authors haven’t done so already I would suggest discussing with authors
of draft-ietf-bess-l2vpn-yang, IETF102 would be a good opportunity and I can
attend a meeting if needed.

I will have to re-review once the issues are addressed.

Other comments/questions/nits:
- General: needs spelling verification
- General: indentation of YANG model has to be fixed, also some descriptions
are too long and wrap. - Add NMDA in abstract (that's what most drafts now do)
- Section 1.1, add space after in "in[RFC6020]" - There are references for
L2VPN/EVPN YANG but none for bridges - Section 2, add reference for IGMP -
Section 2.2, 2nd paragraph needs rewording. Explanation of how reference also
not super clear (add reference to 2.4?) , e.g. what does an
igmp-snooping-instance correspond to (to me it seems to be more a profile than
the instances we have with routing protocols)? And is 1 instance usable in
multiple l2vpn or BRIDGE instances? I believe it’s for 1 instance? Anyway
clarify that. - Section 2.2, 4th paragraph, instead of “routing system” should
this be “snooping device”? - Section 2.5 "This model augment", should be
"augments". - YANG model: s/to configure the igmp snooping/to configure IGMP
snooping/ - YANG model, having a feature for supporting admin-enable seems like
overkill. My first impression was that that's a lot of features for this model,
but I guess that's debatable. - YANG model s/fowarding/forwarding/ - YANG
model, for the lookup modes (IP-based and MAC-based, add reference). I don’t
think adding a vendor-specific CLI-example in the YANG description is a good
idea. - YANG model, use yang-version 1.1 and add reference to import statements
(as per 6087bis) - YANG model, if per-instance-config feature is not supported,
how are the IGMP/MLD instances configured? - YANG model, vlan-index-type, use
of range 4096… not very clear. And vlan-id shouldn’t be uint32, uint16 is
enough. There’s also ieee:vlanid - YANG model, as opposed to using regular
address type for group/multicast addresses, is there a type already defined for
group addresses? If not there should be (V4 and V6) - YANG model,
host-filter-mode, add reference - Appendix A, fix diagram