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 <[email protected]>
> ---
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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
