On Mon, Jan 22, 2024 at 5:26 PM Dumitru Ceara <dce...@redhat.com> wrote:
> On 1/22/24 15:14, Mohammad Heib wrote: > > Expose the igmp/mld group protocol version through the > > IGMP_GROUP table in SBDB. > > > > This patch can be used by ovn consumer for debuggability purposes, user > > now can match between the protocol version used in the OVN logical > > switches and the uplink ports. > > > > Rreported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2160476 > > Signed-off-by: Mohammad Heib <mh...@redhat.com> > > --- > Hi Dumitru, Thank you for your review :), i have small question please see below. > Hi Mohammad, > > I have a few (minor) comments, please see below. > > > NEWS | 2 ++ > > controller/ip-mcast.c | 8 ++++++++ > > controller/ip-mcast.h | 3 +++ > > controller/pinctrl.c | 19 ++++++++++++++++++- > > northd/ovn-northd.c | 2 +- > > ovn-sb.ovsschema | 5 +++-- > > ovn-sb.xml | 4 ++++ > > tests/ovn.at | 3 +++ > > 8 files changed, 42 insertions(+), 4 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index 5f267b4c6..9075e7d80 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -14,6 +14,8 @@ Post v23.09.0 > > - ovn-northd-ddlog has been removed. > > - A new LSP option "enable_router_port_acl" has been added to enable > > conntrack for the router port whose peer is l3dgw_port if set it > true. > > + - IGMP_Group have a new "protocol" column that displays the the group > > Nit: s/have/has > > > + protocol version. > > > > OVN v23.09.0 - 15 Sep 2023 > > -------------------------- > > diff --git a/controller/ip-mcast.c b/controller/ip-mcast.c > > index a870fb29e..83e41c81f 100644 > > --- a/controller/ip-mcast.c > > +++ b/controller/ip-mcast.c > > @@ -226,6 +226,14 @@ igmp_group_cleanup(struct ovsdb_idl_txn > *ovnsb_idl_txn, > > return true; > > } > > > > + > > +void igmp_group_set_protocol(const struct sbrec_igmp_group *group, > > + mcast_group_proto protocol) > > +{ > > + sbrec_igmp_group_set_protocol(group, > > + > mcast_snooping_group_protocol_str(protocol)); > > +} > > + > > Does it make more sense to add the protocol as argument to > igmp_group_update_ports() and rename that function to igmp_group_update()? > yes that make sense and will make the code more cleaner. > > > static const struct sbrec_igmp_group * > > igmp_group_lookup_(struct ovsdb_idl_index *igmp_groups, > > const char *addr_str, > > diff --git a/controller/ip-mcast.h b/controller/ip-mcast.h > > index 326f39db1..f0c34343f 100644 > > --- a/controller/ip-mcast.h > > +++ b/controller/ip-mcast.h > > @@ -63,4 +63,7 @@ void igmp_group_delete(const struct sbrec_igmp_group > *g); > > bool igmp_group_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, > > struct ovsdb_idl_index *igmp_groups); > > > > +void igmp_group_set_protocol(const struct sbrec_igmp_group *group, > > + mcast_group_proto protocol); > > + > > #endif /* controller/ip-mcast.h */ > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > index 77bf67e58..6379b7afb 100644 > > --- a/controller/pinctrl.c > > +++ b/controller/pinctrl.c > > @@ -180,6 +180,7 @@ struct pinctrl { > > bool mac_binding_can_timestamp; > > bool fdb_can_timestamp; > > bool dns_supports_ovn_owned; > > + bool igmp_support_protocol; > > }; > > > > static struct pinctrl pinctrl; > > @@ -3586,11 +3587,21 @@ pinctrl_update(const struct ovsdb_idl *idl, > const char *br_int_name) > > if (dns_supports_ovn_owned != pinctrl.dns_supports_ovn_owned) { > > pinctrl.dns_supports_ovn_owned = dns_supports_ovn_owned; > > > > - /* Notify pinctrl_handler that fdb timestamp column > > + /* Notify pinctrl_handler that dns ovn_owned column > > * availability has changed. */ > > notify_pinctrl_handler(); > > } > > > > + bool igmp_support_proto = > > + sbrec_server_has_igmp_group_table_col_protocol(idl); > > + if (igmp_support_proto != pinctrl.igmp_support_protocol) { > > + pinctrl.igmp_support_protocol = igmp_support_proto; > > We only use this in the main thread, when updating the SB, why can't we > just directly check the column support there instead? > *like you mean to call sbrec_server_has_igmp_group_table_col_protocol(idl); inside the **ip_mcast_sync function?* *something like this:* * /* Set Group protocol*/ if (sbrec_server_has_igmp_group_table_col_protocol(idl)) { igmp_group_set_protocol(sbrec_igmp, mc_group->protocol_version); }* > > > + > > + /* Notify pinctrl_handler that igmp protocol column > > + * availability has changed. */ > > + notify_pinctrl_handler(); > > + } > > + > > ovs_mutex_unlock(&pinctrl_mutex); > > } > > > > @@ -5400,6 +5411,12 @@ ip_mcast_sync(struct ovsdb_idl_txn *ovnsb_idl_txn, > > local_dp->datapath, > chassis); > > } > > > > + /* Set Group protocol*/ > > + if (pinctrl.igmp_support_protocol) { > > + igmp_group_set_protocol(sbrec_igmp, > > + mc_group->protocol_version); > > + } > > + > > igmp_group_update_ports(sbrec_igmp, > sbrec_datapath_binding_by_key, > > sbrec_port_binding_by_key, > ip_ms->ms, > > mc_group); > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index f3868068d..700c9cf6e 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -120,7 +120,7 @@ static const char *rbac_svc_monitor_auth_update[] = > > static const char *rbac_igmp_group_auth[] = > > {""}; > > static const char *rbac_igmp_group_update[] = > > - {"address", "chassis", "datapath", "ports"}; > > + {"address", "protocol", "chassis", "datapath", "ports"}; > > static const char *rbac_bfd_auth[] = > > {""}; > > static const char *rbac_bfd_update[] = > > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema > > index 72e230b75..240d65f69 100644 > > --- a/ovn-sb.ovsschema > > +++ b/ovn-sb.ovsschema > > @@ -1,7 +1,7 @@ > > { > > "name": "OVN_Southbound", > > - "version": "20.30.0", > > - "cksum": "2972392849 31172", > > + "version": "20.31.0", > > + "cksum": "2643271413 31220", > > "tables": { > > "SB_Global": { > > "columns": { > > @@ -480,6 +480,7 @@ > > "IGMP_Group": { > > "columns": { > > "address": {"type": "string"}, > > + "protocol": {"type": "string"}, > > "datapath": {"type": {"key": {"type": "uuid", > > "refTable": > "Datapath_Binding", > > "refType": "weak"}, > > diff --git a/ovn-sb.xml b/ovn-sb.xml > > index e393f92b3..56e26b674 100644 > > --- a/ovn-sb.xml > > +++ b/ovn-sb.xml > > @@ -4756,6 +4756,10 @@ tcp.flags = RST; > > Destination IPv4 address for the IGMP group. > > </column> > > > > + <column name="protocol"> > > + Group protocol version either IGMPv1,v2,v3 or MLDv1,v2. > > + </column> > > + > > <column name="datapath"> > > Datapath to which this IGMP group belongs. > > </column> > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 8cc4c311c..180b9bfdd 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -22549,6 +22549,7 @@ send_igmp_v3_report hv2-vif1 hv2 000000000002 > $(ip_to_hex 10 0 0 2) f9f9 \ > > > > # Check that the IGMP Group is learned on both hv. > > wait_row_count IGMP_Group 2 address=239.0.1.68 > > +wait_row_count IGMP_Group 2 protocol=IGMPv3 > > check ovn-nbctl --wait=hv sync > > > > AT_CAPTURE_FILE([sbflows3]) > > @@ -22626,6 +22627,7 @@ send_igmp_v3_report hv1-vif1 hv1 \ > > > > # Check that the IGMP Group is learned. > > wait_row_count IGMP_Group 1 address=224.0.0.42 > > +wait_row_count IGMP_Group 1 protocol=IGMPv3 > > > > AS_BOX([IGMP traffic test 3]) > > # Send traffic and make sure it gets flooded to all ports. > > @@ -23287,6 +23289,7 @@ send_mld_v2_report hv2-vif1 hv2 \ > > > > # Check that the IP multicast group is learned on both hv. > > wait_row_count IGMP_Group 2 address='"ff0a:dead:beef::1"' > > +wait_row_count IGMP_Group 2 protocol=MLDv2 > > > > # This gives the ovn-controller nodes a chance to see the new > IGMP_Group. > > check ovn-nbctl --wait=hv sync > > Regards, > Dumitru > > Thanks, _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
