Re: [pim] Hi Reshad, the issues about igmp snooping model are addressed. Thanks a lot! Re: [yang-doctors] Yangdoctors early review of draft-ietf-pim-igmp-mld-snooping-yang-03

["Reshad Rahman (rrahman)" <[e-mail]>]

Hi Hongji,

I’ve taken a look at the latest rev, thanks for changing the model to augment l2vpn-instance and bridge, I believe this improves the model substantially. Before posting review comments on the latest version, I’d like to get clarifications/closure on the following (this was my 2nd main comment from initial review, email attached).

With the augment of if:interface, we have IGMP/MLD snooping data both under l2vpn-instance or bridge (via the augment) and under if:interface. To me such a segmentation of the IGMP/MLD snooping data is undesirable, is it not possible to have the data for static-mrouter-interface and static-l2-multicast-group under l2vpn-instance or bridge? For example, looking at static-mrouter-interface, we have leaf-lists of ACs/PWs and then under interface we have an l2vpn-instance name (should be leafref btw), so looks like interface refers to l2vpn-instance and l2vpn-instance has lists of ACs and PWs for the same mrouter-interface functionality. I don’t get it.

Regards,
Reshad.

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?
--------Augmenting if:interface in this model is used to configure static multicast router interface (static-mrouter-interface) and static l2 multicast forward table.
                    They are the concepts in IGMP/MLD Snooping. Augmenting if:interface could cover BRIDGE and L2VPN scenario at the same time.




From: Hongji Zhao <[e-mail]>
Date: Monday, August 6, 2018 at 11:36 PM
To: "Reshad Rahman (rrahman)" <[e-mail]>
Cc: "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>
Subject: Hi Reshad, the issues about igmp snooping model are addressed. Thanks a lot! Re: [yang-doctors] Yangdoctors early review of draft-ietf-pim-igmp-mld-snooping-yang-03

Hi Reshad,

I have updated the draft-ietf-pim-igmp-mld-snooping-yang according to your comments and upload the latest version.

In the new version I augmented the l2vpn-instance (from draft-ietf-bess-l2vpn-yang) and ieee802-dot1q-bridge module instead of the old reference style. I also addressed your other comments.

Could you please review it again? Thanks a lot!



https://datatracker.ietf.org/doc/draft-ietf-pim-igmp-mld-snooping-yang/


BR/Hongji

赵宏吉



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" <[e-mail] on behalf of rrahman=[e-mail]<mailto:[e-mail]%20on%20behalf%20of%20rrahman=[e-mail]>> 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)      The YANG model has a new container+list for bridges and

    “l2vpn-instances”.  Why not augment l2vpn-instance (from

    draft-ietf-bess-l2vpn-yang)? …



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

--- Begin Message ---

Hi Reshad,

 

Below are the explanations for major issues.  Thank you very much! 

 

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.

--------  There are 2 reasons for not augment l2vpn-instance

   a) If augment l2vpn-instance, the igmp-snooping-instances need to defined as grouping. This igmp snooping model will not be an independent model, and couldn't be augmented furtherly;

   b) If augment l2vpn-instance and BRIDGE, this draft will rely on l2vpn-instance and BRIDGE.

 

 

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?

--------Augmenting if:interface in this model is used to configure static multicast router interface (static-mrouter-interface) and static l2 multicast forward table.

                    They are the concepts in IGMP/MLD Snooping. Augmenting if:interface could cover BRIDGE and L2VPN scenario at the same time. 

                                

 

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)?

--------Generally IGMP/MLD Snooping is corresponding to l2vpn-instance. They are in the same level. AC or pw is lower than 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)?

--------It is similar as Question 1.

 

- 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.

  ----------------There are config data and stata data in igmp-snooping-instance. So I think it is an instance. Generally there is no state data in profile.

                                                                  1 instance is usable in multiple l2vpn or BRIDGE instances, but we don't recommend.

 

 

 

BR/Hongji

赵宏吉

 

-----Original Message-----
From: Reshad Rahman (rrahman) <[e-mail]> 
Sent: Friday, June 29, 2018 5:57 AM
To: [e-mail]; [e-mail]
Cc: [e-mail]
Subject: Re: [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" <[e-mail]  on behalf of rrahman=[e-mail]> 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:

    - [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

    

    

    _______________________________________________

    yang-doctors mailing list

    [e-mail]

    https://www.ietf.org/mailman/listinfo/yang-doctors

 

--- End Message ---