On Wed, May 13, 2020 at 8:21 PM Dumitru Ceara <[email protected]> wrote:
> On 5/11/20 11:46 AM, [email protected] wrote: > > From: Numan Siddique <[email protected]> > > > > This patch handles ct zone changes and OVS interface changes > incrementally > > in the flow output stage. > > > > Any changes to ct zone can be handled by running physical_run() instead > of running > > flow_output_run(). And any changes to OVS interfaces can be either > handled > > incrementally (for OVS interfaces representing VIFs) or just running > > physical_run() (for tunnel and other types of interfaces). > > > > To better handle this, a new engine node 'physical_flow_changes' is > added which > > handles changes to ct zone and OVS interfaces. > > > > Signed-off-by: Numan Siddique <[email protected]> > > --- > > controller/binding.c | 22 --------- > > controller/lflow.c | 13 ++++++ > > controller/lflow.h | 2 + > > controller/ovn-controller.c | 92 ++++++++++++++++++++++++++++++++++++- > > controller/ovn-controller.h | 22 +++++++++ > > controller/physical.c | 51 ++++++++++++++++++++ > > controller/physical.h | 5 +- > > 7 files changed, 182 insertions(+), 25 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index 662424518..3821edc03 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -499,22 +499,6 @@ remove_local_lport_ids(const struct > sbrec_port_binding *binding_rec, > > sset_find_and_delete(local_lport_ids, buf); > > } > > > > -enum local_binding_type { > > - BT_VIF, > > - BT_CHILD, > > - BT_VIRTUAL > > -}; > > - > > -struct local_binding { > > - struct ovs_list node; /* In parent if any. */ > > - char *name; > > - enum local_binding_type type; > > - const struct ovsrec_interface *iface; > > - const struct sbrec_port_binding *pb; > > - > > - struct ovs_list children; > > -}; > > - > > static struct local_binding * > > local_binding_create(const char *name, const struct ovsrec_interface > *iface, > > const struct sbrec_port_binding *pb, > > @@ -535,12 +519,6 @@ local_binding_add(struct shash *local_bindings, > struct local_binding *lbinding) > > shash_add(local_bindings, lbinding->name, lbinding); > > } > > > > -static struct local_binding * > > -local_binding_find(struct shash *local_bindings, const char *name) > > -{ > > - return shash_find_data(local_bindings, name); > > -} > > - > > Hi Numan, > > I think if struct local_binding is to be shared between units we should > probably decide on a single place where the structure is defined and all > it's related API functions should be declared there. > > Now we have "struct local_binding" and local_binding_find() in > ovn-controller.h and local_bindings_destroy() in binding.h. > I've moved to binding.h in v6. > > > static void > > local_binding_destroy(struct local_binding *lbinding) > > { > > diff --git a/controller/lflow.c b/controller/lflow.c > > index 01214a3a6..512258cd3 100644 > > --- a/controller/lflow.c > > +++ b/controller/lflow.c > > @@ -847,3 +847,16 @@ 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")) { > > + return false; > > + } > > + } > > + > > + return true; > > +} > > Shouldn't lflow_evaluate_pb_changes() be part of a separate patch? > These changes are required, but not in this patch, but in patch 8. I've moved this to patch 8 in v6. > > diff --git a/controller/lflow.h b/controller/lflow.h > > index f02f709d7..fa54d4de2 100644 > > --- a/controller/lflow.h > > +++ b/controller/lflow.h > > @@ -48,6 +48,7 @@ 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 simap; > > struct sset; > > struct uuid; > > @@ -153,6 +154,7 @@ void lflow_handle_changed_neighbors( > > const struct hmap *local_datapaths, > > struct ovn_desired_flow_table *); > > > > +bool lflow_evaluate_pb_changes(const struct sbrec_port_binding_table *); > > void lflow_destroy(void); > > > > void lflow_expr_destroy(struct hmap *lflow_expr_cache); > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index f94cd16d1..4eee9a2c9 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -1369,8 +1369,13 @@ static void init_physical_ctx(struct engine_node > *node, > > > > ovs_assert(br_int && chassis); > > > > + struct ovsrec_interface_table *iface_table = > > + (struct ovsrec_interface_table *)EN_OVSDB_GET( > > + engine_get_input("OVS_interface", > > + engine_get_input("physical_flow_changes", node))); > > struct ed_type_ct_zones *ct_zones_data = > > - engine_get_input_data("ct_zones", node); > > + engine_get_input_data("ct_zones", > > + engine_get_input("physical_flow_changes", node)); > > struct simap *ct_zones = &ct_zones_data->current; > > > > p_ctx->sbrec_port_binding_by_name = sbrec_port_binding_by_name; > > @@ -1378,12 +1383,14 @@ static void init_physical_ctx(struct engine_node > *node, > > p_ctx->mc_group_table = multicast_group_table; > > p_ctx->br_int = br_int; > > p_ctx->chassis_table = chassis_table; > > + p_ctx->iface_table = iface_table; > > p_ctx->chassis = chassis; > > p_ctx->active_tunnels = &rt_data->active_tunnels; > > p_ctx->local_datapaths = &rt_data->local_datapaths; > > p_ctx->local_lports = &rt_data->local_lports; > > p_ctx->ct_zones = ct_zones; > > p_ctx->mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve; > > + p_ctx->local_bindings = &rt_data->local_bindings; > > } > > > > static void init_lflow_ctx(struct engine_node *node, > > @@ -1646,6 +1653,8 @@ flow_output_sb_port_binding_handler(struct > engine_node *node, void *data) > > * always logical router port, and any change to logical router > port > > * would just trigger recompute. > > * > > + * - When a port binding of type 'remote' is updated by ovn-ic. > > + * > > * Although there is no correctness issue so far (except the > unusual ACL > > * use case, which doesn't seem to be a real problem), it might be > better > > * to handle this more gracefully, without the need to consider > these > > @@ -1656,6 +1665,14 @@ flow_output_sb_port_binding_handler(struct > engine_node *node, void *data) > > 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; > > + } > > + > > physical_handle_port_binding_changes(&p_ctx, flow_table); > > > > engine_set_node_state(node, EN_UPDATED); > > @@ -1791,6 +1808,70 @@ flow_output_port_groups_handler(struct > engine_node *node, void *data) > > return _flow_output_resource_ref_handler(node, data, > REF_TYPE_PORTGROUP); > > } > > > > +struct ed_type_physical_flow_changes { > > + bool need_physical_run; > > + bool ovs_ifaces_changed; > > +}; > > + > > +static bool > > +flow_output_physical_flow_changes_handler(struct engine_node *node, > void *data) > > +{ > > + struct ed_type_physical_flow_changes *pfc = > > + engine_get_input_data("physical_flow_changes", node); > > + struct ed_type_runtime_data *rt_data = > > + engine_get_input_data("runtime_data", node); > > + > > + struct ed_type_flow_output *fo = data; > > + struct physical_ctx p_ctx; > > + init_physical_ctx(node, rt_data, &p_ctx); > > + > > + engine_set_node_state(node, EN_UPDATED); > > + if (pfc->need_physical_run) { > > + pfc->need_physical_run = false; > > + physical_run(&p_ctx, &fo->flow_table); > > + return true; > > + } > > + > > + if (pfc->ovs_ifaces_changed) { > > + pfc->ovs_ifaces_changed = false; > > + return physical_handle_ovs_iface_changes(&p_ctx, > &fo->flow_table); > > + } > > + > > + return true; > > +} > > + > > +static void * > > +en_physical_flow_changes_init(struct engine_node *node OVS_UNUSED, > > + struct engine_arg *arg OVS_UNUSED) > > +{ > > + struct ed_type_physical_flow_changes *data = xzalloc(sizeof *data); > > + return data; > > +} > > + > > +static void > > +en_physical_flow_changes_cleanup(void *data OVS_UNUSED) > > +{ > > + > > +} > > + > > +static void > > +en_physical_flow_changes_run(struct engine_node *node, void *data > OVS_UNUSED) > > +{ > > + struct ed_type_physical_flow_changes *pfc = data; > > + pfc->need_physical_run = true; > > + engine_set_node_state(node, EN_UPDATED); > > +} > > + > > +static bool > > +physical_flow_changes_ovs_iface_handler(struct engine_node *node > OVS_UNUSED, > > + void *data OVS_UNUSED) > > +{ > > + struct ed_type_physical_flow_changes *pfc = data; > > + pfc->ovs_ifaces_changed = true; > > + engine_set_node_state(node, EN_UPDATED); > > + return true; > > +} > > + > > struct ovn_controller_exit_args { > > bool *exiting; > > bool *restart; > > @@ -1913,6 +1994,7 @@ main(int argc, char *argv[]) > > ENGINE_NODE(runtime_data, "runtime_data"); > > ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve"); > > ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected"); > > + ENGINE_NODE(physical_flow_changes, "physical_flow_changes"); > > ENGINE_NODE(flow_output, "flow_output"); > > ENGINE_NODE(addr_sets, "addr_sets"); > > ENGINE_NODE(port_groups, "port_groups"); > > @@ -1932,13 +2014,19 @@ main(int argc, char *argv[]) > > engine_add_input(&en_port_groups, &en_sb_port_group, > > port_groups_sb_port_group_handler); > > > > + engine_add_input(&en_physical_flow_changes, &en_ct_zones, > > + NULL); > > + engine_add_input(&en_physical_flow_changes, &en_ovs_interface, > > + physical_flow_changes_ovs_iface_handler); > > + > > engine_add_input(&en_flow_output, &en_addr_sets, > > flow_output_addr_sets_handler); > > engine_add_input(&en_flow_output, &en_port_groups, > > flow_output_port_groups_handler); > > engine_add_input(&en_flow_output, &en_runtime_data, NULL); > > - engine_add_input(&en_flow_output, &en_ct_zones, NULL); > > engine_add_input(&en_flow_output, &en_mff_ovn_geneve, NULL); > > + engine_add_input(&en_flow_output, &en_physical_flow_changes, > > + flow_output_physical_flow_changes_handler); > > > > engine_add_input(&en_flow_output, &en_ovs_open_vswitch, NULL); > > engine_add_input(&en_flow_output, &en_ovs_bridge, NULL); > > diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h > > index 5d9466880..40c358cef 100644 > > --- a/controller/ovn-controller.h > > +++ b/controller/ovn-controller.h > > @@ -72,6 +72,28 @@ struct local_datapath { > > struct local_datapath *get_local_datapath(const struct hmap *, > > uint32_t tunnel_key); > > > > +enum local_binding_type { > > + BT_VIF, > > + BT_CHILD, > > + BT_VIRTUAL > > +}; > > + > > +struct local_binding { > > + struct ovs_list node; /* In parent if any. */ > > + char *name; > > + enum local_binding_type type; > > + const struct ovsrec_interface *iface; > > + const struct sbrec_port_binding *pb; > > + > > + struct ovs_list children; > > +}; > > + > > +static inline struct local_binding * > > +local_binding_find(struct shash *local_bindings, const char *name) > > +{ > > + return shash_find_data(local_bindings, name); > > +} > > + > > const struct ovsrec_bridge *get_bridge(const struct ovsrec_bridge_table > *, > > const char *br_name); > > > > diff --git a/controller/physical.c b/controller/physical.c > > index 144aeb7bd..03eb677d1 100644 > > --- a/controller/physical.c > > +++ b/controller/physical.c > > @@ -1780,6 +1780,57 @@ physical_run(struct physical_ctx *p_ctx, > > simap_destroy(&new_tunnel_to_ofport); > > } > > > > +bool > > +physical_handle_ovs_iface_changes(struct physical_ctx *p_ctx, > > + struct ovn_desired_flow_table > *flow_table) > > +{ > > + const struct ovsrec_interface *iface_rec; > > + OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec, > p_ctx->iface_table) { > > + if (!strcmp(iface_rec->type, "geneve") || > > + !strcmp(iface_rec->type, "patch")) { > > + return false; > > + } > > + } > > + > > + struct ofpbuf ofpacts; > > + ofpbuf_init(&ofpacts, 0); > > + > > + OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec, > p_ctx->iface_table) { > > + const char *iface_id = smap_get(&iface_rec->external_ids, > "iface-id"); > > + if (!iface_id) { > > + continue; > > + } > > + > > + const struct local_binding *lb = > > + local_binding_find(p_ctx->local_bindings, iface_id); > > + > > + if (!lb || !lb->pb) { > > + continue; > > + } > > + > > + int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; > > + if (ovsrec_interface_is_deleted(iface_rec)) { > > + ofctrl_remove_flows(flow_table, &lb->pb->header_.uuid); > > + simap_find_and_delete(&localvif_to_ofport, iface_id); > > If I understand correctly we should always be able to find this iface_id > in localvif_to_ofport, right? > > Should we log an error when it's not there? Or assert? > I missed this comment and didn't address it in v6. Yes, iface_id should be present in localvif_to_ofport. I think we could assert. But should it be very strict ? Thanks Numan > Thanks, > Dumitru > > > + } else { > > + if (!ovsrec_interface_is_new(iface_rec)) { > > + ofctrl_remove_flows(flow_table, &lb->pb->header_.uuid); > > + } > > + > > + simap_put(&localvif_to_ofport, iface_id, ofport); > > + consider_port_binding(p_ctx->sbrec_port_binding_by_name, > > + p_ctx->mff_ovn_geneve, > p_ctx->ct_zones, > > + p_ctx->active_tunnels, > > + p_ctx->local_datapaths, > > + lb->pb, p_ctx->chassis, > > + flow_table, &ofpacts); > > + } > > + } > > + > > + ofpbuf_uninit(&ofpacts); > > + return true; > > +} > > + > > bool > > get_tunnel_ofport(const char *chassis_name, char *encap_ip, ofp_port_t > *ofport) > > { > > diff --git a/controller/physical.h b/controller/physical.h > > index dadf84ea1..9ca34436a 100644 > > --- a/controller/physical.h > > +++ b/controller/physical.h > > @@ -49,11 +49,13 @@ struct physical_ctx { > > const struct ovsrec_bridge *br_int; > > const struct sbrec_chassis_table *chassis_table; > > const struct sbrec_chassis *chassis; > > + const struct ovsrec_interface_table *iface_table; > > const struct sset *active_tunnels; > > struct hmap *local_datapaths; > > struct sset *local_lports; > > const struct simap *ct_zones; > > enum mf_field_id mff_ovn_geneve; > > + struct shash *local_bindings; > > }; > > > > void physical_register_ovs_idl(struct ovsdb_idl *); > > @@ -63,7 +65,8 @@ void physical_handle_port_binding_changes(struct > physical_ctx *, > > struct ovn_desired_flow_table > *); > > void physical_handle_mc_group_changes(struct physical_ctx *, > > struct ovn_desired_flow_table *); > > - > > +bool physical_handle_ovs_iface_changes(struct physical_ctx *, > > + struct ovn_desired_flow_table *); > > bool get_tunnel_ofport(const char *chassis_name, char *encap_ip, > > ofp_port_t *ofport); > > #endif /* controller/physical.h */ > > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
