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'
