[ovs-dev] [PATCH ovn v7 8/9] ovn-controller: Use the tracked runtime data changes for flow calculation.
Numan Siddique
numans at ovn.org
Wed May 27 08:23:32 UTC 2020
On Wed, May 27, 2020 at 3:30 AM Han Zhou <hzhou at ovn.org> wrote:
> On Tue, May 26, 2020 at 6:57 AM Numan Siddique <numans at ovn.org> wrote:
> >
> >
> >
> > On Thu, May 21, 2020 at 12:42 PM Han Zhou <hzhou at ovn.org> wrote:
> >>
> >> On Wed, May 20, 2020 at 12:40 PM <numans at ovn.org> wrote:
> >> >
> >> > From: Venkata Anil <anilvenkata at redhat.com>
> >> >
> >> > 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.
> >> >
> >> > Acked-by: Mark Michelson <mmichels 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>
> >> > ---
> >> > controller/lflow.c | 82 +++++++++++++++++++++++-
> >> > controller/lflow.h | 14 +++-
> >> > controller/ovn-controller.c | 124
> ++++++++++++++++++++----------------
> >> > controller/physical.h | 3 +-
> >> > tests/ovn-performance.at | 4 +-
> >> > 5 files changed, 168 insertions(+), 59 deletions(-)
> >> >
> >> > diff --git a/controller/lflow.c b/controller/lflow.c
> >> > index 01214a3a6..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",
> >> > @@ -847,3 +849,81 @@ lflow_destroy(void)
> >> > expr_symtab_destroy(&symtab);
> >> > shash_destroy(&symtab);
> >> > }
> >> > +
> >> > +bool
> >> > +lflow_evaluate_pb_changes(const struct sbrec_port_binding_table
> >> *pb_table)
> >> > +{
> >> > + const struct sbrec_port_binding *binding;
> >> > + SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (binding, pb_table) {
> >> > + if (!strcmp(binding->type, "remote")) {
> >>
> >> "remote" can be handled just like any regular VIF, just that it would
> never
> >> be bound on the current chassis.
> >
> >
> > I didn't handle this comment in v8 as I was not to look into the remote
> code again
> > to understand a bit.
> >
> > The present approach is a bit inefficient because it would resort to full
> recompute when
> > any port bindings of type remote change. Are you fine If I handle this in
> next patch
> > series which I've started working on ?
> >
> Hi Numan,
>
> I think it would be better to address it in this series because "remote"
> port binding changes were handled incrementally even before this series.
> Without this, any remote port changes will trigger recompute on all HVs in
> all AZs. It is in fact the "easy" part because we are sure that "remote" is
> never going to be bound or unbound from current chassis.
>
Thanks. It would be wrong to make it non I-P now. I'll address it in v9 and
submit soon.
Thanks
Numan
>
> > If I understand correctly from the comment, if any remote port binding
> changes,
> > we need to add this in the tracked_dp_bindings ? so that
> lflow_handle_flows_for_lport()
> > will handle this ?
>
> I assume that we don't need to handle anything in extra, just keeping the
> original change-handler for port-bindings in flow_output. What do you
> think?
>
> >
> > Thanks
> > Numan
> >
> >>
> >> > + return false;
> >> > + }
> >> > + }
> >> > +
> >> > + 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 f02f709d7..00c1b5c47 100644
> >> > --- a/controller/lflow.h
> >> > +++ b/controller/lflow.h
> >> > @@ -48,6 +48,9 @@ struct sbrec_dhcp_options_table;
> >> > 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;
> >> > @@ -72,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
> >> > @@ -117,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;
> >> > @@ -157,4 +162,11 @@ 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 *);
> >> > +bool lflow_evaluate_pb_changes(const struct sbrec_port_binding_table
> *);
> >> > #endif /* controller/lflow.h */
> >> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> >> > index dc790f0f7..e53f750bf 100644
> >> > --- a/controller/ovn-controller.c
> >> > +++ b/controller/ovn-controller.c
> >> > @@ -1440,6 +1440,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),
> >> > @@ -1487,6 +1492,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;
> >> > @@ -1641,7 +1648,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);
> >> > @@ -1649,56 +1657,21 @@ 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.
> >> > - *
> >> > - * 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.
> >> > - */
> >> > struct physical_ctx p_ctx;
> >> > init_physical_ctx(node, rt_data, &p_ctx);
> >> >
> >> > + if (!lflow_evaluate_pb_changes(p_ctx.port_binding_table)) {
> >> > + /* If this returns false, it means there is an impact on
> >> > + * the logical flow processing because of some changes in
> >> > + * port bindings. Return false so that recompute is triggered
> >> > + * for this stage. */
> >> > + return false;
> >> > + }
> >> > +
> >> > + /* 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.
> >> > + */
> >> > physical_handle_port_binding_changes(&p_ctx, flow_table);
> >> >
> >> > engine_set_node_state(node, EN_UPDATED);
> >> > @@ -1786,6 +1759,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();
> >> > }
> >> > @@ -1909,16 +1884,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 {
> >> > @@ -1976,6 +1987,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);
> >> > @@ -2125,6 +2139,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 6e064e64f..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]
> >> > --
> >> > 2.26.2
> >> >
> >> > _______________________________________________
> >> > dev mailing list
> >> > dev at openvswitch.org
> >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >> _______________________________________________
> >> dev mailing list
> >> dev at openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
More information about the dev
mailing list