[ovs-dev] [PATCH ovn 1/4] ovn-controller: Fix incremental processing for multicast group dependency.
Han Zhou
hzhou at ovn.org
Fri Jun 11 19:36:09 UTC 2021
On Fri, Jun 4, 2021 at 6:56 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 5/28/21 9:23 PM, Han Zhou wrote:
> > Multicast group changes are handled for physical flows in I-P, but not
> > handled for logical flows. Although logical flow doesn't care about the
> > content of a multicast group, the existance of it matters for logical
> > flow processing. If the multicast group is not found when the logical
> > flow is processed at first but laster it is created/monitored by
> > ovn-controller, the logical flow that has been processed before should
> > be reprocessed. This would avoid the workaound of ovn_lflow_add_unique()
> > used in northd when adding multicast group related logical flows.
> >
> > Signed-off-by: Han Zhou <hzhou at ovn.org>
> > ---
>
> The change looks mostly OK to me but I have a few minor comments.
Thanks for the review!
>
> > controller/lflow.c | 40 +++++++++++++++++++++++++++++++++++++
> > controller/lflow.h | 5 ++++-
> > controller/ovn-controller.c | 13 +++++++++++-
> > lib/ovn-util.h | 8 ++++++++
> > 4 files changed, 64 insertions(+), 2 deletions(-)
> >
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index 680b8cca1..0abb6eb96 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -55,6 +55,8 @@ struct lookup_port_aux {
> > struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath;
> > struct ovsdb_idl_index *sbrec_port_binding_by_name;
> > const struct sbrec_datapath_binding *dp;
> > + const struct sbrec_logical_flow *lflow;
> > + struct lflow_resource_ref *lfrr;
> > };
> >
> > struct condition_aux {
> > @@ -103,6 +105,16 @@ lookup_port_cb(const void *aux_, const char
*port_name, unsigned int *portp)
> > return true;
> > }
> >
> > + /* Store the key (DP + name) that used to lookup the multicast
group to
> > + * lflow reference, so that in the future when the multicast
group's
> > + * existance (found/not found) changes, the logical flow that
references
> > + * this multicast group can be reprocessed. */
> > + struct ds mg_key = DS_EMPTY_INITIALIZER;
> > + get_mc_group_key(port_name, aux->dp->tunnel_key, &mg_key);
> > + lflow_resource_add(aux->lfrr, REF_TYPE_MC_GROUP, ds_cstr(&mg_key),
> > + &aux->lflow->header_.uuid);
> > + ds_destroy(&mg_key);
> > +
> > const struct sbrec_multicast_group *mg = mcgroup_lookup_by_dp_name(
> > aux->sbrec_multicast_group_by_name_datapath, aux->dp,
port_name);
> > if (mg) {
> > @@ -570,6 +582,8 @@ add_matches_to_flow_table(const struct
sbrec_logical_flow *lflow,
> > = l_ctx_in->sbrec_multicast_group_by_name_datapath,
> > .sbrec_port_binding_by_name =
l_ctx_in->sbrec_port_binding_by_name,
> > .dp = dp,
> > + .lflow = lflow,
> > + .lfrr = l_ctx_out->lfrr,
> > };
> >
> > /* Encode OVN logical actions into OpenFlow. */
> > @@ -769,6 +783,8 @@ consider_logical_flow__(const struct
sbrec_logical_flow *lflow,
> > = l_ctx_in->sbrec_multicast_group_by_name_datapath,
> > .sbrec_port_binding_by_name =
l_ctx_in->sbrec_port_binding_by_name,
> > .dp = dp,
> > + .lflow = lflow,
> > + .lfrr = l_ctx_out->lfrr,
> > };
> > struct condition_aux cond_aux = {
> > .sbrec_port_binding_by_name =
l_ctx_in->sbrec_port_binding_by_name,
> > @@ -1745,6 +1761,30 @@ lflow_handle_flows_for_lport(const struct
sbrec_port_binding *pb,
> > l_ctx_in, l_ctx_out, &changed);
> > }
> >
> > +bool
> > +lflow_handle_mc_group_changes(struct lflow_ctx_in *l_ctx_in,
> > + struct lflow_ctx_out *l_ctx_out)
>
> For consistency with other functions that handle changes incrementally
> (e.g., lflow_handle_changed_neighbors(), lflow_handle_changed_flows()) I
> think we should rename this to lflow_handle_changed_mc_groups().
>
Yes, but lflow_handle_mc_group_changes() is more consistent with the
function name physical_handle_mc_group_changes(). I am ok either way, so I
updated it to lflow_handle_changed_mc_groups() in v2.
> > +{
> > + bool changed, ret = true;
>
> Nit: Personal preference, I'd define these on two different lines.
>
OK
> > + struct ds mg_key = DS_EMPTY_INITIALIZER;
> > + const struct sbrec_multicast_group *mg;
> > + SBREC_MULTICAST_GROUP_TABLE_FOR_EACH_TRACKED (mg,
> > +
l_ctx_in->mc_group_table) {
> > + get_mc_group_key(mg->name, mg->datapath->tunnel_key, &mg_key);
> > + if (!sbrec_multicast_group_is_new(mg)
> > + && !sbrec_multicast_group_is_deleted(mg)) {
> > + continue;
> > + }
> > + if (!lflow_handle_changed_ref(REF_TYPE_MC_GROUP,
ds_cstr(&mg_key),
> > + l_ctx_in, l_ctx_out, &changed)) {
> > + ret = false;
> > + break;
> > + }
> > + }
> > + ds_destroy(&mg_key);
> > + return ret;
> > +}
> > +
> > bool
> > lflow_handle_changed_lbs(struct lflow_ctx_in *l_ctx_in,
> > struct lflow_ctx_out *l_ctx_out)
> > diff --git a/controller/lflow.h b/controller/lflow.h
> > index 3c929d8a6..07263ff47 100644
> > --- a/controller/lflow.h
> > +++ b/controller/lflow.h
> > @@ -78,7 +78,8 @@ struct uuid;
> > enum ref_type {
> > REF_TYPE_ADDRSET,
> > REF_TYPE_PORTGROUP,
> > - REF_TYPE_PORTBINDING
> > + REF_TYPE_PORTBINDING,
> > + REF_TYPE_MC_GROUP
> > };
> >
> > struct ref_lflow_node {
> > @@ -179,4 +180,6 @@ bool lflow_add_flows_for_datapath(const struct
sbrec_datapath_binding *,
> > bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *,
> > struct lflow_ctx_in *,
> > struct lflow_ctx_out *);
> > +bool lflow_handle_mc_group_changes(struct lflow_ctx_in *,
> > + struct lflow_ctx_out *);
> > #endif /* controller/lflow.h */
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index d48ddc7a2..bf29aaaad 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -2272,6 +2272,13 @@ flow_output_sb_multicast_group_handler(struct
engine_node *node, void *data)
> > struct ed_type_flow_output *fo = data;
> > struct ovn_desired_flow_table *flow_table = &fo->flow_table;
> >
> > + struct lflow_ctx_in l_ctx_in;
> > + struct lflow_ctx_out l_ctx_out;
> > + init_lflow_ctx(node, rt_data, fo, &l_ctx_in, &l_ctx_out);
> > + if (!lflow_handle_mc_group_changes(&l_ctx_in, &l_ctx_out)) {
> > + return false;
> > + }
> > +
>
> Is there a specific reason why this block can't be after the one
> performing physical_handle_mc_group_changes() below? I think it doesn't
> change computation but it would then match the order of the I-P nodes in
> the DAG.
>
No specific reason at all. It is just natural for me to handle logical part
first and then physical. From the DAG point of view, it is handling the
same input (mc_group) at the same output node (flow_output), so it doesn't
tell anything about order.
> > struct physical_ctx p_ctx;
> > init_physical_ctx(node, rt_data, &p_ctx);
> >
> > @@ -2345,8 +2352,12 @@ _flow_output_resource_ref_handler(struct
engine_node *node, void *data,
> > deleted = &pg_data->deleted;
> > break;
> >
> > - /* This ref type is handled in the
flow_output_runtime_data_handler. */
> > case REF_TYPE_PORTBINDING:
> > + /* This ref type is handled in the
> > + * flow_output_runtime_data_handler. */
> > + case REF_TYPE_MC_GROUP:
> > + /* This ref type is handled in the
> > + * flow_output_sb_multicast_group_handler. */
> > default:
> > OVS_NOT_REACHED();
> > }
> > diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> > index 5533dbd0c..bf01668b5 100644
> > --- a/lib/ovn-util.h
> > +++ b/lib/ovn-util.h
> > @@ -138,6 +138,14 @@ get_unique_lport_key(uint64_t dp_tunnel_key,
uint64_t lport_tunnel_key,
> > lport_tunnel_key);
> > }
> >
> > +static inline void
> > +get_mc_group_key(const char *mg_name, int64_t dp_tunnel_key,
> > + struct ds *mg_key)
> > +{
> > + ds_clear(mg_key);
>
> Nit: get_sb_port_group_name() below assumes the caller has cleared the
> dynamic string, if needed. For consistency I think we should make both
> functions behave in the same way; either expect the caller to clear the
> string, or clear it themselves.
>
Agree. I noticed that, too, but just didn't want to introduce unrelated
changes (northd) to this patch. I added a new patch in the series to make
these helper functions more consistent.
> Related, but needs a different patch, we should probably change
> get_unique_lport_key() to use a dynamic string too.
>
Not sure if this is worth a change. I think it is ok for the function
get_unique_lport_key() to use char* because it's convenient for the caller
to use a fixed sized buffer e.g. char[16] on the stack, without the need to
free.
Thanks,
Han
> > + ds_put_format(mg_key, "%"PRId64"_%s", dp_tunnel_key, mg_name);
> > +}
> > +
> > static inline void
> > get_sb_port_group_name(const char *nb_pg_name, int64_t dp_tunnel_key,
> > struct ds *sb_pg_name)
> >
>
> Regards,
> Dumitru
>
More information about the dev
mailing list