[ovs-dev] [PATCH ovn] ovn-controller: Fix port group I-P when they contain non-vif ports.
Numan Siddique
numans at ovn.org
Fri Jun 25 14:36:41 UTC 2021
On Fri, Jun 25, 2021 at 7:51 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> It's valid that port_groups contain non-vif ports, they can actually
> contain any type of logical_switch_port.
>
> Also, there's no need to allocate a new sset containing the local ports'
> names every time the I-P engine processes a change, we can maintain a
> sset and incrementally update it when port bindings are added/removed.
>
> Reported-at: https://github.com/ovn-org/ovn/pull/61#issuecomment-865094163
> Reported-by: Antonio Ojea <aojea at redhat.com>
> Fixes: 0cfeba6b55e3 ("ovn-controller: Fix port group conjunction flow explosion problem.")
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
Hi Dumitru,
Thanks for the fix. I think it would be great to have a test case to
exercise the scenario.
I've one small nit comment. Otherwise the patch looks good to me.
Thanks
Numan
> ---
> controller/binding.c | 25 +++++--------------------
> controller/binding.h | 11 ++---------
> controller/ovn-controller.c | 24 +++++++-----------------
> 3 files changed, 14 insertions(+), 46 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 7fde0fdbb..1f6188e0d 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -549,6 +549,7 @@ update_local_lport_ids(const struct sbrec_port_binding *pb,
> tracked_binding_datapath_lport_add(pb, b_ctx->tracked_dp_bindings);
> }
> }
> + sset_add(&b_ctx->lbinding_data->port_bindings, pb->logical_port);
> }
>
> /* Remove a port binding id from the set of local lport IDs. Also track if
> @@ -569,6 +570,8 @@ remove_local_lport_ids(const struct sbrec_port_binding *pb,
> tracked_binding_datapath_lport_add(pb, b_ctx->tracked_dp_bindings);
> }
> }
> + sset_find_and_delete(&b_ctx->lbinding_data->port_bindings,
> + pb->logical_port);
> }
>
> /* Corresponds to each Port_Binding.type. */
> @@ -683,6 +686,7 @@ local_binding_data_init(struct local_binding_data *lbinding_data)
> {
> shash_init(&lbinding_data->bindings);
> shash_init(&lbinding_data->lports);
> + sset_init(&lbinding_data->port_bindings);
> }
>
> void
> @@ -702,6 +706,7 @@ local_binding_data_destroy(struct local_binding_data *lbinding_data)
> shash_delete(&lbinding_data->bindings, node);
> }
>
> + sset_destroy(&lbinding_data->port_bindings);
> shash_destroy(&lbinding_data->lports);
> shash_destroy(&lbinding_data->bindings);
> }
> @@ -2926,23 +2931,3 @@ cleanup:
>
> return b_lport;
> }
> -
> -struct sset *
> -binding_collect_local_binding_lports(struct local_binding_data *lbinding_data)
> -{
> - struct sset *lports = xzalloc(sizeof *lports);
> - sset_init(lports);
> - struct shash_node *shash_node;
> - SHASH_FOR_EACH (shash_node, &lbinding_data->lports) {
> - struct binding_lport *b_lport = shash_node->data;
> - sset_add(lports, b_lport->name);
> - }
> - return lports;
> -}
> -
> -void
> -binding_destroy_local_binding_lports(struct sset *lports)
> -{
> - sset_destroy(lports);
> - free(lports);
> -}
> diff --git a/controller/binding.h b/controller/binding.h
> index 8f3289476..7a35faa0d 100644
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -22,6 +22,7 @@
> #include "openvswitch/hmap.h"
> #include "openvswitch/uuid.h"
> #include "openvswitch/list.h"
> +#include "sset.h"
>
> struct hmap;
> struct ovsdb_idl;
> @@ -93,6 +94,7 @@ struct binding_ctx_out {
> struct local_binding_data {
> struct shash bindings;
> struct shash lports;
> + struct sset port_bindings;
I'd suggest changing the name of this variable to "lport_names". Although
sset would indicate that it represents a collection of port binding
names, "lport_names"
would be clear in my opinion.
> };
>
> void local_binding_data_init(struct local_binding_data *);
> @@ -133,13 +135,4 @@ bool binding_handle_port_binding_changes(struct binding_ctx_in *,
> void binding_tracked_dp_destroy(struct hmap *tracked_datapaths);
>
> void binding_dump_local_bindings(struct local_binding_data *, struct ds *);
> -
> -/* Generates a sset of lport names from local_binding_data.
> - * Note: the caller is responsible for destroying and freeing the returned
> - * sset, by calling binding_detroy_local_binding_lports(). */
> -struct sset *binding_collect_local_binding_lports(struct local_binding_data *);
> -
> -/* Destroy and free the lports sset returned by
> - * binding_collect_local_binding_lports(). */
> -void binding_destroy_local_binding_lports(struct sset *lports);
> #endif /* controller/binding.h */
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 3968ef059..5675b97dd 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1635,11 +1635,8 @@ en_port_groups_run(struct engine_node *node, void *data)
> struct ed_type_runtime_data *rt_data =
> engine_get_input_data("runtime_data", node);
>
> - struct sset *local_b_lports = binding_collect_local_binding_lports(
> - &rt_data->lbinding_data);
> - port_groups_init(pg_table, local_b_lports, &pg->port_group_ssets,
> - &pg->port_groups_cs_local);
> - binding_destroy_local_binding_lports(local_b_lports);
> + port_groups_init(pg_table, &rt_data->lbinding_data.port_bindings,
> + &pg->port_group_ssets, &pg->port_groups_cs_local);
>
> engine_set_node_state(node, EN_UPDATED);
> }
> @@ -1656,12 +1653,9 @@ port_groups_sb_port_group_handler(struct engine_node *node, void *data)
> struct ed_type_runtime_data *rt_data =
> engine_get_input_data("runtime_data", node);
>
> - struct sset *local_b_lports = binding_collect_local_binding_lports(
> - &rt_data->lbinding_data);
> - port_groups_update(pg_table, local_b_lports, &pg->port_group_ssets,
> - &pg->port_groups_cs_local, &pg->new, &pg->deleted,
> - &pg->updated);
> - binding_destroy_local_binding_lports(local_b_lports);
> + port_groups_update(pg_table, &rt_data->lbinding_data.port_bindings,
> + &pg->port_group_ssets, &pg->port_groups_cs_local,
> + &pg->new, &pg->deleted, &pg->updated);
>
> if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) ||
> !sset_is_empty(&pg->updated)) {
> @@ -1694,9 +1688,6 @@ port_groups_runtime_data_handler(struct engine_node *node, void *data)
> goto out;
> }
>
> - struct sset *local_b_lports = binding_collect_local_binding_lports(
> - &rt_data->lbinding_data);
> -
> const struct sbrec_port_group *pg_sb;
> SBREC_PORT_GROUP_TABLE_FOR_EACH (pg_sb, pg_table) {
> struct sset *pg_lports = shash_find_data(&pg->port_group_ssets,
> @@ -1723,13 +1714,12 @@ port_groups_runtime_data_handler(struct engine_node *node, void *data)
> if (need_update) {
> expr_const_sets_add_strings(&pg->port_groups_cs_local, pg_sb->name,
> (const char *const *) pg_sb->ports,
> - pg_sb->n_ports, local_b_lports);
> + pg_sb->n_ports,
> + &rt_data->lbinding_data.port_bindings);
> sset_add(&pg->updated, pg_sb->name);
> }
> }
>
> - binding_destroy_local_binding_lports(local_b_lports);
> -
> out:
> if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) ||
> !sset_is_empty(&pg->updated)) {
> --
> 2.27.0
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list