On Wed, Feb 19, 2020, 3:01 AM Mark Michelson <mmich...@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, num...@ovn.org wrote: > > From: Numan Siddique <num...@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 <num...@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 > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev