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

Han Zhou hzhou at ovn.org
Mon Jun 28 06:05:13 UTC 2021


On Fri, Jun 25, 2021 at 2:38 PM Numan Siddique <numans at ovn.org> wrote:
>
> 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.

Agree. And I just noticed that the comments for local_lport_ids in
ovn-controller.c is not correct any more (probably since very long time
ago):
    /* Contains the same ports as local_lports, but in the format:

     * <datapath-tunnel-key>_<port-tunnel-key> */

    struct sset local_lport_ids;

It in fact contains more lports than local_lports, including patch ports,
and they are used for different purposes. I think we should rename them.

>
> > 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. I checked the code again and realized that local_lports
doesn't contain patch ports, so it cannot be used for the port-group I-P
directly. I think you meant that it can be reused if we add patch ports to
it, but then zone id allocation behavior will be affected. In my opinion
we'd better not do so because they serve different purposes. It looks that
what is required by port-group processing is close to what we have for
local_lport_ids, but just names instead of tunnel keys, so we may combine
with local_lport_ids as a struct, e.g.:

struct related_lports {
    struct sset lport_names;
    struct sset lport_ids; /* the current local_lport_ids */
}

"related_lports" represents lports that are related to this chassis,
including patch ports, to distinguish from the "local_lports". Maybe the
name is still not good, but I just couldn't think of a better one.
The functions update_local_lport_ids()/remove_local_lport_ids() are indeed
the right place to update it (except that we should rename them).

Thanks,
Han

> 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