[ovs-dev] [PATCH ovn] ovn-controller: Fix port group I-P when they contain non-vif ports.

Han Zhou hzhou at ovn.org
Fri Jun 25 18:52:52 UTC 2021


On Fri, Jun 25, 2021 at 4:50 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.
>
Thanks Dumitru for the fix and thanks Numan for the review.

I battled with myself when deciding to allocate a new sset for the local
ports' names at the I-P main iteration level. I did this because the
current data structures maintaining the local bindings were already very
complex, and the sset was not to maintain any extra information but just
redundant information (for efficiency). So I decided to abstract this part
as a helper function so that I don't add more complexity in the binding
data structure, and other I-P engine nodes doesn't need to understand the
internals of how the bindings are maintained in the bindings module.
Regarding the cost, the local binding data should be small, and the sset
allocation is at the main loop level, so really nothing to worry about the
cost.

However, I didn't think about the non-VIF use case of port-group, and the
local_binding doesn't maintain non-VIF bindings, so came the bug. This
patch fixes it by maintaining a sset that includes all types of lport
names. It looks correct to me, but I have some comments:

1) The structures in bindings module is really complex and I spent a lot of
time to understand it before, but when I am reviewing this patch I had to
spent some time again to understand it. There are fields in binding_ctx
look quite similar, and the comments don't tell exactly the difference:

    - struct local_binding_data *lbinding_data;
    - struct sset *local_lports;
    - struct sset *local_lport_ids;

According to the code (and also the bug the patch is trying to fix),
lbinding_data is supposed to maintain VIFs only.
local_lports maintains all types, but it maintains *potentially* local VIFs
as well (meaning the VIF may not be bound locally yet). I was thinking if I
could use local_lports directly. I think it would work, but just not
accurate enough (maybe it doesn't really matter).
The local_lport_ids may look straightforward, which maintains generated id
keys for local_lports, but the functions update_local_lport_ids() and
remove_local_lport_ids() are not only updating the local_lport_ids, but
also tracking information of lbinding_data, which is really confusing.

2) Now for this patch, the intention is to include non-VIF bindings, but it
adds a sset to maintain all types of lports in "lbinding_data", which was
supposed to maintain VIF bindings only. I think it is not the right place
to maintain this sset. And the
update_local_lport_ids()/remove_local_lport_ids() are not the right place
to update them either.

So here are my suggestions:

1) Clarify a little more about the role of each of the above fields in
binding_ctx with some comments.
2) Can we use local_lports directly, instead of maintaining a new sset? If
we can't (I am not sure yet), can we generate it on-the-fly, just updating
the "binding_collect_local_binding_lports" by adding non-VIF lports from
"local_lports"? I really don't think the cost makes any difference overall.
If none of the above work, can we maintain it as a separate field at
binding_ctx level instead of in lbinding_data (with proper comment telling
the difference from local_lports)?

(+1 for Numan's comment regarding test case)

I hope this makes sense. Thanks again for the fix!
Han

> 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>
> ---
>  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;
>  };
>
>  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
>


More information about the dev mailing list