[ovs-dev] [PATCH ovn v3 3/3] ovn-controller: Fix incremental processing for logical port references.
Han Zhou
hzhou at ovn.org
Mon Jun 21 06:51:35 UTC 2021
If a lflow has an lport name in the match, but when the lflow is
processed the port-binding is not seen by ovn-controller, the
corresponding openflow will not be created. Later if the port-binding is
created/monitored by ovn-controller, the lflow is not reprocessed
because the lflow didn't change and ovn-controller doesn't know that the
port-binding affects the lflow. This patch fixes the problem by tracking
the references when parsing the lflow, even if the port-binding is not
found when the lflow is firstly parsed. A test case is also added to
cover the scenario.
Signed-off-by: Han Zhou <hzhou at ovn.org>
---
controller/lflow.c | 63 ++++++++++++++++++++++++++-----------
controller/lflow.h | 3 ++
controller/ovn-controller.c | 35 ++++++++++++++++-----
include/ovn/expr.h | 2 +-
lib/expr.c | 14 +++------
tests/ovn.at | 47 +++++++++++++++++++++++++++
tests/test-ovn.c | 4 +--
utilities/ovn-trace.c | 2 +-
8 files changed, 132 insertions(+), 38 deletions(-)
diff --git a/controller/lflow.c b/controller/lflow.c
index 34eca135a..b7699a309 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -61,6 +61,7 @@ struct lookup_port_aux {
struct condition_aux {
struct ovsdb_idl_index *sbrec_port_binding_by_name;
+ const struct sbrec_datapath_binding *dp;
const struct sbrec_chassis *chassis;
const struct sset *active_tunnels;
const struct sbrec_logical_flow *lflow;
@@ -98,6 +99,12 @@ lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp)
const struct lookup_port_aux *aux = aux_;
+ /* Store the name that used to lookup the lport to lflow reference, so that
+ * in the future when the lport's port binding changes, the logical flow
+ * that references this lport can be reprocessed. */
+ lflow_resource_add(aux->lfrr, REF_TYPE_PORTBINDING, port_name,
+ &aux->lflow->header_.uuid);
+
const struct sbrec_port_binding *pb
= lport_lookup_by_name(aux->sbrec_port_binding_by_name, port_name);
if (pb && pb->datapath == aux->dp) {
@@ -149,19 +156,18 @@ is_chassis_resident_cb(const void *c_aux_, const char *port_name)
{
const struct condition_aux *c_aux = c_aux_;
+ /* Store the port name that used to lookup the lport to lflow reference, so
+ * that in the future when the lport's port-binding changes the logical
+ * flow that references this lport can be reprocessed. */
+ lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, port_name,
+ &c_aux->lflow->header_.uuid);
+
const struct sbrec_port_binding *pb
= lport_lookup_by_name(c_aux->sbrec_port_binding_by_name, port_name);
if (!pb) {
return false;
}
- /* Store the port_name to lflow reference. */
- int64_t dp_id = pb->datapath->tunnel_key;
- char buf[16];
- get_unique_lport_key(dp_id, pb->tunnel_key, buf, sizeof(buf));
- lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, buf,
- &c_aux->lflow->header_.uuid);
-
if (strcmp(pb->type, "chassisredirect")) {
/* for non-chassisredirect ports */
return pb->chassis && pb->chassis == c_aux->chassis;
@@ -623,8 +629,6 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
int64_t dp_id = dp->tunnel_key;
char buf[16];
get_unique_lport_key(dp_id, port_id, buf, sizeof(buf));
- lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTBINDING, buf,
- &lflow->header_.uuid);
if (!sset_contains(l_ctx_in->local_lport_ids, buf)) {
VLOG_DBG("lflow "UUID_FMT
" port %s in match is not local, skip",
@@ -788,6 +792,7 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
};
struct condition_aux cond_aux = {
.sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name,
+ .dp = dp,
.chassis = l_ctx_in->chassis,
.active_tunnels = l_ctx_in->active_tunnels,
.lflow = lflow,
@@ -805,7 +810,6 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
struct hmap *matches = NULL;
size_t matches_size = 0;
- bool is_cr_cond_present = false;
bool pg_addr_set_ref = false;
uint32_t n_conjs = 0;
@@ -843,8 +847,8 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
case LCACHE_T_NONE:
case LCACHE_T_CONJ_ID:
case LCACHE_T_EXPR:
- expr = expr_evaluate_condition(expr, is_chassis_resident_cb, &cond_aux,
- &is_cr_cond_present);
+ expr = expr_evaluate_condition(expr, is_chassis_resident_cb,
+ &cond_aux);
expr = expr_normalize(expr);
break;
case LCACHE_T_MATCHES:
@@ -883,7 +887,9 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
/* Cache new entry if caching is enabled. */
if (lflow_cache_is_enabled(l_ctx_out->lflow_cache)) {
- if (cached_expr && !is_cr_cond_present) {
+ if (cached_expr
+ && !lflow_ref_lookup(&l_ctx_out->lfrr->lflow_ref_table,
+ &lflow->header_.uuid)) {
lflow_cache_add_matches(l_ctx_out->lflow_cache,
&lflow->header_.uuid, matches,
matches_size);
@@ -1746,21 +1752,42 @@ lflow_processing_end:
return handled;
}
+/* Handles a port-binding change that is possibly related to a lport's
+ * residence status on this chassis. */
bool
lflow_handle_flows_for_lport(const struct sbrec_port_binding *pb,
struct lflow_ctx_in *l_ctx_in,
struct lflow_ctx_out *l_ctx_out)
{
bool changed;
- int64_t dp_id = pb->datapath->tunnel_key;
- char pb_ref_name[16];
- get_unique_lport_key(dp_id, pb->tunnel_key, pb_ref_name,
- sizeof(pb_ref_name));
- return lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb_ref_name,
+ return lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb->logical_port,
l_ctx_in, l_ctx_out, &changed);
}
+/* Handles port-binding add/deletions. */
+bool
+lflow_handle_changed_port_bindings(struct lflow_ctx_in *l_ctx_in,
+ struct lflow_ctx_out *l_ctx_out)
+{
+ bool ret = true;
+ bool changed;
+ const struct sbrec_port_binding *pb;
+ SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
+ l_ctx_in->port_binding_table) {
+ if (!sbrec_port_binding_is_new(pb)
+ && !sbrec_port_binding_is_deleted(pb)) {
+ continue;
+ }
+ if (!lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb->logical_port,
+ l_ctx_in, l_ctx_out, &changed)) {
+ ret = false;
+ break;
+ }
+ }
+ return ret;
+}
+
bool
lflow_handle_changed_mc_groups(struct lflow_ctx_in *l_ctx_in,
struct lflow_ctx_out *l_ctx_out)
diff --git a/controller/lflow.h b/controller/lflow.h
index e98edf81d..9d8882ae5 100644
--- a/controller/lflow.h
+++ b/controller/lflow.h
@@ -130,6 +130,7 @@ struct lflow_ctx_in {
struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath;
struct ovsdb_idl_index *sbrec_logical_flow_by_logical_dp_group;
struct ovsdb_idl_index *sbrec_port_binding_by_name;
+ const struct sbrec_port_binding_table *port_binding_table;
const struct sbrec_dhcp_options_table *dhcp_options_table;
const struct sbrec_dhcpv6_options_table *dhcpv6_options_table;
const struct sbrec_datapath_binding_table *dp_binding_table;
@@ -182,4 +183,6 @@ bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *,
struct lflow_ctx_out *);
bool lflow_handle_changed_mc_groups(struct lflow_ctx_in *,
struct lflow_ctx_out *);
+bool lflow_handle_changed_port_bindings(struct lflow_ctx_in *,
+ struct lflow_ctx_out *);
#endif /* controller/lflow.h */
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index c28fd6f2d..8136afe5c 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -1966,6 +1966,10 @@ init_lflow_ctx(struct engine_node *node,
engine_get_input("SB_multicast_group", node),
"name_datapath");
+ struct sbrec_port_binding_table *port_binding_table =
+ (struct sbrec_port_binding_table *)EN_OVSDB_GET(
+ engine_get_input("SB_port_binding", node));
+
struct sbrec_dhcp_options_table *dhcp_table =
(struct sbrec_dhcp_options_table *)EN_OVSDB_GET(
engine_get_input("SB_dhcp_options", node));
@@ -2029,6 +2033,7 @@ init_lflow_ctx(struct engine_node *node,
l_ctx_in->sbrec_logical_flow_by_logical_dp_group =
sbrec_logical_flow_by_dp_group;
l_ctx_in->sbrec_port_binding_by_name = sbrec_port_binding_by_name;
+ l_ctx_in->port_binding_table = port_binding_table;
l_ctx_in->dhcp_options_table = dhcp_table;
l_ctx_in->dhcpv6_options_table = dhcpv6_table;
l_ctx_in->mac_binding_table = mac_binding_table;
@@ -2221,6 +2226,25 @@ lflow_output_sb_multicast_group_handler(struct engine_node *node, void *data)
return true;
}
+static bool
+lflow_output_sb_port_binding_handler(struct engine_node *node, void *data)
+{
+ struct ed_type_runtime_data *rt_data =
+ engine_get_input_data("runtime_data", node);
+
+ struct ed_type_lflow_output *lfo = data;
+
+ struct lflow_ctx_in l_ctx_in;
+ struct lflow_ctx_out l_ctx_out;
+ init_lflow_ctx(node, rt_data, lfo, &l_ctx_in, &l_ctx_out);
+ if (!lflow_handle_changed_port_bindings(&l_ctx_in, &l_ctx_out)) {
+ return false;
+ }
+
+ engine_set_node_state(node, EN_UPDATED);
+ return true;
+}
+
static bool
_lflow_output_resource_ref_handler(struct engine_node *node, void *data,
enum ref_type ref_type)
@@ -2285,8 +2309,9 @@ _lflow_output_resource_ref_handler(struct engine_node *node, void *data,
break;
case REF_TYPE_PORTBINDING:
- /* This ref type is handled in the
- * flow_output_runtime_data_handler. */
+ /* This ref type is handled in:
+ * - flow_output_runtime_data_handler
+ * - flow_output_sb_port_binding_handler. */
case REF_TYPE_MC_GROUP:
/* This ref type is handled in the
* flow_output_sb_multicast_group_handler. */
@@ -2895,12 +2920,8 @@ main(int argc, char *argv[])
engine_add_input(&en_lflow_output, &en_sb_chassis,
pflow_lflow_output_sb_chassis_handler);
- /* Any changes to the port binding, need not be handled
- * for lflow_outout engine. We still need sb_port_binding
- * as input to access the port binding data in lflow.c and
- * hence the noop handler. */
engine_add_input(&en_lflow_output, &en_sb_port_binding,
- engine_noop_handler);
+ lflow_output_sb_port_binding_handler);
engine_add_input(&en_lflow_output, &en_ovs_open_vswitch, NULL);
engine_add_input(&en_lflow_output, &en_ovs_bridge, NULL);
diff --git a/include/ovn/expr.h b/include/ovn/expr.h
index de90e87e1..3b5653f7b 100644
--- a/include/ovn/expr.h
+++ b/include/ovn/expr.h
@@ -438,7 +438,7 @@ struct expr *expr_evaluate_condition(
struct expr *,
bool (*is_chassis_resident)(const void *c_aux,
const char *port_name),
- const void *c_aux, bool *condition_present);
+ const void *c_aux);
struct expr *expr_normalize(struct expr *);
bool expr_honors_invariants(const struct expr *);
diff --git a/lib/expr.c b/lib/expr.c
index f728f9537..e3f6bb892 100644
--- a/lib/expr.c
+++ b/lib/expr.c
@@ -2121,16 +2121,13 @@ static struct expr *
expr_evaluate_condition__(struct expr *expr,
bool (*is_chassis_resident)(const void *c_aux,
const char *port_name),
- const void *c_aux, bool *condition_present)
+ const void *c_aux)
{
bool result;
switch (expr->cond.type) {
case EXPR_COND_CHASSIS_RESIDENT:
result = is_chassis_resident(c_aux, expr->cond.string);
- if (condition_present != NULL) {
- *condition_present = true;
- }
break;
default:
@@ -2146,7 +2143,7 @@ struct expr *
expr_evaluate_condition(struct expr *expr,
bool (*is_chassis_resident)(const void *c_aux,
const char *port_name),
- const void *c_aux, bool *condition_present)
+ const void *c_aux)
{
struct expr *sub, *next;
@@ -2156,15 +2153,14 @@ expr_evaluate_condition(struct expr *expr,
LIST_FOR_EACH_SAFE (sub, next, node, &expr->andor) {
ovs_list_remove(&sub->node);
struct expr *e = expr_evaluate_condition(sub, is_chassis_resident,
- c_aux, condition_present);
+ c_aux);
e = expr_fix(e);
expr_insert_andor(expr, next, e);
}
return expr_fix(expr);
case EXPR_T_CONDITION:
- return expr_evaluate_condition__(expr, is_chassis_resident, c_aux,
- condition_present);
+ return expr_evaluate_condition__(expr, is_chassis_resident, c_aux);
case EXPR_T_CMP:
case EXPR_T_BOOLEAN:
@@ -3517,7 +3513,7 @@ expr_parse_microflow__(struct lexer *lexer,
e = expr_simplify(e);
e = expr_evaluate_condition(e, microflow_is_chassis_resident_cb,
- NULL, NULL);
+ NULL);
e = expr_normalize(e);
struct match m = MATCH_CATCHALL_INITIALIZER;
diff --git a/tests/ovn.at b/tests/ovn.at
index bc494fcad..e6d609b5b 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -26782,3 +26782,50 @@ AT_CHECK([ovs-ofctl dump-flows br-int "nw_src=10.0.0.0/24" | \
OVN_CLEANUP([hv1])
AT_CLEANUP
])
+
+# Test when a logical port name is used in an ACL before it is created. When it
+# is created later, the ACL should be programmed as openflow rules by
+# ovn-controller. Although this is not likely to happen in real world use
+# cases, it is possible that a port-binding is observed by ovn-controller AFTER
+# an lflow that references the port is processed. So this test case is to make
+# sure the incremental processing in ovn-controller reprocesses the lflow when
+# the port-binding is observed.
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn -- ACL referencing lport before creation])
+ovn_start
+
+net_add n1
+
+sim_add hv1
+as hv1
+check ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+
+# Bind both lp1 and lp2 on the chasis.
+check ovs-vsctl add-port br-int lp1 -- set interface lp1 external_ids:iface-id=lp1
+check ovs-vsctl add-port br-int lp2 -- set interface lp2 external_ids:iface-id=lp2
+
+# Create only lport lp1, but not lp2.
+check ovn-nbctl ls-add lsw0
+check ovn-nbctl lsp-add lsw0 lp1 \
+ -- lsp-set-addresses lp1 "f0:00:00:00:00:01 10.0.0.11"
+
+# Each lport is referenced by an ACL.
+check ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp1" && ip4.src == 10.0.0.111' allow-related
+check ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp2" && ip4.src == 10.0.0.222' allow-related
+
+# The first ACL should be programmed.
+check ovn-nbctl --wait=hv sync
+AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.111], [0], [ignore])
+
+# Now create the lport lp2.
+check ovn-nbctl lsp-add lsw0 lp2 \
+ -- lsp-set-addresses lp2 "f0:00:00:00:00:02 10.0.0.22"
+
+check ovn-nbctl --wait=hv sync
+# Now the second ACL should be programmed.
+AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.222], [0], [ignore])
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index 98cc2c503..c6d8b287b 100644
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -318,7 +318,7 @@ test_parse_expr__(int steps)
if (steps > 1) {
expr = expr_simplify(expr);
expr = expr_evaluate_condition(expr, is_chassis_resident_cb,
- &ports, NULL);
+ &ports);
}
if (steps > 2) {
expr = expr_normalize(expr);
@@ -922,7 +922,7 @@ test_tree_shape_exhaustively(struct expr *expr, struct shash *symtab,
modified = expr_simplify(expr_clone(expr));
modified = expr_evaluate_condition(
modified, tree_shape_is_chassis_resident_cb,
- NULL, NULL);
+ NULL);
ovs_assert(expr_honors_invariants(modified));
if (operation >= OP_NORMALIZE) {
diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
index 49463c5c2..5d016b757 100644
--- a/utilities/ovn-trace.c
+++ b/utilities/ovn-trace.c
@@ -973,7 +973,7 @@ parse_lflow_for_datapath(const struct sbrec_logical_flow *sblf,
match = expr_simplify(match);
match = expr_evaluate_condition(match,
ovntrace_is_chassis_resident,
- NULL, NULL);
+ NULL);
}
struct ovntrace_flow *flow = xzalloc(sizeof *flow);
--
2.30.2
More information about the dev
mailing list