On Thu, May 21, 2020 at 12:42 PM Han Zhou <hz...@ovn.org> wrote: > On Wed, May 20, 2020 at 12:40 PM <num...@ovn.org> wrote: > > > > From: Venkata Anil <anilvenk...@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 <mmich...@redhat.com> > > Co-Authored-by: Numan Siddique <num...@ovn.org> > > Signed-off-by: Venkata Anil <anilvenk...@redhat.com> > > Signed-off-by: Numan Siddique <num...@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 ? 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 ? 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 > > 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 > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev