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 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()?
> 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?
> +
> + /* 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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
