diff mbox series

[ovs-dev,v2,ovn] Save the addr set and port groups in lflow expr cache

Message ID 20200218195351.1223687-1-numans@ovn.org
State Accepted
Headers show
Series [ovs-dev,v2,ovn] Save the addr set and port groups in lflow expr cache | expand

Commit Message

Numan Siddique Feb. 18, 2020, 7:53 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 saving the addr set and port group sets in
the lfow expr cache and updating the lflow_resource_ref.

[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 | 63 ++++++++++++++++++++++++++++++++++------------
 tests/ovn.at       |  4 +++
 2 files changed, 51 insertions(+), 16 deletions(-)

Comments

Mark Michelson Feb. 18, 2020, 9:30 p.m. UTC | #1 
Hi, Numan. Would it be possible to add a test case that exercises the fix?

On 2/18/20 2:53 PM, 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 saving the addr set and port group sets in
> the lfow expr cache and updating the lflow_resource_ref.
> 
> [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 | 63 ++++++++++++++++++++++++++++++++++------------
>   tests/ovn.at       |  4 +++
>   2 files changed, 51 insertions(+), 16 deletions(-)
> 
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 79d89131a..110809df1 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -268,16 +268,21 @@ struct lflow_expr {
>       struct hmap_node node;
>       struct uuid lflow_uuid; /* key */
>       struct expr *expr;
> +    struct sset addr_sets_ref;
> +    struct sset port_groups_ref;
>   };
>   
>   static void
>   lflow_expr_add(struct hmap *lflow_expr_cache,
>                  const struct sbrec_logical_flow *lflow,
> -               struct expr *lflow_expr)
> +               struct expr *lflow_expr, struct sset *addr_sets_ref,
> +               struct sset *port_groups_ref)
>   {
>       struct lflow_expr *le = xmalloc(sizeof *le);
>       le->lflow_uuid = lflow->header_.uuid;
>       le->expr = lflow_expr;
> +    sset_clone(&le->addr_sets_ref, addr_sets_ref);
> +    sset_clone(&le->port_groups_ref, port_groups_ref);
>       hmap_insert(lflow_expr_cache, &le->node, uuid_hash(&le->lflow_uuid));
>   }
>   
> @@ -301,6 +306,8 @@ lflow_expr_delete(struct hmap *lflow_expr_cache, struct lflow_expr *le)
>   {
>       hmap_remove(lflow_expr_cache, &le->node);
>       expr_destroy(le->expr);
> +    sset_destroy(&le->addr_sets_ref);
> +    sset_destroy(&le->port_groups_ref);
>       free(le);
>   }
>   
> @@ -310,6 +317,8 @@ lflow_expr_destroy(struct hmap *lflow_expr_cache)
>       struct lflow_expr *le;
>       HMAP_FOR_EACH_POP (le, node, lflow_expr_cache) {
>           expr_destroy(le->expr);
> +        sset_destroy(&le->addr_sets_ref);
> +        sset_destroy(&le->port_groups_ref);
>           free(le);
>       }
>   
> @@ -548,6 +557,25 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t n_conjs)
>       return true;
>   }
>   
> +static void
> +lflow_update_resource_refs(const struct sbrec_logical_flow *lflow,
> +                           struct sset *addr_sets_ref,
> +                           struct sset *port_groups_ref,
> +                           struct lflow_resource_ref *lfrr)
> +{
> +    const char *addr_set_name;
> +    SSET_FOR_EACH (addr_set_name, addr_sets_ref) {
> +        lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name,
> +                           &lflow->header_.uuid);
> +    }
> +
> +    const char *port_group_name;
> +    SSET_FOR_EACH (port_group_name, port_groups_ref) {
> +        lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name,
> +                           &lflow->header_.uuid);
> +    }
> +}
> +
>   static bool
>   consider_logical_flow(const struct sbrec_logical_flow *lflow,
>                         struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
> @@ -615,33 +643,27 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
>       struct hmap matches;
>       struct expr *expr = NULL;
>   
> -    struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
> -    struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
>       struct lflow_expr *le = lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow);
>       if (le) {
>           if (delete_expr_from_cache) {
>               lflow_expr_delete(l_ctx_out->lflow_expr_cache, le);
> +            le = NULL;
>           } else {
>               expr = le->expr;
>           }
>       }
>   
>       if (!expr) {
> +        struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
> +        struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
> +
>           expr = expr_parse_string(lflow->match, &symtab, l_ctx_in->addr_sets,
>                                    l_ctx_in->port_groups, &addr_sets_ref,
>                                    &port_groups_ref, &error);
> -        const char *addr_set_name;
> -        SSET_FOR_EACH (addr_set_name, &addr_sets_ref) {
> -            lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET,
> -                               addr_set_name, &lflow->header_.uuid);
> -        }
> -        const char *port_group_name;
> -        SSET_FOR_EACH (port_group_name, &port_groups_ref) {
> -            lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTGROUP,
> -                               port_group_name, &lflow->header_.uuid);
> -        }
> -        sset_destroy(&addr_sets_ref);
> -        sset_destroy(&port_groups_ref);
> +        /* Add the address set and port groups (if any) to the lflow
> +         * references. */
> +        lflow_update_resource_refs(lflow, &addr_sets_ref, &port_groups_ref,
> +                                   l_ctx_out->lfrr);
>   
>           if (!error) {
>               if (prereqs) {
> @@ -658,15 +680,24 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
>               free(error);
>               ovnacts_free(ovnacts.data, ovnacts.size);
>               ofpbuf_uninit(&ovnacts);
> +            sset_destroy(&addr_sets_ref);
> +            sset_destroy(&port_groups_ref);
>               return true;
>           }
>   
>           expr = expr_simplify(expr);
>           expr = expr_normalize(expr);
>   
> -        lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr);
> +        lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr,
> +                       &addr_sets_ref, &port_groups_ref);
> +        sset_destroy(&addr_sets_ref);
> +        sset_destroy(&port_groups_ref);
>       } else {
>           expr_destroy(prereqs);
> +        /* Add the cached address set and port groups (if any) to the lflow
> +         * references. */
> +        lflow_update_resource_refs(lflow, &le->addr_sets_ref,
> +                                   &le->port_groups_ref, l_ctx_out->lfrr);
>       }
>   
>       struct condition_aux cond_aux = {
> 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'
>
Numan Siddique Feb. 19, 2020, 12:58 a.m. UTC | #2 
On Wed, Feb 19, 2020, 3:01 AM Mark Michelson <mmichels@redhat.com> wrote:

> Hi, Numan. Would it be possible to add a test case that exercises the fix?
>

Hi Mark,

The modified test case in this patch fails without the fix.

Numan


> On 2/18/20 2:53 PM, 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 saving the addr set and port group sets in
> > the lfow expr cache and updating the lflow_resource_ref.
> >
> > [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 | 63 ++++++++++++++++++++++++++++++++++------------
> >   tests/ovn.at       |  4 +++
> >   2 files changed, 51 insertions(+), 16 deletions(-)
> >
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index 79d89131a..110809df1 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -268,16 +268,21 @@ struct lflow_expr {
> >       struct hmap_node node;
> >       struct uuid lflow_uuid; /* key */
> >       struct expr *expr;
> > +    struct sset addr_sets_ref;
> > +    struct sset port_groups_ref;
> >   };
> >
> >   static void
> >   lflow_expr_add(struct hmap *lflow_expr_cache,
> >                  const struct sbrec_logical_flow *lflow,
> > -               struct expr *lflow_expr)
> > +               struct expr *lflow_expr, struct sset *addr_sets_ref,
> > +               struct sset *port_groups_ref)
> >   {
> >       struct lflow_expr *le = xmalloc(sizeof *le);
> >       le->lflow_uuid = lflow->header_.uuid;
> >       le->expr = lflow_expr;
> > +    sset_clone(&le->addr_sets_ref, addr_sets_ref);
> > +    sset_clone(&le->port_groups_ref, port_groups_ref);
> >       hmap_insert(lflow_expr_cache, &le->node,
> uuid_hash(&le->lflow_uuid));
> >   }
> >
> > @@ -301,6 +306,8 @@ lflow_expr_delete(struct hmap *lflow_expr_cache,
> struct lflow_expr *le)
> >   {
> >       hmap_remove(lflow_expr_cache, &le->node);
> >       expr_destroy(le->expr);
> > +    sset_destroy(&le->addr_sets_ref);
> > +    sset_destroy(&le->port_groups_ref);
> >       free(le);
> >   }
> >
> > @@ -310,6 +317,8 @@ lflow_expr_destroy(struct hmap *lflow_expr_cache)
> >       struct lflow_expr *le;
> >       HMAP_FOR_EACH_POP (le, node, lflow_expr_cache) {
> >           expr_destroy(le->expr);
> > +        sset_destroy(&le->addr_sets_ref);
> > +        sset_destroy(&le->port_groups_ref);
> >           free(le);
> >       }
> >
> > @@ -548,6 +557,25 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t
> n_conjs)
> >       return true;
> >   }
> >
> > +static void
> > +lflow_update_resource_refs(const struct sbrec_logical_flow *lflow,
> > +                           struct sset *addr_sets_ref,
> > +                           struct sset *port_groups_ref,
> > +                           struct lflow_resource_ref *lfrr)
> > +{
> > +    const char *addr_set_name;
> > +    SSET_FOR_EACH (addr_set_name, addr_sets_ref) {
> > +        lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name,
> > +                           &lflow->header_.uuid);
> > +    }
> > +
> > +    const char *port_group_name;
> > +    SSET_FOR_EACH (port_group_name, port_groups_ref) {
> > +        lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name,
> > +                           &lflow->header_.uuid);
> > +    }
> > +}
> > +
> >   static bool
> >   consider_logical_flow(const struct sbrec_logical_flow *lflow,
> >                         struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
> > @@ -615,33 +643,27 @@ consider_logical_flow(const struct
> sbrec_logical_flow *lflow,
> >       struct hmap matches;
> >       struct expr *expr = NULL;
> >
> > -    struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
> > -    struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
> >       struct lflow_expr *le =
> lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow);
> >       if (le) {
> >           if (delete_expr_from_cache) {
> >               lflow_expr_delete(l_ctx_out->lflow_expr_cache, le);
> > +            le = NULL;
> >           } else {
> >               expr = le->expr;
> >           }
> >       }
> >
> >       if (!expr) {
> > +        struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
> > +        struct sset port_groups_ref =
> SSET_INITIALIZER(&port_groups_ref);
> > +
> >           expr = expr_parse_string(lflow->match, &symtab,
> l_ctx_in->addr_sets,
> >                                    l_ctx_in->port_groups, &addr_sets_ref,
> >                                    &port_groups_ref, &error);
> > -        const char *addr_set_name;
> > -        SSET_FOR_EACH (addr_set_name, &addr_sets_ref) {
> > -            lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET,
> > -                               addr_set_name, &lflow->header_.uuid);
> > -        }
> > -        const char *port_group_name;
> > -        SSET_FOR_EACH (port_group_name, &port_groups_ref) {
> > -            lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTGROUP,
> > -                               port_group_name, &lflow->header_.uuid);
> > -        }
> > -        sset_destroy(&addr_sets_ref);
> > -        sset_destroy(&port_groups_ref);
> > +        /* Add the address set and port groups (if any) to the lflow
> > +         * references. */
> > +        lflow_update_resource_refs(lflow, &addr_sets_ref,
> &port_groups_ref,
> > +                                   l_ctx_out->lfrr);
> >
> >           if (!error) {
> >               if (prereqs) {
> > @@ -658,15 +680,24 @@ consider_logical_flow(const struct
> sbrec_logical_flow *lflow,
> >               free(error);
> >               ovnacts_free(ovnacts.data, ovnacts.size);
> >               ofpbuf_uninit(&ovnacts);
> > +            sset_destroy(&addr_sets_ref);
> > +            sset_destroy(&port_groups_ref);
> >               return true;
> >           }
> >
> >           expr = expr_simplify(expr);
> >           expr = expr_normalize(expr);
> >
> > -        lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr);
> > +        lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr,
> > +                       &addr_sets_ref, &port_groups_ref);
> > +        sset_destroy(&addr_sets_ref);
> > +        sset_destroy(&port_groups_ref);
> >       } else {
> >           expr_destroy(prereqs);
> > +        /* Add the cached address set and port groups (if any) to the
> lflow
> > +         * references. */
> > +        lflow_update_resource_refs(lflow, &le->addr_sets_ref,
> > +                                   &le->port_groups_ref,
> l_ctx_out->lfrr);
> >       }
> >
> >       struct condition_aux cond_aux = {
> > 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'
> >
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Han Zhou Feb. 19, 2020, 7:38 a.m. UTC | #3 
On Tue, Feb 18, 2020 at 11:54 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 saving the addr set and port group sets in
> the lfow expr cache and updating the lflow_resource_ref.
>
> [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 | 63 ++++++++++++++++++++++++++++++++++------------
>  tests/ovn.at       |  4 +++
>  2 files changed, 51 insertions(+), 16 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 79d89131a..110809df1 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -268,16 +268,21 @@ struct lflow_expr {
>      struct hmap_node node;
>      struct uuid lflow_uuid; /* key */
>      struct expr *expr;
> +    struct sset addr_sets_ref;
> +    struct sset port_groups_ref;
>  };
>
>  static void
>  lflow_expr_add(struct hmap *lflow_expr_cache,
>                 const struct sbrec_logical_flow *lflow,
> -               struct expr *lflow_expr)
> +               struct expr *lflow_expr, struct sset *addr_sets_ref,
> +               struct sset *port_groups_ref)
>  {
>      struct lflow_expr *le = xmalloc(sizeof *le);
>      le->lflow_uuid = lflow->header_.uuid;
>      le->expr = lflow_expr;
> +    sset_clone(&le->addr_sets_ref, addr_sets_ref);
> +    sset_clone(&le->port_groups_ref, port_groups_ref);
>      hmap_insert(lflow_expr_cache, &le->node, uuid_hash(&le->lflow_uuid));
>  }
>
> @@ -301,6 +306,8 @@ lflow_expr_delete(struct hmap *lflow_expr_cache,
struct lflow_expr *le)
>  {
>      hmap_remove(lflow_expr_cache, &le->node);
>      expr_destroy(le->expr);
> +    sset_destroy(&le->addr_sets_ref);
> +    sset_destroy(&le->port_groups_ref);
>      free(le);
>  }
>
> @@ -310,6 +317,8 @@ lflow_expr_destroy(struct hmap *lflow_expr_cache)
>      struct lflow_expr *le;
>      HMAP_FOR_EACH_POP (le, node, lflow_expr_cache) {
>          expr_destroy(le->expr);
> +        sset_destroy(&le->addr_sets_ref);
> +        sset_destroy(&le->port_groups_ref);
>          free(le);
>      }
>
> @@ -548,6 +557,25 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t
n_conjs)
>      return true;
>  }
>
> +static void
> +lflow_update_resource_refs(const struct sbrec_logical_flow *lflow,
> +                           struct sset *addr_sets_ref,
> +                           struct sset *port_groups_ref,
> +                           struct lflow_resource_ref *lfrr)
> +{
> +    const char *addr_set_name;
> +    SSET_FOR_EACH (addr_set_name, addr_sets_ref) {
> +        lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name,
> +                           &lflow->header_.uuid);
> +    }
> +
> +    const char *port_group_name;
> +    SSET_FOR_EACH (port_group_name, port_groups_ref) {
> +        lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name,
> +                           &lflow->header_.uuid);
> +    }
> +}
> +
>  static bool
>  consider_logical_flow(const struct sbrec_logical_flow *lflow,
>                        struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
> @@ -615,33 +643,27 @@ consider_logical_flow(const struct
sbrec_logical_flow *lflow,
>      struct hmap matches;
>      struct expr *expr = NULL;
>
> -    struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
> -    struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
>      struct lflow_expr *le = lflow_expr_get(l_ctx_out->lflow_expr_cache,
lflow);
>      if (le) {
>          if (delete_expr_from_cache) {
>              lflow_expr_delete(l_ctx_out->lflow_expr_cache, le);
> +            le = NULL;
>          } else {
>              expr = le->expr;
>          }
>      }
>
>      if (!expr) {
> +        struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
> +        struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
> +
>          expr = expr_parse_string(lflow->match, &symtab,
l_ctx_in->addr_sets,
>                                   l_ctx_in->port_groups, &addr_sets_ref,
>                                   &port_groups_ref, &error);
> -        const char *addr_set_name;
> -        SSET_FOR_EACH (addr_set_name, &addr_sets_ref) {
> -            lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET,
> -                               addr_set_name, &lflow->header_.uuid);
> -        }
> -        const char *port_group_name;
> -        SSET_FOR_EACH (port_group_name, &port_groups_ref) {
> -            lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTGROUP,
> -                               port_group_name, &lflow->header_.uuid);
> -        }
> -        sset_destroy(&addr_sets_ref);
> -        sset_destroy(&port_groups_ref);
> +        /* Add the address set and port groups (if any) to the lflow
> +         * references. */
> +        lflow_update_resource_refs(lflow, &addr_sets_ref,
&port_groups_ref,
> +                                   l_ctx_out->lfrr);
>
>          if (!error) {
>              if (prereqs) {
> @@ -658,15 +680,24 @@ consider_logical_flow(const struct
sbrec_logical_flow *lflow,
>              free(error);
>              ovnacts_free(ovnacts.data, ovnacts.size);
>              ofpbuf_uninit(&ovnacts);
> +            sset_destroy(&addr_sets_ref);
> +            sset_destroy(&port_groups_ref);
>              return true;
>          }
>
>          expr = expr_simplify(expr);
>          expr = expr_normalize(expr);
>
> -        lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr);
> +        lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr,
> +                       &addr_sets_ref, &port_groups_ref);
> +        sset_destroy(&addr_sets_ref);
> +        sset_destroy(&port_groups_ref);
>      } else {
>          expr_destroy(prereqs);
> +        /* Add the cached address set and port groups (if any) to the
lflow
> +         * references. */
> +        lflow_update_resource_refs(lflow, &le->addr_sets_ref,
> +                                   &le->port_groups_ref,
l_ctx_out->lfrr);
>      }
>
>      struct condition_aux cond_aux = {
> 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

Acked-by: Han Zhou <hzhou@ovn.org>
Numan Siddique Feb. 19, 2020, 11:29 a.m. UTC | #4 
On Wed, Feb 19, 2020 at 1:10 PM Han Zhou <hzhou@ovn.org> wrote:
>
> On Tue, Feb 18, 2020 at 11:54 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 saving the addr set and port group sets in
> > the lfow expr cache and updating the lflow_resource_ref.
> >
> > [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 | 63 ++++++++++++++++++++++++++++++++++------------
> >  tests/ovn.at       |  4 +++
> >  2 files changed, 51 insertions(+), 16 deletions(-)
> >
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index 79d89131a..110809df1 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -268,16 +268,21 @@ struct lflow_expr {
> >      struct hmap_node node;
> >      struct uuid lflow_uuid; /* key */
> >      struct expr *expr;
> > +    struct sset addr_sets_ref;
> > +    struct sset port_groups_ref;
> >  };
> >
> >  static void
> >  lflow_expr_add(struct hmap *lflow_expr_cache,
> >                 const struct sbrec_logical_flow *lflow,
> > -               struct expr *lflow_expr)
> > +               struct expr *lflow_expr, struct sset *addr_sets_ref,
> > +               struct sset *port_groups_ref)
> >  {
> >      struct lflow_expr *le = xmalloc(sizeof *le);
> >      le->lflow_uuid = lflow->header_.uuid;
> >      le->expr = lflow_expr;
> > +    sset_clone(&le->addr_sets_ref, addr_sets_ref);
> > +    sset_clone(&le->port_groups_ref, port_groups_ref);
> >      hmap_insert(lflow_expr_cache, &le->node, uuid_hash(&le->lflow_uuid));
> >  }
> >
> > @@ -301,6 +306,8 @@ lflow_expr_delete(struct hmap *lflow_expr_cache,
> struct lflow_expr *le)
> >  {
> >      hmap_remove(lflow_expr_cache, &le->node);
> >      expr_destroy(le->expr);
> > +    sset_destroy(&le->addr_sets_ref);
> > +    sset_destroy(&le->port_groups_ref);
> >      free(le);
> >  }
> >
> > @@ -310,6 +317,8 @@ lflow_expr_destroy(struct hmap *lflow_expr_cache)
> >      struct lflow_expr *le;
> >      HMAP_FOR_EACH_POP (le, node, lflow_expr_cache) {
> >          expr_destroy(le->expr);
> > +        sset_destroy(&le->addr_sets_ref);
> > +        sset_destroy(&le->port_groups_ref);
> >          free(le);
> >      }
> >
> > @@ -548,6 +557,25 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t
> n_conjs)
> >      return true;
> >  }
> >
> > +static void
> > +lflow_update_resource_refs(const struct sbrec_logical_flow *lflow,
> > +                           struct sset *addr_sets_ref,
> > +                           struct sset *port_groups_ref,
> > +                           struct lflow_resource_ref *lfrr)
> > +{
> > +    const char *addr_set_name;
> > +    SSET_FOR_EACH (addr_set_name, addr_sets_ref) {
> > +        lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name,
> > +                           &lflow->header_.uuid);
> > +    }
> > +
> > +    const char *port_group_name;
> > +    SSET_FOR_EACH (port_group_name, port_groups_ref) {
> > +        lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name,
> > +                           &lflow->header_.uuid);
> > +    }
> > +}
> > +
> >  static bool
> >  consider_logical_flow(const struct sbrec_logical_flow *lflow,
> >                        struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
> > @@ -615,33 +643,27 @@ consider_logical_flow(const struct
> sbrec_logical_flow *lflow,
> >      struct hmap matches;
> >      struct expr *expr = NULL;
> >
> > -    struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
> > -    struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
> >      struct lflow_expr *le = lflow_expr_get(l_ctx_out->lflow_expr_cache,
> lflow);
> >      if (le) {
> >          if (delete_expr_from_cache) {
> >              lflow_expr_delete(l_ctx_out->lflow_expr_cache, le);
> > +            le = NULL;
> >          } else {
> >              expr = le->expr;
> >          }
> >      }
> >
> >      if (!expr) {
> > +        struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
> > +        struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
> > +
> >          expr = expr_parse_string(lflow->match, &symtab,
> l_ctx_in->addr_sets,
> >                                   l_ctx_in->port_groups, &addr_sets_ref,
> >                                   &port_groups_ref, &error);
> > -        const char *addr_set_name;
> > -        SSET_FOR_EACH (addr_set_name, &addr_sets_ref) {
> > -            lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET,
> > -                               addr_set_name, &lflow->header_.uuid);
> > -        }
> > -        const char *port_group_name;
> > -        SSET_FOR_EACH (port_group_name, &port_groups_ref) {
> > -            lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTGROUP,
> > -                               port_group_name, &lflow->header_.uuid);
> > -        }
> > -        sset_destroy(&addr_sets_ref);
> > -        sset_destroy(&port_groups_ref);
> > +        /* Add the address set and port groups (if any) to the lflow
> > +         * references. */
> > +        lflow_update_resource_refs(lflow, &addr_sets_ref,
> &port_groups_ref,
> > +                                   l_ctx_out->lfrr);
> >
> >          if (!error) {
> >              if (prereqs) {
> > @@ -658,15 +680,24 @@ consider_logical_flow(const struct
> sbrec_logical_flow *lflow,
> >              free(error);
> >              ovnacts_free(ovnacts.data, ovnacts.size);
> >              ofpbuf_uninit(&ovnacts);
> > +            sset_destroy(&addr_sets_ref);
> > +            sset_destroy(&port_groups_ref);
> >              return true;
> >          }
> >
> >          expr = expr_simplify(expr);
> >          expr = expr_normalize(expr);
> >
> > -        lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr);
> > +        lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr,
> > +                       &addr_sets_ref, &port_groups_ref);
> > +        sset_destroy(&addr_sets_ref);
> > +        sset_destroy(&port_groups_ref);
> >      } else {
> >          expr_destroy(prereqs);
> > +        /* Add the cached address set and port groups (if any) to the
> lflow
> > +         * references. */
> > +        lflow_update_resource_refs(lflow, &le->addr_sets_ref,
> > +                                   &le->port_groups_ref,
> l_ctx_out->lfrr);
> >      }
> >
> >      struct condition_aux cond_aux = {
> > 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
>
> Acked-by: Han Zhou <hzhou@ovn.org>

Thanks for the review. I applied this patch to master and branch-20.03.

Thanks
Numan

> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/controller/lflow.c b/controller/lflow.c
index 79d89131a..110809df1 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -268,16 +268,21 @@  struct lflow_expr {
     struct hmap_node node;
     struct uuid lflow_uuid; /* key */
     struct expr *expr;
+    struct sset addr_sets_ref;
+    struct sset port_groups_ref;
 };
 
 static void
 lflow_expr_add(struct hmap *lflow_expr_cache,
                const struct sbrec_logical_flow *lflow,
-               struct expr *lflow_expr)
+               struct expr *lflow_expr, struct sset *addr_sets_ref,
+               struct sset *port_groups_ref)
 {
     struct lflow_expr *le = xmalloc(sizeof *le);
     le->lflow_uuid = lflow->header_.uuid;
     le->expr = lflow_expr;
+    sset_clone(&le->addr_sets_ref, addr_sets_ref);
+    sset_clone(&le->port_groups_ref, port_groups_ref);
     hmap_insert(lflow_expr_cache, &le->node, uuid_hash(&le->lflow_uuid));
 }
 
@@ -301,6 +306,8 @@  lflow_expr_delete(struct hmap *lflow_expr_cache, struct lflow_expr *le)
 {
     hmap_remove(lflow_expr_cache, &le->node);
     expr_destroy(le->expr);
+    sset_destroy(&le->addr_sets_ref);
+    sset_destroy(&le->port_groups_ref);
     free(le);
 }
 
@@ -310,6 +317,8 @@  lflow_expr_destroy(struct hmap *lflow_expr_cache)
     struct lflow_expr *le;
     HMAP_FOR_EACH_POP (le, node, lflow_expr_cache) {
         expr_destroy(le->expr);
+        sset_destroy(&le->addr_sets_ref);
+        sset_destroy(&le->port_groups_ref);
         free(le);
     }
 
@@ -548,6 +557,25 @@  update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t n_conjs)
     return true;
 }
 
+static void
+lflow_update_resource_refs(const struct sbrec_logical_flow *lflow,
+                           struct sset *addr_sets_ref,
+                           struct sset *port_groups_ref,
+                           struct lflow_resource_ref *lfrr)
+{
+    const char *addr_set_name;
+    SSET_FOR_EACH (addr_set_name, addr_sets_ref) {
+        lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name,
+                           &lflow->header_.uuid);
+    }
+
+    const char *port_group_name;
+    SSET_FOR_EACH (port_group_name, port_groups_ref) {
+        lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name,
+                           &lflow->header_.uuid);
+    }
+}
+
 static bool
 consider_logical_flow(const struct sbrec_logical_flow *lflow,
                       struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
@@ -615,33 +643,27 @@  consider_logical_flow(const struct sbrec_logical_flow *lflow,
     struct hmap matches;
     struct expr *expr = NULL;
 
-    struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
-    struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
     struct lflow_expr *le = lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow);
     if (le) {
         if (delete_expr_from_cache) {
             lflow_expr_delete(l_ctx_out->lflow_expr_cache, le);
+            le = NULL;
         } else {
             expr = le->expr;
         }
     }
 
     if (!expr) {
+        struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
+        struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
+
         expr = expr_parse_string(lflow->match, &symtab, l_ctx_in->addr_sets,
                                  l_ctx_in->port_groups, &addr_sets_ref,
                                  &port_groups_ref, &error);
-        const char *addr_set_name;
-        SSET_FOR_EACH (addr_set_name, &addr_sets_ref) {
-            lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET,
-                               addr_set_name, &lflow->header_.uuid);
-        }
-        const char *port_group_name;
-        SSET_FOR_EACH (port_group_name, &port_groups_ref) {
-            lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTGROUP,
-                               port_group_name, &lflow->header_.uuid);
-        }
-        sset_destroy(&addr_sets_ref);
-        sset_destroy(&port_groups_ref);
+        /* Add the address set and port groups (if any) to the lflow
+         * references. */
+        lflow_update_resource_refs(lflow, &addr_sets_ref, &port_groups_ref,
+                                   l_ctx_out->lfrr);
 
         if (!error) {
             if (prereqs) {
@@ -658,15 +680,24 @@  consider_logical_flow(const struct sbrec_logical_flow *lflow,
             free(error);
             ovnacts_free(ovnacts.data, ovnacts.size);
             ofpbuf_uninit(&ovnacts);
+            sset_destroy(&addr_sets_ref);
+            sset_destroy(&port_groups_ref);
             return true;
         }
 
         expr = expr_simplify(expr);
         expr = expr_normalize(expr);
 
-        lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr);
+        lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr,
+                       &addr_sets_ref, &port_groups_ref);
+        sset_destroy(&addr_sets_ref);
+        sset_destroy(&port_groups_ref);
     } else {
         expr_destroy(prereqs);
+        /* Add the cached address set and port groups (if any) to the lflow
+         * references. */
+        lflow_update_resource_refs(lflow, &le->addr_sets_ref,
+                                   &le->port_groups_ref, l_ctx_out->lfrr);
     }
 
     struct condition_aux cond_aux = {
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'