[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 21:37:54 UTC 2021


On Fri, Jun 25, 2021 at 2:53 PM Han Zhou <hzhou at ovn.org> wrote:
>
> 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;
>

I agree with the complexity and the naming confusion.

I think local_lports and local_lport_ids have been maintained in the
binding code
since a long time and lbinding_data was added recently.

I think there is a lot of redundant data which can be unified.


> According to the code (and also the bug the patch is trying to fix),
> lbinding_data is supposed to maintain VIFs only.

I agree.  "lbinding_data" is supposed to maintain local vif binding information.

> 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.

These comments would be super helpful.  But I think it is outside the
scope of this bug fix patch.  It's better if it's a separate patch.

> 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)?

I think local_lports can be used.  The side effect would be that we
will be allocating
the zone id even for patch ports.

Thanks
Numan

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


More information about the dev mailing list