[ovs-dev] [PATCH ovn] ovn-controller: Don't clear the lflow resources ref during flow_output_run
Numan Siddique
numans at ovn.org
Tue Feb 18 19:55:53 UTC 2020
On Tue, Feb 18, 2020 at 11:50 PM Han Zhou <zhouhan at gmail.com> wrote:
>
> On Tue, Feb 18, 2020 at 9:27 AM Numan Siddique <numans at ovn.org> wrote:
>
> > On Tue, Feb 18, 2020 at 10:34 PM Han Zhou <zhouhan at gmail.com> wrote:
> > >
> > > 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.
> >
> > Hi Han,
> >
> > I'll take a closer look at your comments and come back tomorrow.
> > Is it wise to disable or revert lflow expr caching commit ? And then
> > address all these
> > issues properly ? SInce we are close to 20.03 release .
> >
>
> Yes, reverting for 20.03 sounds good to me. We can always back port later
> when it’s mature enough.
Hi Han,
I've submitted v2 by taking a different approach as suggested by you. v2 will
now store the addr set and port group name sets as part of lflow expr cache
and add those to the lflow resource ref.
Kindly take a look. I think this should work now without any issues :).
Thanks
Numan
> >
> > Thanks
> > Numan
> >
> > >
> > > 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
> > > _______________________________________________
> > > dev mailing list
> > > dev at openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> >
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list