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

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

Hi Hongji,

Yes having the 2 identities is good.

You don’t need to have a leaf name because control-plane-protocol already has that.

       container control-plane-protocols {
         description
           "Support for control-plane protocol instances.";
         list control-plane-protocol {
           key "type name";
           description
             "Each entry contains a control-plane protocol instance.";
           leaf type {
             type identityref {
               base control-plane-protocol;
             }
             description
               "Type of the control-plane protocol -- an identity
                derived from the 'control-plane-protocol'
                base identity.";
           }
           leaf name {
             type string;
             description
               "An arbitrary name of the control-plane protocol
                instance.";
           }


Regards,
Reshad.

From: Hongji Zhao <[e-mail]>
Date: Thursday, October 25, 2018 at 12:04 AM
To: "Reshad Rahman (rrahman)" <[e-mail]>
Cc: "[e-mail]" <[e-mail]>, YANG Doctors <[e-mail]>
Subject: RE: Yangdoctors early review of draft-ietf-pim-igmp-mld-snooping-yang-05

Hi Reshad,


  1.  I have 2 new identities , 1 for MLD instance and 1 for IGMP snooping. Is it ok?


identity igmp-snooping {
    base rt:control-plane-protocol;
    description
      "IGMP snooping protocol";
  }
augment "/rt:routing/rt:control-plane-protocols/rt:control-plane-protocol" {
    container igmp-snooping-instance {
                  when "derived-from-or-self(../type, 'ims:igmp-snooping')" {
        description
          "This container is only valid for IGMP snooping protocol.";
      }

      leaf name {
        type string;
        description
          "Name of the igmp-snooping-instance";
      }
    …
}



  1.  I have added the prefix “num-” to the statistics
|        +--ro statistics
    |           +--ro received
    |           |  +--ro num-query?                  yang:counter64
    |           |  +--ro num-membership-report-v1?   yang:counter64
    |           |  +--ro num-membership-report-v2?   yang:counter64
    |           |  +--ro num-membership-report-v3?   yang:counter64
    |           |  +--ro num-leave?                  yang:counter64
   |           |  +--ro num-non-member-leave?       yang:counter64
    |           |  +--ro num-pim-hello?              yang:counter64




BR/Hongji
赵宏吉

From: Reshad Rahman (rrahman) <[e-mail]>
Sent: Wednesday, October 24, 2018 5:00 AM
To: Hongji Zhao <[e-mail]>
Cc: [e-mail]; YANG Doctors <[e-mail]>
Subject: Re: Yangdoctors early review of draft-ietf-pim-igmp-mld-snooping-yang-05

Hi Hongji,

Inline.

From: Hongji Zhao <[e-mail]<mailto:[e-mail]>>
Date: Monday, October 22, 2018 at 10:16 PM
To: "Reshad Rahman (rrahman)" <[e-mail]<mailto:[e-mail]>>
Cc: "[e-mail]<mailto:[e-mail]>" <[e-mail]<mailto:[e-mail]>>, YANG Doctors <[e-mail]<mailto:[e-mail]>>
Subject: RE: Yangdoctors early review of draft-ietf-pim-igmp-mld-snooping-yang-05

Hi Reshad,

Regarding the main issues/questions:

  *     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.
        ----I would like to change the structure like this. What do you think of that?
augment /rt:routing/rt:control-plane-protocols/rt:control-plane-protocol:
                    +--rw igmp-snooping-instance* [name]
                             +--rw name                                 string
                        ...
                    +--rw mld-snooping-instance* [name]
                               +--rw name                                 string
                ...

<RR> Looks good to me. So you’ll 2 new identities, 1 for MLD instance and 1 for IGMP snooping?

Minor comments, suggestions and nits:
-              Leaf exclude-lite, in description should there an explanation or reference of what exclude lite means?
            ------------- I would like to change the description like this. What do you think of that?
                     leaf exclude-lite {
                               …
                               description
                                 "Track the Lightweight IGMPv3 and MLDv2 protocol report";
                               reference "RFC5790";
                     }
<RR> Good with me.

-              Grouping statistics-sent-received:
4.            Counter names, e.g. “pim” is too short, it should be something along the lines of num-pim-messages or num-pim.
-------------Is  pim-hello ok? Here we just want to count the pim hello packet which is related to multicast router interface.
              +--ro statistics
    |              +--ro received
    |              |  +--ro query?                  yang:counter64
    |              |  +--ro membership-report-v1?   yang:counter64
    |              |  +--ro membership-report-v2?   yang:counter64
    |              |  +--ro membership-report-v3?   yang:counter64
    |              |  +--ro leave?                  yang:counter64
    |              |  +--ro non-member-leave?       yang:counter64
    |              |  +--ro pim-hello?              yang:counter64

<RR> I would personally have used a more explicit name such as num-pim-hello (likewise for other counters), because pim-hello does not clearly indicate that it’s a counter. But I don’t feel strongly enough about this to object to what you propose above.

-              RPCs clear-xxx-snooping-groups, rename to clear-xxx-snooping-cache (as per description)?
              ----------------what about modify the description? I think clear-xxx-snooping-groups is better.
                                rpc clear-igmp-snooping-groups {
                                 if-feature rpc-clear-groups;
                                  description
                                   "Clear the specified IGMP snooping group.";
  input {
…
 <RR> I’ll defer to PIM WG to decide whether groups is the appropriate term.

-              Examples (Appendix A)
2.            Were the examples validated with a tool? You can use yanglib
-------------- I used json-yang-master to validate the examples, but didn’t use libyang.
                How to use libyang? Do you mean the yanglint?  Do you have some example about it?
<RR> If you used a tool to validate the examples, then good enough, you don’t need to use libyang.

Thanks for putting up with my reviews ☺

Regards,
Reshad.


Regarding other comments I have addressed and I will update the draft together with the comments above.  Thanks a lot!


BR/Hongji
赵宏吉