On Tue, Feb 18, 2020 at 11:54 AM <[email protected]> wrote: > > From: Numan Siddique <[email protected]> > > 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 <[email protected]> > --- > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Acked-by: Han Zhou <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
