On Mon, Jun 21, 2021 at 2:52 AM Han Zhou <hz...@ovn.org> wrote:
>
> 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>
Hi Han,
Thanks for fixing these issues. I've a few questions. I haven't
reviewed the patch completely.
> ---
> 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);
I'm not very clear why the variable "is_cr_cond_present" not required any more.
For logical flows with "is_chassis_resident" match condition, we cache
only the "expr" right ?
What is the behavior after this patch ?
> + 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)) {
This check - lflow_ref_lookup() is not very clear to me. Can you
please explain a bit about this ?
>From what I understand we cache the expr matches if the flow is not
referenced by resources.
Before this patch we were caching the 'expr matches' for a logical
flow like - 'ip4 && inport == "lp1"'
But now since there is a reference for this logical flow in the
lflow_ref table, we only cache the expr tree.
Is this intentional ? Correct me if my understanding is wrong.
> 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;
> +}
> +
One downside of adding the port binding handler is that we will process
a logical flow 'A' twice if a port binding is created and it is
claimed in the same loop
as the added test case does for the port - "lp2".
We do that first in lflow_handle_flows_for_lport() and then in
lflow_handle_changed_port_bindings().
I'm also not very clear why we need the port_binding handler here ?
Can you please give a scenario for this ?
The added test case passes for me even If I make the function
lflow_handle_changed_port_bindings()
return at the beginning without doing anything.
Thanks
Numan
> 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
>
> _______________________________________________
> 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
