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

FYI, easier to read https://datatracker.ietf.org/doc/review-ietf-pim-igmp-mld-snooping-yang-03-yangdoctors-early-rahman-2018-06-28/ than the email below.

On 2018-06-28, 5:42 PM, "yang-doctors on behalf of Reshad Rahman" <yang-doctors-bounces@ietf.org on behalf of rrahman=40cisco.com@dmarc.ietf.org> wrote:

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

    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

    _______________________________________________
    yang-doctors mailing list
    yang-doctors@ietf.org
    https://www.ietf.org/mailman/listinfo/yang-doctors