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

[Hongji Zhao <[e-mail]>]

Hi Reshad,

  Thank you very much for your comments. I will address them as soon as possible.

BR/Hongji
赵宏吉

-----Original Message-----
From: Reshad Rahman <[e-mail]> 
Sent: Friday, June 29, 2018 5:42 AM
To: [e-mail]
Cc: [e-mail]; [e-mail]
Subject: Yangdoctors early review of draft-ietf-pim-igmp-mld-snooping-yang-03

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