On Wed, Jan 31, 2024 at 1:14 PM Mohammad Heib <mh...@redhat.com> 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 > <https://bugzilla.redhat.com/show_bug.cgi?id=2160476Signed-off-by>: > Mohammad Heib <mh...@redhat.com> > --- > Hi Mohammad. thank you for the patch, I have two small comments down below. > NEWS | 2 ++ > controller/ip-mcast.c | 10 ++++++++-- > controller/ip-mcast.h | 5 +++-- > controller/pinctrl.c | 28 ++++++++++++++++++++++++---- > northd/ovn-northd.c | 2 +- > ovn-sb.ovsschema | 5 +++-- > ovn-sb.xml | 4 ++++ > tests/ovn.at | 3 +++ > 8 files changed, 48 insertions(+), 11 deletions(-) > > diff --git a/NEWS b/NEWS > index 6553bd078..6505ef22b 100644 > --- a/NEWS > +++ b/NEWS > @@ -18,6 +18,8 @@ Post v23.09.0 > - Support selecting encapsulation IP based on the source/destination > VIF's > settting. See ovn-controller(8) 'external_ids:ovn-encap-ip' for more > details. > + - IGMP_Group has new "protocol" column that displays the the group > + protocol version. > > OVN v23.09.0 - 15 Sep 2023 > -------------------------- > diff --git a/controller/ip-mcast.c b/controller/ip-mcast.c > index a870fb29e..18a3e1fc2 100644 > --- a/controller/ip-mcast.c > +++ b/controller/ip-mcast.c > @@ -107,11 +107,12 @@ igmp_mrouter_create(struct ovsdb_idl_txn *idl_txn, > } > > void > -igmp_group_update_ports(const struct sbrec_igmp_group *g, > +igmp_group_update(const struct sbrec_igmp_group *g, > struct ovsdb_idl_index *datapaths, > struct ovsdb_idl_index *port_bindings, > const struct mcast_snooping *ms OVS_UNUSED, > - const struct mcast_group *mc_group) > + const struct mcast_group *mc_group, > + const char *protocol) > OVS_REQ_RDLOCK(ms->rwlock) > { > struct igmp_group_port *old_ports_storage = > @@ -151,6 +152,11 @@ igmp_group_update_ports(const struct sbrec_igmp_group > *g, > sbrec_igmp_group_update_ports_delvalue(g, igmp_port->port); > } > > + /* set Group protocol */ > + if (protocol) { > + sbrec_igmp_group_set_protocol(g, protocol); > + } > + > free(old_ports_storage); > hmap_destroy(&old_ports); > } > diff --git a/controller/ip-mcast.h b/controller/ip-mcast.h > index 326f39db1..2a8921976 100644 > --- a/controller/ip-mcast.h > +++ b/controller/ip-mcast.h > @@ -45,11 +45,12 @@ struct sbrec_igmp_group *igmp_mrouter_create( > const struct sbrec_datapath_binding *datapath, > const struct sbrec_chassis *chassis); > > -void igmp_group_update_ports(const struct sbrec_igmp_group *g, > +void igmp_group_update(const struct sbrec_igmp_group *g, > struct ovsdb_idl_index *datapaths, > struct ovsdb_idl_index *port_bindings, > const struct mcast_snooping *ms, > - const struct mcast_group *mc_group) > + const struct mcast_group *mc_group, > + const char *protocol) > OVS_REQ_RDLOCK(ms->rwlock); > void > igmp_mrouter_update_ports(const struct sbrec_igmp_group *g, > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index bd3bd3d81..f3597b489 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. */ > This change is nice, but not really related to the commit itself. I should be separate. > 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; > + > + /* Notify pinctrl_handler that igmp protocol column > + * availability has changed. */ > + notify_pinctrl_handler(); > + } > + > ovs_mutex_unlock(&pinctrl_mutex); > } > > @@ -5400,9 +5411,18 @@ ip_mcast_sync(struct ovsdb_idl_txn *ovnsb_idl_txn, > local_dp->datapath, > chassis); > } > > - igmp_group_update_ports(sbrec_igmp, > sbrec_datapath_binding_by_key, > - sbrec_port_binding_by_key, ip_ms->ms, > - mc_group); > + /* Set Group protocol if supported */ > + if (pinctrl.igmp_support_protocol) { > + igmp_group_update(sbrec_igmp, > sbrec_datapath_binding_by_key, > + sbrec_port_binding_by_key, ip_ms->ms, > + mc_group, > + mcast_snooping_group_protocol_str( > + mc_group->protocol_version)); > + } else { > + igmp_group_update(sbrec_igmp, > sbrec_datapath_binding_by_key, > + sbrec_port_binding_by_key, ip_ms->ms, > + mc_group, NULL); > + } > nit: We can avoid duplication if we pass pinctrl.igmp_support_protocol and extract the value there WDYT? > } > > /* Mrouters. */ > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index dadc1af38..5e593069f 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 28c6b6c34..c4e67e4f5 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 > -- > 2.34.3 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> amu...@redhat.com <https://red.ht/sig> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
