[ovs-dev] [PATCH ovn] ovn-controller: Don't clear the lflow resources ref during flow_output_run

[Han Zhou]

On Tue, Feb 18, 2020 at 7:32 AM <numans at ovn.org> wrote:
>
> From: Numan Siddique <numans at ovn.org>
>
> After the patch [1], which added caching of lflow expr, the
lflow_resource_ref
> is not rebuilt properly when lflow_run() is called. If a lflow is already
cached
> in lflow expr cache, then the lflow_resource_ref is not updated.
> But flow_output_run() clears the lflow_resource_ref cache before calling
lflow_run().
>
> This results in incorrect OF flows present in the switch even if the
> address set/port group references have changed.
>
> This patch fixes this issue by not clearing the lflow_resource_ref cache.
> This cache gets cleaned up during lflow change handler or in lflow_run().

Hi Numan,

This approach looks dangerous to me, since the lflow_resource_ref is not a
cache but part of the engine data. Originally, the life cycle of it follows
the same principle like other data of I-P engine, now if we change the
principle we need to be very careful.
At least one scenario would have problem. E.g. when there is a pending
transaction, we cannot run engine in that iteration, and we will trigger a
complete recompute next time, because the tracking data would be lost in
the next iteration. So it is not possible to call
lflow_resource_destroy_lflow() for deleted rows in that case because there
won't be any deleted flows tracked. It seems the commit 672508f6
(ovn-controller: Fix memory issues due to lflow expr caching.) would have
similar problem, too, in that case.

I am not sure if there would be other corner cases that would have issue
with this approach. Probably we can think more of it for making the data
required to build the lflow_resource_ref part of the expr cache.

Thanks,
Han
>
> [1] - 8795bec737b9("ovn-controller: Cache logical flow expr tree for each
lflow.")
> Fixes: 8795bec737b9("ovn-controller: Cache logical flow expr tree for
each lflow.")
>
> Signed-off-by: Numan Siddique <numans at ovn.org>
> ---
>  controller/lflow.c          | 3 +++
>  controller/ovn-controller.c | 2 --
>  tests/ovn.at                | 4 ++++
>  3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 79d89131a..072b2f1f1 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -924,6 +924,9 @@ lflow_run(struct lflow_ctx_in *l_ctx_in, struct
lflow_ctx_out *l_ctx_out)
>      SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow,
>
l_ctx_in->logical_flow_table) {

Use of FOR_EACH_TRACKED in recompute is not guaranteed to get tracked
changes.

>          if (sbrec_logical_flow_is_deleted(lflow)) {
> +            /* Delete entries from lflow resource reference. */
> +            lflow_resource_destroy_lflow(l_ctx_out->lfrr,
> +                                         &lflow->header_.uuid);
>              struct lflow_expr *le =
>                  lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow);
>              if (le) {
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 4d245ca28..b3be5c2db 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1441,7 +1441,6 @@ en_flow_output_run(struct engine_node *node, void
*data)
>      struct ovn_extend_table *group_table = &fo->group_table;
>      struct ovn_extend_table *meter_table = &fo->meter_table;
>      uint32_t *conj_id_ofs = &fo->conj_id_ofs;
> -    struct lflow_resource_ref *lfrr = &fo->lflow_resource_ref;
>
>      static bool first_run = true;
>      if (first_run) {
> @@ -1450,7 +1449,6 @@ en_flow_output_run(struct engine_node *node, void
*data)
>          ovn_desired_flow_table_clear(flow_table);
>          ovn_extend_table_clear(group_table, false /* desired */);
>          ovn_extend_table_clear(meter_table, false /* desired */);
> -        lflow_resource_clear(lfrr);
>      }
>
>      *conj_id_ofs = 1;
> diff --git a/tests/ovn.at b/tests/ovn.at
> index ea6760e1f..254645a3a 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -13778,6 +13778,10 @@ for i in 1 2 3; do
>      n_flows_after=`ovs-ofctl dump-flows br-int | grep "10.1.2.10" | wc
-l`
>      AT_CHECK([test $(expr $n_flows_before \* 2) = $n_flows_after], [0],
[ignore])
>
> +    # Trigger full recompute. Creating a chassis would trigger full
recompute.
> +    ovn-sbctl chassis-add tst geneve 127.0.0.4
> +    ovn-sbctl chassis-del tst
> +
>      # Remove an ACL
>      ovn-nbctl --wait=hv acl-del ls1 to-lport 200 \
>              'outport=="lp2" && ip4 && ip4.src == $as1'
> --
> 2.24.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev