[ovs-dev] [PATCH ovn 1/4] ovn-controller: Fix incremental processing for multicast group dependency.

Dumitru Ceara dceara at redhat.com
Fri Jun 4 13:56:19 UTC 2021


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.

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

> +{
> +    bool changed, ret = true;

Nit: Personal preference, I'd define these on two different lines.

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

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

Related, but needs a different patch, we should probably change
get_unique_lport_key() to use a dynamic string too.

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