[ovs-dev,ovn] ovn-controller: Don't clear the lflow resources ref during flow_output_run
diff mbox series

Message ID 20200218153159.843124-1-numans@ovn.org
State Changes Requested
Headers show
Series
  • [ovs-dev,ovn] ovn-controller: Don't clear the lflow resources ref during flow_output_run
Related show

Commit Message

Numan Siddique Feb. 18, 2020, 3:31 p.m. UTC
From: Numan Siddique <numans@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().

[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@ovn.org>
---
 controller/lflow.c          | 3 +++
 controller/ovn-controller.c | 2 --
 tests/ovn.at                | 4 ++++
 3 files changed, 7 insertions(+), 2 deletions(-)

Comments

Han Zhou Feb. 18, 2020, 5:03 p.m. UTC | #1
On Tue, Feb 18, 2020 at 7:32 AM <numans@ovn.org> wrote:
>
> From: Numan Siddique <numans@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@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@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Numan Siddique Feb. 18, 2020, 5:27 p.m. UTC | #2
On Tue, Feb 18, 2020 at 10:34 PM Han Zhou <zhouhan@gmail.com> wrote:
>
> On Tue, Feb 18, 2020 at 7:32 AM <numans@ovn.org> wrote:
> >
> > From: Numan Siddique <numans@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 .

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@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@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Han Zhou Feb. 18, 2020, 6:19 p.m. UTC | #3
On Tue, Feb 18, 2020 at 9:27 AM Numan Siddique <numans@ovn.org> wrote:

> On Tue, Feb 18, 2020 at 10:34 PM Han Zhou <zhouhan@gmail.com> wrote:
> >
> > On Tue, Feb 18, 2020 at 7:32 AM <numans@ovn.org> wrote:
> > >
> > > From: Numan Siddique <numans@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.

>
> 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@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@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
Numan Siddique Feb. 18, 2020, 7:55 p.m. UTC | #4
On Tue, Feb 18, 2020 at 11:50 PM Han Zhou <zhouhan@gmail.com> wrote:
>
> On Tue, Feb 18, 2020 at 9:27 AM Numan Siddique <numans@ovn.org> wrote:
>
> > On Tue, Feb 18, 2020 at 10:34 PM Han Zhou <zhouhan@gmail.com> wrote:
> > >
> > > On Tue, Feb 18, 2020 at 7:32 AM <numans@ovn.org> wrote:
> > > >
> > > > From: Numan Siddique <numans@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@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@openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Han Zhou Feb. 19, 2020, 7:44 a.m. UTC | #5
On Tue, Feb 18, 2020 at 11:56 AM Numan Siddique <numans@ovn.org> wrote:
>
> On Tue, Feb 18, 2020 at 11:50 PM Han Zhou <zhouhan@gmail.com> wrote:
> >
> > On Tue, Feb 18, 2020 at 9:27 AM Numan Siddique <numans@ovn.org> wrote:
> >
> > > On Tue, Feb 18, 2020 at 10:34 PM Han Zhou <zhouhan@gmail.com> wrote:
> > > >
> > > > On Tue, Feb 18, 2020 at 7:32 AM <numans@ovn.org> wrote:
> > > > >
> > > > > From: Numan Siddique <numans@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
>

Hi Numan,

Thanks for the revision. I acked.

For the change-tracking problem I mentioned for commit 672508f6
(ovn-controller: Fix memory issues due to lflow expr caching.), do you plan
to address that as well? It would still have memory leak in some corner
cases, but the impact may be minor.

Thanks,
Han
Numan Siddique Feb. 19, 2020, 10:10 a.m. UTC | #6
On Wed, Feb 19, 2020 at 1:16 PM Han Zhou <zhouhan@gmail.com> wrote:
>
> On Tue, Feb 18, 2020 at 11:56 AM Numan Siddique <numans@ovn.org> wrote:
> >
> > On Tue, Feb 18, 2020 at 11:50 PM Han Zhou <zhouhan@gmail.com> wrote:
> > >
> > > On Tue, Feb 18, 2020 at 9:27 AM Numan Siddique <numans@ovn.org> wrote:
> > >
> > > > On Tue, Feb 18, 2020 at 10:34 PM Han Zhou <zhouhan@gmail.com> wrote:
> > > > >
> > > > > On Tue, Feb 18, 2020 at 7:32 AM <numans@ovn.org> wrote:
> > > > > >
> > > > > > From: Numan Siddique <numans@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
> >
>
> Hi Numan,
>
> Thanks for the revision. I acked.
>

Thanks for the review.

> For the change-tracking problem I mentioned for commit 672508f6
> (ovn-controller: Fix memory issues due to lflow expr caching.), do you plan
> to address that as well? It would still have memory leak in some corner
> cases, but the impact may be minor.

I wouldn't say memory leak, but it would be in the cache lingering around.
But I see the chances are less.

Lets say few logical flows are deleted. If
lflow_handle_changed_flows() handler is called,
it'll clear the expr cache for these lflows. If full recompute is
triggered, then lflow_run()
will clear the expr cache if the deleted logical flows are still in
the tracked buffer. If however,
for some reason (may be due to engine abort) the deleted lflows are not tracked,
the expr cache will not be cleared for these lflows.

One way to address this would be to periodically clear the expr cache
say every 'x' minutes.
What do you think of this approach ? This would be a straightforward
implementation.

Thanks
Numan

>
> Thanks,
> Han
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Han Zhou Feb. 19, 2020, 6:17 p.m. UTC | #7
On Wed, Feb 19, 2020 at 2:11 AM Numan Siddique <numans@ovn.org> wrote:
>
> On Wed, Feb 19, 2020 at 1:16 PM Han Zhou <zhouhan@gmail.com> wrote:
> >
> > On Tue, Feb 18, 2020 at 11:56 AM Numan Siddique <numans@ovn.org> wrote:
> > >
> > > On Tue, Feb 18, 2020 at 11:50 PM Han Zhou <zhouhan@gmail.com> wrote:
> > > >
> > > > On Tue, Feb 18, 2020 at 9:27 AM Numan Siddique <numans@ovn.org>
wrote:
> > > >
> > > > > On Tue, Feb 18, 2020 at 10:34 PM Han Zhou <zhouhan@gmail.com>
wrote:
> > > > > >
> > > > > > On Tue, Feb 18, 2020 at 7:32 AM <numans@ovn.org> wrote:
> > > > > > >
> > > > > > > From: Numan Siddique <numans@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
> > >
> >
> > Hi Numan,
> >
> > Thanks for the revision. I acked.
> >
>
> Thanks for the review.
>
> > For the change-tracking problem I mentioned for commit 672508f6
> > (ovn-controller: Fix memory issues due to lflow expr caching.), do you
plan
> > to address that as well? It would still have memory leak in some corner
> > cases, but the impact may be minor.
>
> I wouldn't say memory leak, but it would be in the cache lingering around.
> But I see the chances are less.
>
> Lets say few logical flows are deleted. If
> lflow_handle_changed_flows() handler is called,
> it'll clear the expr cache for these lflows. If full recompute is
> triggered, then lflow_run()
> will clear the expr cache if the deleted logical flows are still in
> the tracked buffer. If however,
> for some reason (may be due to engine abort) the deleted lflows are not
tracked,
> the expr cache will not be cleared for these lflows.
>
The common case for this to happen is when there is an update to SB from
this chassis, e.g. for binding a port or mac-binding update, when the
notification for the update comes, the ovnsb_idl_txn is NULL, so the round
of engine_run() may be skipped. If in the same DB update notification there
are also logical_flow deletions involved, the change tracking is lost and
the cache is left lingering.

With commit e2ab60e3a7 ("ovn-controller: Run I-P engine even when no SB txn
is available.") the chance becomes smaller. It happens only if in the same
iteration when ovnsb_idl_txn is NULL, another updates to SB is required,
e.g. another port-binding change or mac-binding change happened on this
chassis.

> One way to address this would be to periodically clear the expr cache
> say every 'x' minutes.
> What do you think of this approach ? This would be a straightforward
> implementation.
>

Given that the issue is not triggered frequently, the periodical cleanup
sounds like a good idea. It may be undesirable to completely flush the
cache, which can result in spike of CPU and latency every 'x' minutes.
Instead, would it be better to do "garbage collection" every 'x' minutes,
i.e. compare cache with all SB lflows, and delete only the non-existed
entries?

Thanks,
Han

Patch
diff mbox series

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) {
         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'