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 <hz...@ovn.org> --- v3 -> v4: Updated the test case to cover: - when the referenced lport is removed - when the referenced lport is remote 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 | 76 +++++++++++++++++++++++++++++++++++++ tests/test-ovn.c | 4 +- utilities/ovn-trace.c | 2 +- 8 files changed, 161 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 b6afb8fb9..b15ecbb5d 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 db1a0a35c..37192b6d4 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -27000,3 +27000,79 @@ OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [expected]) 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.122' allow-related + +# The first ACL should be programmed, but the second one shouldn't. +check ovn-nbctl --wait=hv sync +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.111], [0], [ignore]) +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.122], [1], [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.122], [0], [ignore]) + +# Remove the lport lp2 again, the OVS flow for the second ACL should be +# removed. +check ovn-nbctl --wait=hv lsp-del lp2 +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.122], [1], [ignore]) + +# Test similar scenario but when the referenced lport is not bound locally. + +# Create only lport lp3, but not lp4. +check ovn-nbctl lsp-add lsw0 lp3 \ + -- lsp-set-addresses lp3 "f0:00:00:00:00:03 10.0.0.33" + +ovn-appctl -t ovn-controller vlog/set file:dbg +check ovn-nbctl acl-add lsw0 to-lport 1002 'inport == "lp3" && ip4.dst == 10.0.0.133' allow-related +check ovn-nbctl acl-add lsw0 to-lport 1002 'inport == "lp4" && ip4.dst == 10.0.0.144' allow-related + +# The ACL for lp3 should be programmed, but the one for lp4 shouldn't. +check ovn-nbctl --wait=hv sync +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.133], [0], [ignore]) +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.144], [1], [ignore]) + +# Now create the lport lp4. +check ovn-nbctl lsp-add lsw0 lp4 \ + -- lsp-set-addresses lp4 "f0:00:00:00:00:04 10.0.0.44" + +# Now the ACL for lp4 should be programmed. +check ovn-nbctl --wait=hv sync +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.144], [0], [ignore]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP +]) diff --git a/tests/test-ovn.c b/tests/test-ovn.c index a4701b4cb..680cf06d6 100644 --- a/tests/test-ovn.c +++ b/tests/test-ovn.c @@ -319,7 +319,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); @@ -923,7 +923,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 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev