[ovs-dev] [PATCH ovn v3 09/10] This patch processes the logical flows of tracked datapaths and tracked logical ports. To handle the tracked logical port changes, reference of logical flows to port bindings is maintained.
Numan Siddique
numans at ovn.org
Sat Apr 18 06:58:55 UTC 2020
On Fri, Apr 17, 2020 at 9:03 PM <numans at ovn.org> wrote:
>
> From: Venkata Anil <anilvenkata at redhat.com>
>
> Co-Authored-by: Numan Siddique <numans at ovn.org>
> Signed-off-by: Venkata Anil <anilvenkata at redhat.com>
> Signed-off-by: Numan Siddique <numans at ovn.org>
The patch comment is messed up with the commit subject. Request to
ignore this issue for now.
I'll correct this in v4 after I get some review comments.
Thanks
Numan
> ---
> controller/lflow.c | 69 ++++++++++++++++++++-
> controller/lflow.h | 12 +++-
> controller/ovn-controller.c | 116 +++++++++++++++++++-----------------
> controller/physical.h | 3 +-
> tests/ovn-performance.at | 8 +--
> 5 files changed, 146 insertions(+), 62 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 512258cd3..45bf4aa4b 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -258,7 +258,7 @@ lflow_resource_destroy_lflow(struct lflow_resource_ref *lfrr,
> /* Adds the logical flows from the Logical_Flow table to flow tables. */
> static void
> add_logical_flows(struct lflow_ctx_in *l_ctx_in,
> - struct lflow_ctx_out *l_ctx_out)
> + struct lflow_ctx_out *l_ctx_out)
> {
> const struct sbrec_logical_flow *lflow;
>
> @@ -649,6 +649,8 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
> int64_t dp_id = lflow->logical_datapath->tunnel_key;
> char buf[16];
> snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, dp_id, port_id);
> + 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",
> @@ -860,3 +862,68 @@ lflow_evaluate_pb_changes(const struct sbrec_port_binding_table *pb_table)
>
> return true;
> }
> +
> +bool
> +lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp,
> + struct lflow_ctx_in *l_ctx_in,
> + struct lflow_ctx_out *l_ctx_out)
> +{
> + bool handled = true;
> + struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
> + struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts);
> + const struct sbrec_dhcp_options *dhcp_opt_row;
> + SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row,
> + l_ctx_in->dhcp_options_table) {
> + dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code,
> + dhcp_opt_row->type);
> + }
> +
> +
> + const struct sbrec_dhcpv6_options *dhcpv6_opt_row;
> + SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row,
> + l_ctx_in->dhcpv6_options_table) {
> + dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, dhcpv6_opt_row->code,
> + dhcpv6_opt_row->type);
> + }
> +
> + struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts);
> + nd_ra_opts_init(&nd_ra_opts);
> +
> + struct controller_event_options controller_event_opts;
> + controller_event_opts_init(&controller_event_opts);
> +
> + struct sbrec_logical_flow *lf_row = sbrec_logical_flow_index_init_row(
> + l_ctx_in->sbrec_logical_flow_by_logical_datapath);
> + sbrec_logical_flow_index_set_logical_datapath(lf_row, dp);
> +
> + const struct sbrec_logical_flow *lflow;
> + SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL (
> + lflow, lf_row, l_ctx_in->sbrec_logical_flow_by_logical_datapath) {
> + if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> + &nd_ra_opts, &controller_event_opts,
> + l_ctx_in, l_ctx_out)) {
> + handled = false;
> + break;
> + }
> + }
> +
> + dhcp_opts_destroy(&dhcp_opts);
> + dhcp_opts_destroy(&dhcpv6_opts);
> + nd_ra_opts_destroy(&nd_ra_opts);
> + controller_event_opts_destroy(&controller_event_opts);
> + return handled;
> +}
> +
> +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)
> +{
> + int64_t dp_id = pb->datapath->tunnel_key;
> + char pb_ref_name[16];
> + snprintf(pb_ref_name, sizeof(pb_ref_name), "%"PRId64"_%"PRId64,
> + dp_id, pb->tunnel_key);
> + bool changed = true;
> + return lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb_ref_name,
> + l_ctx_in, l_ctx_out, &changed);
> +}
> diff --git a/controller/lflow.h b/controller/lflow.h
> index fa54d4de2..53ecc45b4 100644
> --- a/controller/lflow.h
> +++ b/controller/lflow.h
> @@ -49,6 +49,8 @@ struct sbrec_dhcpv6_options_table;
> struct sbrec_logical_flow_table;
> struct sbrec_mac_binding_table;
> struct sbrec_port_binding_table;
> +struct sbrec_datapath_binding;
> +struct sbrec_port_binding;
> struct simap;
> struct sset;
> struct uuid;
> @@ -73,7 +75,8 @@ struct uuid;
>
> enum ref_type {
> REF_TYPE_ADDRSET,
> - REF_TYPE_PORTGROUP
> + REF_TYPE_PORTGROUP,
> + REF_TYPE_PORTBINDING
> };
>
> /* Maintains the relationship for a pair of named resource and
> @@ -118,6 +121,7 @@ void lflow_resource_clear(struct lflow_resource_ref *);
>
> struct lflow_ctx_in {
> struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath;
> + struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath;
> struct ovsdb_idl_index *sbrec_port_binding_by_name;
> const struct sbrec_dhcp_options_table *dhcp_options_table;
> const struct sbrec_dhcpv6_options_table *dhcpv6_options_table;
> @@ -159,4 +163,10 @@ void lflow_destroy(void);
>
> void lflow_expr_destroy(struct hmap *lflow_expr_cache);
>
> +bool lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *,
> + struct lflow_ctx_in *,
> + struct lflow_ctx_out *);
> +bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *,
> + 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 d56131d9c..ea21bbe10 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1438,6 +1438,11 @@ static void init_lflow_ctx(struct engine_node *node,
> engine_get_input("SB_port_binding", node),
> "name");
>
> + struct ovsdb_idl_index *sbrec_logical_flow_by_dp =
> + engine_ovsdb_node_get_index(
> + engine_get_input("SB_logical_flow", node),
> + "logical_datapath");
> +
> struct ovsdb_idl_index *sbrec_mc_group_by_name_dp =
> engine_ovsdb_node_get_index(
> engine_get_input("SB_multicast_group", node),
> @@ -1485,6 +1490,8 @@ static void init_lflow_ctx(struct engine_node *node,
>
> l_ctx_in->sbrec_multicast_group_by_name_datapath =
> sbrec_mc_group_by_name_dp;
> + l_ctx_in->sbrec_logical_flow_by_logical_datapath =
> + sbrec_logical_flow_by_dp;
> l_ctx_in->sbrec_port_binding_by_name = sbrec_port_binding_by_name;
> l_ctx_in->dhcp_options_table = dhcp_table;
> l_ctx_in->dhcpv6_options_table = dhcpv6_table;
> @@ -1639,7 +1646,8 @@ flow_output_sb_mac_binding_handler(struct engine_node *node, void *data)
> }
>
> static bool
> -flow_output_sb_port_binding_handler(struct engine_node *node, void *data)
> +flow_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);
> @@ -1647,54 +1655,9 @@ flow_output_sb_port_binding_handler(struct engine_node *node, void *data)
> struct ed_type_flow_output *fo = data;
> struct ovn_desired_flow_table *flow_table = &fo->flow_table;
>
> - /* XXX: now we handle port-binding changes for physical flow processing
> - * only, but port-binding change can have impact to logical flow
> - * processing, too, in below circumstances:
> - *
> - * - When a port-binding for a lport is inserted/deleted but the lflow
> - * using that lport doesn't change.
> - *
> - * This can happen only when the lport name is used by ACL match
> - * condition, which is specified by user. Even in that case, if the port
> - * is actually bound on the current chassis it will trigger recompute on
> - * that chassis since ovs interface would be updated. So the only
> - * situation this would have real impact is when user defines an ACL
> - * that includes lport that is not on current chassis, and there is a
> - * port-binding creation/deletion related to that lport.e.g.: an ACL is
> - * defined:
> - *
> - * to-lport 1000 'outport=="A" && inport=="B"' allow-related
> - *
> - * If "A" is on current chassis, but "B" is lport that hasn't been
> - * created yet. When a lport "B" is created and bound on another
> - * chassis, the ACL will not take effect on the current chassis until a
> - * recompute is triggered later. This case doesn't seem to be a problem
> - * for real world use cases because usually lport is created before
> - * being referenced by name in ACLs.
> - *
> - * - When is_chassis_resident(<lport>) is used in lflow. In this case the
> - * port binding is not a regular VIF. It can be either "patch" or
> - * "external", with ha-chassis-group assigned. In current
> - * "runtime_data" handling, port-binding changes for these types always
> - * trigger recomputing. So it is fine even if we do not handle it here.
> - * (due to the ovsdb tracking support for referenced table changes,
> - * ha-chassis-group changes will appear as port-binding change).
> - *
> - * - When a mac-binding doesn't change but the port-binding related to
> - * that mac-binding is deleted. In this case the neighbor flow generated
> - * for the mac-binding should be deleted. This would not cause any real
> - * issue for now, since the port-binding related to mac-binding is
> - * always logical router port, and any change to logical router port
> - * would just trigger recompute.
> - *
> - * - When a port binding of type 'remote' is updated by ovn-ic.
> - *
> - * Although there is no correctness issue so far (except the unusual ACL
> - * use case, which doesn't seem to be a real problem), it might be better
> - * to handle this more gracefully, without the need to consider these
> - * tricky scenarios. One approach is to maintain a mapping between lport
> - * names and the lflows that uses them, and reprocess the related lflows
> - * when related port-bindings change.
> + /* We handle port-binding changes for physical flow processing
> + * only. flow_output runtime data handler takes care of processing
> + * logical flows for any port binding changes.
> */
> struct physical_ctx p_ctx;
> init_physical_ctx(node, rt_data, &p_ctx);
> @@ -1794,6 +1757,8 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data,
> updated = &pg_data->updated;
> deleted = &pg_data->deleted;
> break;
> +
> + case REF_TYPE_PORTBINDING:
> default:
> OVS_NOT_REACHED();
> }
> @@ -1917,16 +1882,52 @@ flow_output_runtime_data_handler(struct engine_node *node,
> return false;
> }
>
> - if (!hmap_is_empty(&tracked_data->tracked_dp_bindings)) {
> - /* We are not yet handling the tracked datapath binding
> - * changes. Return false to trigger full recompute. */
> - return false;
> + if (hmap_is_empty(&tracked_data->tracked_dp_bindings)) {
> + if (tracked_data->local_lports_changed) {
> + engine_set_node_state(node, EN_UPDATED);
> + }
> + return true;
> }
>
> - if (tracked_data->local_lports_changed) {
> + struct lflow_ctx_in l_ctx_in;
> + struct lflow_ctx_out l_ctx_out;
> + struct ed_type_flow_output *fo = data;
> + struct ed_type_runtime_data *rt_data =
> + engine_get_input_data("runtime_data", node);
> + init_lflow_ctx(node, rt_data, fo, &l_ctx_in, &l_ctx_out);
> +
> + struct physical_ctx p_ctx;
> + init_physical_ctx(node, rt_data, &p_ctx);
> +
> + bool handled = true;
> + struct tracked_binding_datapath *tdp;
> + HMAP_FOR_EACH (tdp, node, &tracked_data->tracked_dp_bindings) {
> + if (tdp->is_new) {
> + handled = lflow_add_flows_for_datapath(tdp->dp, &l_ctx_in,
> + &l_ctx_out);
> + if (!handled) {
> + break;
> + }
> + } else {
> + struct tracked_binding_lport *lport;
> + LIST_FOR_EACH (lport, list_node, &tdp->lports_head) {
> + if (!lflow_handle_flows_for_lport(lport->pb, &l_ctx_in,
> + &l_ctx_out)) {
> + handled = false;
> + break;
> + }
> + }
> + if (!handled) {
> + break;
> + }
> + }
> + }
> +
> + if (handled) {
> engine_set_node_state(node, EN_UPDATED);
> }
> - return true;
> +
> + return handled;
> }
>
> struct ovn_controller_exit_args {
> @@ -1983,6 +1984,9 @@ main(int argc, char *argv[])
> = chassis_index_create(ovnsb_idl_loop.idl);
> struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath
> = mcast_group_index_create(ovnsb_idl_loop.idl);
> + struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath
> + = ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
> + &sbrec_logical_flow_col_logical_datapath);
> struct ovsdb_idl_index *sbrec_port_binding_by_name
> = ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
> &sbrec_port_binding_col_logical_port);
> @@ -2132,6 +2136,8 @@ main(int argc, char *argv[])
> engine_ovsdb_node_add_index(&en_sb_chassis, "name", sbrec_chassis_by_name);
> engine_ovsdb_node_add_index(&en_sb_multicast_group, "name_datapath",
> sbrec_multicast_group_by_name_datapath);
> + engine_ovsdb_node_add_index(&en_sb_logical_flow, "logical_datapath",
> + sbrec_logical_flow_by_logical_datapath);
> engine_ovsdb_node_add_index(&en_sb_port_binding, "name",
> sbrec_port_binding_by_name);
> engine_ovsdb_node_add_index(&en_sb_port_binding, "key",
> diff --git a/controller/physical.h b/controller/physical.h
> index 9ca34436a..481f03901 100644
> --- a/controller/physical.h
> +++ b/controller/physical.h
> @@ -33,6 +33,7 @@ struct ovsrec_bridge;
> struct simap;
> struct sbrec_multicast_group_table;
> struct sbrec_port_binding_table;
> +struct sbrec_port_binding;
> struct sset;
>
> /* OVN Geneve option information.
> @@ -61,7 +62,7 @@ struct physical_ctx {
> void physical_register_ovs_idl(struct ovsdb_idl *);
> void physical_run(struct physical_ctx *,
> struct ovn_desired_flow_table *);
> -void physical_handle_port_binding_changes(struct physical_ctx *,
> +void physical_handle_port_binding_changes(struct physical_ctx *p_ctx,
> struct ovn_desired_flow_table *);
> void physical_handle_mc_group_changes(struct physical_ctx *,
> struct ovn_desired_flow_table *);
> diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
> index 6df42ce3a..a12757e18 100644
> --- a/tests/ovn-performance.at
> +++ b/tests/ovn-performance.at
> @@ -353,8 +353,8 @@ for i in 1 2; do
> )
>
> # Bind port $lp and wait for it to come up
> - OVN_CONTROLLER_EXPECT_HIT_COND(
> - [hv$i hv$j], [lflow_run], [>0 =0],
> + OVN_CONTROLLER_EXPECT_NO_HIT(
> + [hv$i hv$j], [lflow_run],
> [as hv$i ovs-vsctl add-port br-int $vif -- set Interface $vif external-ids:iface-id=$lp &&
> ovn-nbctl wait-until Logical_Switch_Port $lp 'up=true' &&
> ovn-nbctl --wait=hv sync]
> @@ -404,8 +404,8 @@ for i in 1 2; do
> lp=lp$i
>
> # Delete port $lp
> - OVN_CONTROLLER_EXPECT_HIT_COND(
> - [hv$i hv$j], [lflow_run], [>0 =0],
> + OVN_CONTROLLER_EXPECT_NO_HIT(
> + [hv$i hv$j], [lflow_run],
> [ovn-nbctl --wait=hv lsp-del $lp]
> )
> done
> --
> 2.25.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list