| Message ID | 20200608135038.1379231-1-numans@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Incremental processing improvements. | expand |
On 6/8/20 3:50 PM, numans@ovn.org wrote: > From: Venkata Anil <anilvenkata@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. > > Co-Authored-by: Numan Siddique <numans@ovn.org> > Signed-off-by: Venkata Anil <anilvenkata@redhat.com> > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > controller/lflow.c | 88 ++++++++++++++++++++++++++- > controller/lflow.h | 13 +++- > controller/ovn-controller.c | 116 +++++++++++++++++++----------------- > controller/physical.h | 3 +- > tests/ovn-performance.at | 12 ++-- > 5 files changed, 169 insertions(+), 63 deletions(-) > > diff --git a/controller/lflow.c b/controller/lflow.c > index 01214a3a6..46f84e5f8 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -59,6 +59,10 @@ struct condition_aux { > struct ovsdb_idl_index *sbrec_port_binding_by_name; > const struct sbrec_chassis *chassis; > const struct sset *active_tunnels; > + const struct sbrec_logical_flow *lflow; > + /* Resource reference to store the port name referenced > + * in is_chassis_resident() to lhe logicl flow. */ > + struct lflow_resource_ref *lfrr; > }; > > static bool > @@ -68,6 +72,8 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, > struct controller_event_options *controller_event_opts, > struct lflow_ctx_in *l_ctx_in, > struct lflow_ctx_out *l_ctx_out); > +static void lflow_resource_add(struct lflow_resource_ref *, enum ref_type, > + const char *ref_name, const struct uuid *); > > static bool > lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp) > @@ -120,6 +126,14 @@ is_chassis_resident_cb(const void *c_aux_, const char *port_name) > if (!pb) { > return false; > } > + > + /* Store the port_name to lflow reference. */ > + int64_t dp_id = pb->datapath->tunnel_key; > + char buf[16]; > + snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, dp_id, pb->tunnel_key); Hi Numan, Anil, We do this (or similar) in various places in lflow.c to build a unique key from dp_id and pb->tunnel_key/port_id. Maybe it's good to refactor it into a single function? Also, should we add some compile time checks to make sure 16 chars are enough? > + 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; > @@ -258,7 +272,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) Nit: indentation. > { > const struct sbrec_logical_flow *lflow; > > @@ -594,6 +608,8 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, > .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name, > .chassis = l_ctx_in->chassis, > .active_tunnels = l_ctx_in->active_tunnels, > + .lflow = lflow, > + .lfrr = l_ctx_out->lfrr > }; > expr = expr_simplify(expr, is_chassis_resident_cb, &cond_aux); > expr = expr_normalize(expr); > @@ -649,6 +665,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 +865,71 @@ lflow_destroy(void) > expr_symtab_destroy(&symtab); > shash_destroy(&symtab); > } > + > +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) { > + /* Remove the lflow from flow_table if present before processing it. */ > + ofctrl_remove_flows(l_ctx_out->flow_table, &lflow->header_.uuid); > + > + 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) I think this function should be in ovn-controller.c where we do all the other calls to lflow_handle_changed_ref() for address sets and port groups. > +{ > + 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..c8ed5b686 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; We never use sbrec_port_binding_table, I guess we can remove it. > +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,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 98652866c..2347ec669 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -1502,6 +1502,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), > @@ -1549,6 +1554,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; > @@ -1703,7 +1710,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); > @@ -1711,56 +1719,13 @@ 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); > > + /* 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); > @@ -1848,6 +1813,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: I think a comment here would be helpful; something to specify that port_binding related lflow updates are handled in lflow_handle_flows_for_lport(). > default: > OVS_NOT_REACHED(); > } > @@ -1909,17 +1876,53 @@ flow_output_runtime_data_handler(struct engine_node *node, > return false; > } > > - if (!hmap_is_empty(&rt_data->tracked_dp_bindings)) { > - /* We are not yet handling the tracked datapath binding > - * changes. Return false to trigger full recompute. */ > - return false; > + struct hmap *tracked_dp_bindings = &rt_data->tracked_dp_bindings; > + if (hmap_is_empty(tracked_dp_bindings)) { > + if (rt_data->local_lports_changed) { > + engine_set_node_state(node, EN_UPDATED); > + } > + return true; > + } > + > + struct lflow_ctx_in l_ctx_in; > + struct lflow_ctx_out l_ctx_out; > + struct ed_type_flow_output *fo = data; > + 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; I think we don't need the handled variable here. We can just return false where needed and true at the end. > + struct tracked_binding_datapath *tdp; > + HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) { > + if (tdp->is_new || !datapath_is_switch(tdp->dp)) { Why do we always have to add flows for logical routers? A comment explaining that might be enough. > + handled = lflow_add_flows_for_datapath(tdp->dp, &l_ctx_in, > + &l_ctx_out); > + if (!handled) { > + break; > + } This could be: if (!lflow_add_flows_for_datapath(tdp->dp, &l_ctx_in, &l_ctx_out)) { return false; } > + } else { > + struct shash_node *shash_node; > + SHASH_FOR_EACH (shash_node, &tdp->lports) { > + struct tracked_binding_lport *lport = shash_node->data; > + if (!lflow_handle_flows_for_lport(lport->pb, &l_ctx_in, > + &l_ctx_out)) { > + handled = false; > + break; Similar here. > + } > + } > + > + if (!handled) { > + break; > + } This would not be needed then. > + } > } > > - if (rt_data->local_lports_changed) { > + if (handled) { > engine_set_node_state(node, EN_UPDATED); > } Here we could skip the if instead: engine_set_node_state(node, EN_UPDATED); return true; > > - return true; > + return handled; > } > > static bool > @@ -2095,6 +2098,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); > @@ -2255,6 +2261,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 It looks to me like we don't need the changes in 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 08acd3bae..a12757e18 100644 > --- a/tests/ovn-performance.at > +++ b/tests/ovn-performance.at > @@ -274,7 +274,7 @@ for i in 1 2; do > ) > > # Add router port to $ls > - OVN_CONTROLLER_EXPECT_HIT( > + OVN_CONTROLLER_EXPECT_NO_HIT( > [hv1 hv2], [lflow_run], > [ovn-nbctl --wait=hv lrp-add lr1 $lrp 02:00:00:00:0$i:01 10.0.$i.1/24] > ) > @@ -282,7 +282,7 @@ for i in 1 2; do > [hv1 hv2], [lflow_run], > [ovn-nbctl --wait=hv lsp-add $ls $lsp] > ) > - OVN_CONTROLLER_EXPECT_HIT( > + OVN_CONTROLLER_EXPECT_NO_HIT( > [hv1 hv2], [lflow_run], > [ovn-nbctl --wait=hv lsp-set-type $lsp router] > ) > @@ -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 >
On Tue, Jun 9, 2020 at 4:18 PM Dumitru Ceara <dceara@redhat.com> wrote: > On 6/8/20 3:50 PM, numans@ovn.org wrote: > > From: Venkata Anil <anilvenkata@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. > > > > Co-Authored-by: Numan Siddique <numans@ovn.org> > > Signed-off-by: Venkata Anil <anilvenkata@redhat.com> > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > controller/lflow.c | 88 ++++++++++++++++++++++++++- > > controller/lflow.h | 13 +++- > > controller/ovn-controller.c | 116 +++++++++++++++++++----------------- > > controller/physical.h | 3 +- > > tests/ovn-performance.at | 12 ++-- > > 5 files changed, 169 insertions(+), 63 deletions(-) > > > > diff --git a/controller/lflow.c b/controller/lflow.c > > index 01214a3a6..46f84e5f8 100644 > > --- a/controller/lflow.c > > +++ b/controller/lflow.c > > @@ -59,6 +59,10 @@ struct condition_aux { > > struct ovsdb_idl_index *sbrec_port_binding_by_name; > > const struct sbrec_chassis *chassis; > > const struct sset *active_tunnels; > > + const struct sbrec_logical_flow *lflow; > > + /* Resource reference to store the port name referenced > > + * in is_chassis_resident() to lhe logicl flow. */ > > + struct lflow_resource_ref *lfrr; > > }; > > > > static bool > > @@ -68,6 +72,8 @@ consider_logical_flow(const struct sbrec_logical_flow > *lflow, > > struct controller_event_options > *controller_event_opts, > > struct lflow_ctx_in *l_ctx_in, > > struct lflow_ctx_out *l_ctx_out); > > +static void lflow_resource_add(struct lflow_resource_ref *, enum > ref_type, > > + const char *ref_name, const struct uuid > *); > > > > static bool > > lookup_port_cb(const void *aux_, const char *port_name, unsigned int > *portp) > > @@ -120,6 +126,14 @@ is_chassis_resident_cb(const void *c_aux_, const > char *port_name) > > if (!pb) { > > return false; > > } > > + > > + /* Store the port_name to lflow reference. */ > > + int64_t dp_id = pb->datapath->tunnel_key; > > + char buf[16]; > > + snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, dp_id, > pb->tunnel_key); > > Hi Numan, Anil, > > We do this (or similar) in various places in lflow.c to build a unique > key from dp_id and pb->tunnel_key/port_id. Maybe it's good to refactor > it into a single function? > > Ack. How about I'll add new patch - patch 9 which does this refactor so as to not disturb the existing patches. > Also, should we add some compile time checks to make sure 16 chars are > enough? > I'm thinking to have a util function like void get_lport_dp_key(uint64_t dp_key, uint64_t lport_key, char *buf); How would you suggest to have a compile time check for it ? Also let me know if the function signature is fine. > > + 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; > > @@ -258,7 +272,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) > > Nit: indentation. > > > { > > const struct sbrec_logical_flow *lflow; > > > > @@ -594,6 +608,8 @@ consider_logical_flow(const struct > sbrec_logical_flow *lflow, > > .sbrec_port_binding_by_name = > l_ctx_in->sbrec_port_binding_by_name, > > .chassis = l_ctx_in->chassis, > > .active_tunnels = l_ctx_in->active_tunnels, > > + .lflow = lflow, > > + .lfrr = l_ctx_out->lfrr > > }; > > expr = expr_simplify(expr, is_chassis_resident_cb, &cond_aux); > > expr = expr_normalize(expr); > > @@ -649,6 +665,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 +865,71 @@ lflow_destroy(void) > > expr_symtab_destroy(&symtab); > > shash_destroy(&symtab); > > } > > + > > +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) { > > + /* Remove the lflow from flow_table if present before > processing it. */ > > + ofctrl_remove_flows(l_ctx_out->flow_table, > &lflow->header_.uuid); > > + > > + 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) > > I think this function should be in ovn-controller.c where we do all the > other calls to lflow_handle_changed_ref() for address sets and port groups. > I think you answered yourself below. I'll add a comment in the lflow_handle_changed_ref() as you suggested. > > +{ > > + 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..c8ed5b686 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; > > We never use sbrec_port_binding_table, I guess we can remove it. > Ack. > > > +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,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 98652866c..2347ec669 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -1502,6 +1502,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), > > @@ -1549,6 +1554,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; > > @@ -1703,7 +1710,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); > > @@ -1711,56 +1719,13 @@ 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); > > > > + /* 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); > > @@ -1848,6 +1813,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: > > I think a comment here would be helpful; something to specify that > port_binding related lflow updates are handled in > lflow_handle_flows_for_lport(). > Ack. > > > default: > > OVS_NOT_REACHED(); > > } > > @@ -1909,17 +1876,53 @@ flow_output_runtime_data_handler(struct > engine_node *node, > > return false; > > } > > > > - if (!hmap_is_empty(&rt_data->tracked_dp_bindings)) { > > - /* We are not yet handling the tracked datapath binding > > - * changes. Return false to trigger full recompute. */ > > - return false; > > + struct hmap *tracked_dp_bindings = &rt_data->tracked_dp_bindings; > > + if (hmap_is_empty(tracked_dp_bindings)) { > > + if (rt_data->local_lports_changed) { > > + engine_set_node_state(node, EN_UPDATED); > > + } > > + return true; > > + } > > + > > + struct lflow_ctx_in l_ctx_in; > > + struct lflow_ctx_out l_ctx_out; > > + struct ed_type_flow_output *fo = data; > > + 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; > > I think we don't need the handled variable here. We can just return > false where needed and true at the end. > > > + struct tracked_binding_datapath *tdp; > > + HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) { > > + if (tdp->is_new || !datapath_is_switch(tdp->dp)) { > > Why do we always have to add flows for logical routers? A comment > explaining that might be enough. > > Actually I don't think we need to process the lflow for logical routers. We have logical flows in router datapath as "inport == lrp-. && " and when a logical router port is added to the tracked dp, processing the lflows for the lrp is good enough. I'll remove it. > + handled = lflow_add_flows_for_datapath(tdp->dp, &l_ctx_in, > > + &l_ctx_out); > > + if (!handled) { > > + break; > > + } > > This could be: > > if (!lflow_add_flows_for_datapath(tdp->dp, &l_ctx_in, &l_ctx_out)) { > return false; > } > > > + } else { > > + struct shash_node *shash_node; > > + SHASH_FOR_EACH (shash_node, &tdp->lports) { > > + struct tracked_binding_lport *lport = shash_node->data; > > + if (!lflow_handle_flows_for_lport(lport->pb, &l_ctx_in, > > + &l_ctx_out)) { > > + handled = false; > > + break; > > Similar here. > > > + } > > + } > > + > > + if (!handled) { > > + break; > > + } > > This would not be needed then. > > > + } > > } > > > > - if (rt_data->local_lports_changed) { > > + if (handled) { > > engine_set_node_state(node, EN_UPDATED); > > } > > Here we could skip the if instead: > > engine_set_node_state(node, EN_UPDATED); > return true; > Ack. > > > > > - return true; > > + return handled; > > } > > > > static bool > > @@ -2095,6 +2098,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); > > @@ -2255,6 +2261,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 > > It looks to me like we don't need the changes in physical.h. > Ack. These leftovers. > > > 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 08acd3bae..a12757e18 100644 > > --- a/tests/ovn-performance.at > > +++ b/tests/ovn-performance.at > > @@ -274,7 +274,7 @@ for i in 1 2; do > > ) > > > > # Add router port to $ls > > - OVN_CONTROLLER_EXPECT_HIT( > > + OVN_CONTROLLER_EXPECT_NO_HIT( > > [hv1 hv2], [lflow_run], > > [ovn-nbctl --wait=hv lrp-add lr1 $lrp 02:00:00:00:0$i:01 > 10.0.$i.1/24] > > ) > > @@ -282,7 +282,7 @@ for i in 1 2; do > > [hv1 hv2], [lflow_run], > > [ovn-nbctl --wait=hv lsp-add $ls $lsp] > > ) > > - OVN_CONTROLLER_EXPECT_HIT( > > + OVN_CONTROLLER_EXPECT_NO_HIT( > > [hv1 hv2], [lflow_run], > > [ovn-nbctl --wait=hv lsp-set-type $lsp router] > > ) > > @@ -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 > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 6/9/20 6:26 PM, Numan Siddique wrote: > > On Tue, Jun 9, 2020 at 4:18 PM Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com>> wrote: > > On 6/8/20 3:50 PM, numans@ovn.org <mailto:numans@ovn.org> wrote: > > From: Venkata Anil <anilvenkata@redhat.com > <mailto:anilvenkata@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. > > > > Co-Authored-by: Numan Siddique <numans@ovn.org > <mailto:numans@ovn.org>> > > Signed-off-by: Venkata Anil <anilvenkata@redhat.com > <mailto:anilvenkata@redhat.com>> > > Signed-off-by: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > > --- > > controller/lflow.c | 88 ++++++++++++++++++++++++++- > > controller/lflow.h | 13 +++- > > controller/ovn-controller.c | 116 > +++++++++++++++++++----------------- > > controller/physical.h | 3 +- > > tests/ovn-performance.at <http://ovn-performance.at> | 12 ++-- > > 5 files changed, 169 insertions(+), 63 deletions(-) > > > > diff --git a/controller/lflow.c b/controller/lflow.c > > index 01214a3a6..46f84e5f8 100644 > > --- a/controller/lflow.c > > +++ b/controller/lflow.c > > @@ -59,6 +59,10 @@ struct condition_aux { > > struct ovsdb_idl_index *sbrec_port_binding_by_name; > > const struct sbrec_chassis *chassis; > > const struct sset *active_tunnels; > > + const struct sbrec_logical_flow *lflow; > > + /* Resource reference to store the port name referenced > > + * in is_chassis_resident() to lhe logicl flow. */ > > + struct lflow_resource_ref *lfrr; > > }; > > > > static bool > > @@ -68,6 +72,8 @@ consider_logical_flow(const struct > sbrec_logical_flow *lflow, > > struct controller_event_options > *controller_event_opts, > > struct lflow_ctx_in *l_ctx_in, > > struct lflow_ctx_out *l_ctx_out); > > +static void lflow_resource_add(struct lflow_resource_ref *, enum > ref_type, > > + const char *ref_name, const struct > uuid *); > > > > static bool > > lookup_port_cb(const void *aux_, const char *port_name, unsigned > int *portp) > > @@ -120,6 +126,14 @@ is_chassis_resident_cb(const void *c_aux_, > const char *port_name) > > if (!pb) { > > return false; > > } > > + > > + /* Store the port_name to lflow reference. */ > > + int64_t dp_id = pb->datapath->tunnel_key; > > + char buf[16]; > > + snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, dp_id, > pb->tunnel_key); > > Hi Numan, Anil, > > We do this (or similar) in various places in lflow.c to build a unique > key from dp_id and pb->tunnel_key/port_id. Maybe it's good to refactor > it into a single function? > > > Ack. How about I'll add new patch - patch 9 which does this refactor so > as to not > disturb the existing patches. > > Sounds good to me. > > Also, should we add some compile time checks to make sure 16 chars are > enough? > > > I'm thinking to have a util function like > > void get_lport_dp_key(uint64_t dp_key, uint64_t lport_key, char *buf); > Ack. > How would you suggest to have a compile time check for it ? > I don't have a neat way I guess and as far as I see there's no single place where we define the max size of a port tunnel key but we assume that it's at most 15 bits so we can probably forget about such a check for now. Sorry for the noise. > Also let me know if the function signature is fine. > > > > > + 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; > > @@ -258,7 +272,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) > > Nit: indentation. > > > { > > const struct sbrec_logical_flow *lflow; > > > > @@ -594,6 +608,8 @@ consider_logical_flow(const struct > sbrec_logical_flow *lflow, > > .sbrec_port_binding_by_name = > l_ctx_in->sbrec_port_binding_by_name, > > .chassis = l_ctx_in->chassis, > > .active_tunnels = l_ctx_in->active_tunnels, > > + .lflow = lflow, > > + .lfrr = l_ctx_out->lfrr > > }; > > expr = expr_simplify(expr, is_chassis_resident_cb, &cond_aux); > > expr = expr_normalize(expr); > > @@ -649,6 +665,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 +865,71 @@ lflow_destroy(void) > > expr_symtab_destroy(&symtab); > > shash_destroy(&symtab); > > } > > + > > +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) { > > + /* Remove the lflow from flow_table if present before > processing it. */ > > + ofctrl_remove_flows(l_ctx_out->flow_table, > &lflow->header_.uuid); > > + > > + 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) > > I think this function should be in ovn-controller.c where we do all the > other calls to lflow_handle_changed_ref() for address sets and port > groups. > > > I think you answered yourself below. I'll add a comment in > the lflow_handle_changed_ref() > as you suggested. > Partially, but what I meant is that the only place where we currently check ref_type and call lflow_handle_changed_ref is in _flow_output_resource_ref_handler() in ovn-controller.c. In my opinion would make it easier to follow the code if all calls to lflow_handle_changed_ref() are grouped/close to eachother and therefore all handling of various ref_types is done in the same module. > > > > +{ > > + 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..c8ed5b686 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; > > We never use sbrec_port_binding_table, I guess we can remove it. > > > Ack. > > > > > +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,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 98652866c..2347ec669 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -1502,6 +1502,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), > > @@ -1549,6 +1554,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; > > @@ -1703,7 +1710,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); > > @@ -1711,56 +1719,13 @@ 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); > > > > + /* 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); > > @@ -1848,6 +1813,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: > > I think a comment here would be helpful; something to specify that > port_binding related lflow updates are handled in > lflow_handle_flows_for_lport(). > > > Ack. > > > > > > default: > > OVS_NOT_REACHED(); > > } > > @@ -1909,17 +1876,53 @@ flow_output_runtime_data_handler(struct > engine_node *node, > > return false; > > } > > > > - if (!hmap_is_empty(&rt_data->tracked_dp_bindings)) { > > - /* We are not yet handling the tracked datapath binding > > - * changes. Return false to trigger full recompute. */ > > - return false; > > + struct hmap *tracked_dp_bindings = &rt_data->tracked_dp_bindings; > > + if (hmap_is_empty(tracked_dp_bindings)) { > > + if (rt_data->local_lports_changed) { > > + engine_set_node_state(node, EN_UPDATED); > > + } > > + return true; > > + } > > + > > + struct lflow_ctx_in l_ctx_in; > > + struct lflow_ctx_out l_ctx_out; > > + struct ed_type_flow_output *fo = data; > > + 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; > > I think we don't need the handled variable here. We can just return > false where needed and true at the end. > > > + struct tracked_binding_datapath *tdp; > > + HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) { > > + if (tdp->is_new || !datapath_is_switch(tdp->dp)) { > > Why do we always have to add flows for logical routers? A comment > explaining that might be enough. > > Actually I don't think we need to process the lflow for logical routers. > > We have logical flows in router datapath as "inport == lrp-. && " and when a > logical router port is added to the tracked dp, processing the lflows > for the lrp > is good enough. > > I'll remove it. > Ok. Thanks, Dumitru > > > > + handled = lflow_add_flows_for_datapath(tdp->dp, > &l_ctx_in, > > + &l_ctx_out); > > + if (!handled) { > > + break; > > + } > > This could be: > > if (!lflow_add_flows_for_datapath(tdp->dp, &l_ctx_in, &l_ctx_out)) { > return false; > } > > > + } else { > > + struct shash_node *shash_node; > > + SHASH_FOR_EACH (shash_node, &tdp->lports) { > > + struct tracked_binding_lport *lport = > shash_node->data; > > + if (!lflow_handle_flows_for_lport(lport->pb, > &l_ctx_in, > > + &l_ctx_out)) { > > + handled = false; > > + break; > > Similar here. > > > + } > > + } > > + > > + if (!handled) { > > + break; > > + } > > This would not be needed then. > > > + } > > } > > > > - if (rt_data->local_lports_changed) { > > + if (handled) { > > engine_set_node_state(node, EN_UPDATED); > > } > > Here we could skip the if instead: > > engine_set_node_state(node, EN_UPDATED); > return true; > > > Ack. > > > > > > > > - return true; > > + return handled; > > } > > > > static bool > > @@ -2095,6 +2098,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); > > @@ -2255,6 +2261,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 > > It looks to me like we don't need the changes in physical.h. > > > Ack. These leftovers. > > > > > 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 <http://ovn-performance.at> > b/tests/ovn-performance.at <http://ovn-performance.at> > > index 08acd3bae..a12757e18 100644 > > --- a/tests/ovn-performance.at <http://ovn-performance.at> > > +++ b/tests/ovn-performance.at <http://ovn-performance.at> > > @@ -274,7 +274,7 @@ for i in 1 2; do > > ) > > > > # Add router port to $ls > > - OVN_CONTROLLER_EXPECT_HIT( > > + OVN_CONTROLLER_EXPECT_NO_HIT( > > [hv1 hv2], [lflow_run], > > [ovn-nbctl --wait=hv lrp-add lr1 $lrp 02:00:00:00:0$i:01 > 10.0.$i.1/24] > > ) > > @@ -282,7 +282,7 @@ for i in 1 2; do > > [hv1 hv2], [lflow_run], > > [ovn-nbctl --wait=hv lsp-add $ls $lsp] > > ) > > - OVN_CONTROLLER_EXPECT_HIT( > > + OVN_CONTROLLER_EXPECT_NO_HIT( > > [hv1 hv2], [lflow_run], > > [ovn-nbctl --wait=hv lsp-set-type $lsp router] > > ) > > @@ -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 > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/controller/lflow.c b/controller/lflow.c index 01214a3a6..46f84e5f8 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -59,6 +59,10 @@ struct condition_aux { struct ovsdb_idl_index *sbrec_port_binding_by_name; const struct sbrec_chassis *chassis; const struct sset *active_tunnels; + const struct sbrec_logical_flow *lflow; + /* Resource reference to store the port name referenced + * in is_chassis_resident() to lhe logicl flow. */ + struct lflow_resource_ref *lfrr; }; static bool @@ -68,6 +72,8 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, struct controller_event_options *controller_event_opts, struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out); +static void lflow_resource_add(struct lflow_resource_ref *, enum ref_type, + const char *ref_name, const struct uuid *); static bool lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp) @@ -120,6 +126,14 @@ is_chassis_resident_cb(const void *c_aux_, const char *port_name) if (!pb) { return false; } + + /* Store the port_name to lflow reference. */ + int64_t dp_id = pb->datapath->tunnel_key; + char buf[16]; + snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, dp_id, pb->tunnel_key); + 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; @@ -258,7 +272,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; @@ -594,6 +608,8 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name, .chassis = l_ctx_in->chassis, .active_tunnels = l_ctx_in->active_tunnels, + .lflow = lflow, + .lfrr = l_ctx_out->lfrr }; expr = expr_simplify(expr, is_chassis_resident_cb, &cond_aux); expr = expr_normalize(expr); @@ -649,6 +665,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 +865,71 @@ lflow_destroy(void) expr_symtab_destroy(&symtab); shash_destroy(&symtab); } + +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) { + /* Remove the lflow from flow_table if present before processing it. */ + ofctrl_remove_flows(l_ctx_out->flow_table, &lflow->header_.uuid); + + 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..c8ed5b686 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,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 98652866c..2347ec669 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1502,6 +1502,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), @@ -1549,6 +1554,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; @@ -1703,7 +1710,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); @@ -1711,56 +1719,13 @@ 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); + /* 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); @@ -1848,6 +1813,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,17 +1876,53 @@ flow_output_runtime_data_handler(struct engine_node *node, return false; } - if (!hmap_is_empty(&rt_data->tracked_dp_bindings)) { - /* We are not yet handling the tracked datapath binding - * changes. Return false to trigger full recompute. */ - return false; + struct hmap *tracked_dp_bindings = &rt_data->tracked_dp_bindings; + if (hmap_is_empty(tracked_dp_bindings)) { + if (rt_data->local_lports_changed) { + engine_set_node_state(node, EN_UPDATED); + } + return true; + } + + struct lflow_ctx_in l_ctx_in; + struct lflow_ctx_out l_ctx_out; + struct ed_type_flow_output *fo = data; + 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_dp_bindings) { + if (tdp->is_new || !datapath_is_switch(tdp->dp)) { + handled = lflow_add_flows_for_datapath(tdp->dp, &l_ctx_in, + &l_ctx_out); + if (!handled) { + break; + } + } else { + struct shash_node *shash_node; + SHASH_FOR_EACH (shash_node, &tdp->lports) { + struct tracked_binding_lport *lport = shash_node->data; + if (!lflow_handle_flows_for_lport(lport->pb, &l_ctx_in, + &l_ctx_out)) { + handled = false; + break; + } + } + + if (!handled) { + break; + } + } } - if (rt_data->local_lports_changed) { + if (handled) { engine_set_node_state(node, EN_UPDATED); } - return true; + return handled; } static bool @@ -2095,6 +2098,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); @@ -2255,6 +2261,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 08acd3bae..a12757e18 100644 --- a/tests/ovn-performance.at +++ b/tests/ovn-performance.at @@ -274,7 +274,7 @@ for i in 1 2; do ) # Add router port to $ls - OVN_CONTROLLER_EXPECT_HIT( + OVN_CONTROLLER_EXPECT_NO_HIT( [hv1 hv2], [lflow_run], [ovn-nbctl --wait=hv lrp-add lr1 $lrp 02:00:00:00:0$i:01 10.0.$i.1/24] ) @@ -282,7 +282,7 @@ for i in 1 2; do [hv1 hv2], [lflow_run], [ovn-nbctl --wait=hv lsp-add $ls $lsp] ) - OVN_CONTROLLER_EXPECT_HIT( + OVN_CONTROLLER_EXPECT_NO_HIT( [hv1 hv2], [lflow_run], [ovn-nbctl --wait=hv lsp-set-type $lsp router] ) @@ -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