From 51df5a0f6c582011c0b9259994b4c5d62f2010ae Mon Sep 17 00:00:00 2001 From: Numan Siddique Date: Sat, 11 Jan 2020 16:14:36 +0530 Subject: [PATCH 1/9] Refactor binding_run() to take two context argument - binding_ctx_in and binding_ctx_out. No functional changes are introduced in this patch. Signed-off-by: Numan Siddique --- controller/binding.c | 263 +++++++++++++++++------------------- controller/binding.h | 39 ++++-- controller/ovn-controller.c | 120 +++++++++------- 3 files changed, 217 insertions(+), 205 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index 20a89d07de..007a94866d 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -441,12 +441,9 @@ static bool is_our_chassis(const struct sbrec_chassis *chassis_rec, const struct sbrec_port_binding *binding_rec, const struct sset *active_tunnels, - const struct shash *lport_to_iface, + const struct ovsrec_interface *iface_rec, const struct sset *local_lports) { - const struct ovsrec_interface *iface_rec - = shash_find_data(lport_to_iface, binding_rec->logical_port); - /* Ports of type "virtual" should never be explicitly bound to an OVS * port in the integration bridge. If that's the case, ignore the binding * and log a warning. @@ -490,78 +487,74 @@ is_our_chassis(const struct sbrec_chassis *chassis_rec, * additional processing. */ static const struct sbrec_port_binding ** -consider_local_datapath(struct ovsdb_idl_txn *ovnsb_idl_txn, - struct ovsdb_idl_txn *ovs_idl_txn, - struct ovsdb_idl_index *sbrec_datapath_binding_by_key, - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, - struct ovsdb_idl_index *sbrec_port_binding_by_name, - const struct sset *active_tunnels, - const struct sbrec_chassis *chassis_rec, - const struct sbrec_port_binding *binding_rec, +consider_local_datapath(const struct sbrec_port_binding *binding_rec, struct hmap *qos_map, - struct hmap *local_datapaths, - struct shash *lport_to_iface, - struct sset *local_lports, - struct sset *local_lport_ids, + const struct ovsrec_interface *iface_rec, + struct binding_ctx_in *b_ctx_in, + struct binding_ctx_out *b_ctx_out, const struct sbrec_port_binding **vpbs, size_t *n_vpbs, size_t *n_allocated_vpbs) { - const struct ovsrec_interface *iface_rec - = shash_find_data(lport_to_iface, binding_rec->logical_port); - - bool our_chassis = is_our_chassis(chassis_rec, binding_rec, active_tunnels, - lport_to_iface, local_lports); + bool our_chassis = is_our_chassis(b_ctx_in->chassis_rec, binding_rec, + b_ctx_in->active_tunnels, iface_rec, + b_ctx_out->local_lports); if (iface_rec || (binding_rec->parent_port && binding_rec->parent_port[0] && - sset_contains(local_lports, binding_rec->parent_port))) { + sset_contains(b_ctx_out->local_lports, + binding_rec->parent_port))) { if (binding_rec->parent_port && binding_rec->parent_port[0]) { /* Add child logical port to the set of all local ports. */ - sset_add(local_lports, binding_rec->logical_port); + sset_add(b_ctx_out->local_lports, binding_rec->logical_port); } - add_local_datapath(sbrec_datapath_binding_by_key, - sbrec_port_binding_by_datapath, - sbrec_port_binding_by_name, - binding_rec->datapath, false, local_datapaths); - if (iface_rec && qos_map && ovs_idl_txn) { + add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, + b_ctx_in->sbrec_port_binding_by_datapath, + b_ctx_in->sbrec_port_binding_by_name, + binding_rec->datapath, false, + b_ctx_out->local_datapaths); + if (iface_rec && qos_map && b_ctx_in->ovs_idl_txn) { get_qos_params(binding_rec, qos_map); } } else if (!strcmp(binding_rec->type, "l2gateway")) { if (our_chassis) { - sset_add(local_lports, binding_rec->logical_port); - add_local_datapath(sbrec_datapath_binding_by_key, - sbrec_port_binding_by_datapath, - sbrec_port_binding_by_name, - binding_rec->datapath, false, local_datapaths); + sset_add(b_ctx_out->local_lports, binding_rec->logical_port); + add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, + b_ctx_in->sbrec_port_binding_by_datapath, + b_ctx_in->sbrec_port_binding_by_name, + binding_rec->datapath, false, + b_ctx_out->local_datapaths); } } else if (!strcmp(binding_rec->type, "chassisredirect")) { if (ha_chassis_group_contains(binding_rec->ha_chassis_group, - chassis_rec)) { - add_local_datapath(sbrec_datapath_binding_by_key, - sbrec_port_binding_by_datapath, - sbrec_port_binding_by_name, - binding_rec->datapath, false, local_datapaths); + b_ctx_in->chassis_rec)) { + add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, + b_ctx_in->sbrec_port_binding_by_datapath, + b_ctx_in->sbrec_port_binding_by_name, + binding_rec->datapath, false, + b_ctx_out->local_datapaths); } } else if (!strcmp(binding_rec->type, "l3gateway")) { if (our_chassis) { - add_local_datapath(sbrec_datapath_binding_by_key, - sbrec_port_binding_by_datapath, - sbrec_port_binding_by_name, - binding_rec->datapath, true, local_datapaths); + add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, + b_ctx_in->sbrec_port_binding_by_datapath, + b_ctx_in->sbrec_port_binding_by_name, + binding_rec->datapath, true, + b_ctx_out->local_datapaths); } } else if (!strcmp(binding_rec->type, "localnet")) { /* Add all localnet ports to local_lports so that we allocate ct zones * for them. */ - sset_add(local_lports, binding_rec->logical_port); - if (qos_map && ovs_idl_txn) { + sset_add(b_ctx_out->local_lports, binding_rec->logical_port); + if (qos_map && b_ctx_in->ovs_idl_txn) { get_qos_params(binding_rec, qos_map); } } else if (!strcmp(binding_rec->type, "external")) { if (ha_chassis_group_contains(binding_rec->ha_chassis_group, - chassis_rec)) { - add_local_datapath(sbrec_datapath_binding_by_key, - sbrec_port_binding_by_datapath, - sbrec_port_binding_by_name, - binding_rec->datapath, false, local_datapaths); + b_ctx_in->chassis_rec)) { + add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, + b_ctx_in->sbrec_port_binding_by_datapath, + b_ctx_in->sbrec_port_binding_by_name, + binding_rec->datapath, false, + b_ctx_out->local_datapaths); } } @@ -570,65 +563,63 @@ consider_local_datapath(struct ovsdb_idl_txn *ovnsb_idl_txn, || !strcmp(binding_rec->type, "localport") || !strcmp(binding_rec->type, "vtep") || !strcmp(binding_rec->type, "localnet")) { - update_local_lport_ids(local_lport_ids, binding_rec); + update_local_lport_ids(b_ctx_out->local_lport_ids, binding_rec); } - ovs_assert(ovnsb_idl_txn); - if (ovnsb_idl_txn) { - const char *vif_chassis = smap_get(&binding_rec->options, + ovs_assert(b_ctx_in->ovnsb_idl_txn); + const char *vif_chassis = smap_get(&binding_rec->options, "requested-chassis"); - bool can_bind = !vif_chassis || !vif_chassis[0] - || !strcmp(vif_chassis, chassis_rec->name) - || !strcmp(vif_chassis, chassis_rec->hostname); - - if (can_bind && our_chassis) { - if (binding_rec->chassis != chassis_rec) { - if (binding_rec->chassis) { - VLOG_INFO("Changing chassis for lport %s from %s to %s.", - binding_rec->logical_port, - binding_rec->chassis->name, - chassis_rec->name); - } else { - VLOG_INFO("Claiming lport %s for this chassis.", - binding_rec->logical_port); - } - for (int i = 0; i < binding_rec->n_mac; i++) { - VLOG_INFO("%s: Claiming %s", - binding_rec->logical_port, binding_rec->mac[i]); - } - sbrec_port_binding_set_chassis(binding_rec, chassis_rec); + bool can_bind = !vif_chassis || !vif_chassis[0] + || !strcmp(vif_chassis, b_ctx_in->chassis_rec->name) + || !strcmp(vif_chassis, b_ctx_in->chassis_rec->hostname); + + if (can_bind && our_chassis) { + if (binding_rec->chassis != b_ctx_in->chassis_rec) { + if (binding_rec->chassis) { + VLOG_INFO("Changing chassis for lport %s from %s to %s.", + binding_rec->logical_port, + binding_rec->chassis->name, + b_ctx_in->chassis_rec->name); + } else { + VLOG_INFO("Claiming lport %s for this chassis.", + binding_rec->logical_port); } - /* Check if the port encap binding, if any, has changed */ - struct sbrec_encap *encap_rec = sbrec_get_port_encap( - chassis_rec, iface_rec); - if (encap_rec && binding_rec->encap != encap_rec) { - sbrec_port_binding_set_encap(binding_rec, encap_rec); + for (int i = 0; i < binding_rec->n_mac; i++) { + VLOG_INFO("%s: Claiming %s", + binding_rec->logical_port, binding_rec->mac[i]); } - } else if (binding_rec->chassis == chassis_rec) { - if (!strcmp(binding_rec->type, "virtual")) { - if (*n_vpbs == *n_allocated_vpbs) { - vpbs = x2nrealloc(vpbs, n_allocated_vpbs, sizeof *vpbs); - } - vpbs[(*n_vpbs)] = binding_rec; - (*n_vpbs)++; - } else { - VLOG_INFO("Releasing lport %s from this chassis.", - binding_rec->logical_port); - if (binding_rec->encap) { - sbrec_port_binding_set_encap(binding_rec, NULL); - } - sbrec_port_binding_set_chassis(binding_rec, NULL); + sbrec_port_binding_set_chassis(binding_rec, b_ctx_in->chassis_rec); + } + /* Check if the port encap binding, if any, has changed */ + struct sbrec_encap *encap_rec = + sbrec_get_port_encap(b_ctx_in->chassis_rec, iface_rec); + if (encap_rec && binding_rec->encap != encap_rec) { + sbrec_port_binding_set_encap(binding_rec, encap_rec); + } + } else if (binding_rec->chassis == b_ctx_in->chassis_rec) { + if (!strcmp(binding_rec->type, "virtual")) { + if (*n_vpbs == *n_allocated_vpbs) { + vpbs = x2nrealloc(vpbs, n_allocated_vpbs, sizeof *vpbs); } - } else if (our_chassis) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); - VLOG_INFO_RL(&rl, - "Not claiming lport %s, chassis %s " - "requested-chassis %s", - binding_rec->logical_port, - chassis_rec->name, - vif_chassis); + vpbs[(*n_vpbs)] = binding_rec; + (*n_vpbs)++; + } else { + VLOG_INFO("Releasing lport %s from this chassis.", + binding_rec->logical_port); + if (binding_rec->encap) { + sbrec_port_binding_set_encap(binding_rec, NULL); + } + sbrec_port_binding_set_chassis(binding_rec, NULL); } + } else if (our_chassis) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); + VLOG_INFO_RL(&rl, + "Not claiming lport %s, chassis %s " + "requested-chassis %s", + binding_rec->logical_port, b_ctx_in->chassis_rec->name, + vif_chassis); } + return vpbs; } @@ -722,23 +713,9 @@ consider_localnet_port(const struct sbrec_port_binding *binding_rec, } void -binding_run(struct ovsdb_idl_txn *ovnsb_idl_txn, - struct ovsdb_idl_txn *ovs_idl_txn, - struct ovsdb_idl_index *sbrec_datapath_binding_by_key, - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, - struct ovsdb_idl_index *sbrec_port_binding_by_name, - const struct ovsrec_port_table *port_table, - const struct ovsrec_qos_table *qos_table, - const struct sbrec_port_binding_table *port_binding_table, - const struct ovsrec_bridge *br_int, - const struct sbrec_chassis *chassis_rec, - const struct sset *active_tunnels, - const struct ovsrec_bridge_table *bridge_table, - const struct ovsrec_open_vswitch_table *ovs_table, - struct hmap *local_datapaths, struct sset *local_lports, - struct sset *local_lport_ids) +binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) { - if (!chassis_rec) { + if (!b_ctx_in->chassis_rec) { return; } @@ -749,9 +726,9 @@ binding_run(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap qos_map; hmap_init(&qos_map); - if (br_int) { - get_local_iface_ids(br_int, &lport_to_iface, local_lports, - &egress_ifaces); + if (b_ctx_in->br_int) { + get_local_iface_ids(b_ctx_in->br_int, &lport_to_iface, + b_ctx_out->local_lports, &egress_ifaces); } /* Array to store pointers to local virtual ports. It is populated by @@ -768,16 +745,14 @@ binding_run(struct ovsdb_idl_txn *ovnsb_idl_txn, * later. This is special case for virtual ports is needed in order to * make sure we update the virtual_parent port bindings first. */ - SBREC_PORT_BINDING_TABLE_FOR_EACH (binding_rec, port_binding_table) { + SBREC_PORT_BINDING_TABLE_FOR_EACH (binding_rec, + b_ctx_in->port_binding_table) { + const struct ovsrec_interface *iface_rec + = shash_find_data(&lport_to_iface, binding_rec->logical_port); vpbs = - consider_local_datapath(ovnsb_idl_txn, ovs_idl_txn, - sbrec_datapath_binding_by_key, - sbrec_port_binding_by_datapath, - sbrec_port_binding_by_name, - active_tunnels, chassis_rec, binding_rec, + consider_local_datapath(binding_rec, sset_is_empty(&egress_ifaces) ? NULL : - &qos_map, local_datapaths, &lport_to_iface, - local_lports, local_lport_ids, + &qos_map, iface_rec, b_ctx_in, b_ctx_out, vpbs, &n_vpbs, &n_allocated_vpbs); } @@ -785,26 +760,29 @@ binding_run(struct ovsdb_idl_txn *ovnsb_idl_txn, * updated above. */ for (size_t i = 0; i < n_vpbs; i++) { - consider_local_virtual_port(sbrec_port_binding_by_name, chassis_rec, - vpbs[i]); + consider_local_virtual_port(b_ctx_in->sbrec_port_binding_by_name, + b_ctx_in->chassis_rec, vpbs[i]); } free(vpbs); - add_ovs_bridge_mappings(ovs_table, bridge_table, &bridge_mappings); + add_ovs_bridge_mappings(b_ctx_in->ovs_table, b_ctx_in->bridge_table, + &bridge_mappings); /* Run through each binding record to see if it is a localnet port * on local datapaths discovered from above loop, and update the * corresponding local datapath accordingly. */ - SBREC_PORT_BINDING_TABLE_FOR_EACH (binding_rec, port_binding_table) { + SBREC_PORT_BINDING_TABLE_FOR_EACH (binding_rec, + b_ctx_in->port_binding_table) { if (!strcmp(binding_rec->type, "localnet")) { consider_localnet_port(binding_rec, &bridge_mappings, - &egress_ifaces, local_datapaths); + &egress_ifaces, b_ctx_out->local_datapaths); } } shash_destroy(&bridge_mappings); if (!sset_is_empty(&egress_ifaces) - && set_noop_qos(ovs_idl_txn, port_table, qos_table, &egress_ifaces)) { + && set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, + b_ctx_in->qos_table, &egress_ifaces)) { const char *entry; SSET_FOR_EACH (entry, &egress_ifaces) { setup_qos(entry, &qos_map); @@ -850,13 +828,18 @@ binding_evaluate_port_binding_changes( * - If a regular VIF is unbound from this chassis, the local ovsdb * interface table will be updated, which will trigger recompute. * - * - If the port is not a regular VIF, and not a "remote" port, - * always trigger recompute. */ - if (binding_rec->chassis == chassis_rec - || is_our_chassis(chassis_rec, binding_rec, - active_tunnels, &lport_to_iface, local_lports) - || (strcmp(binding_rec->type, "") && strcmp(binding_rec->type, - "remote"))) { + * - If the port is not a regular VIF, always trigger recompute. */ + if (binding_rec->chassis == chassis_rec) { + changed = true; + break; + } + + const struct ovsrec_interface *iface_rec + = shash_find_data(&lport_to_iface, binding_rec->logical_port); + if (is_our_chassis(chassis_rec, binding_rec, active_tunnels, iface_rec, + local_lports) || + (strcmp(binding_rec->type, "") && strcmp(binding_rec->type, + "remote"))) { changed = true; break; } diff --git a/controller/binding.h b/controller/binding.h index 924891c1b4..d0b8331af5 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -32,22 +32,31 @@ struct sbrec_chassis; struct sbrec_port_binding_table; struct sset; +struct binding_ctx_in { + struct ovsdb_idl_txn *ovnsb_idl_txn; + struct ovsdb_idl_txn *ovs_idl_txn; + struct ovsdb_idl_index *sbrec_datapath_binding_by_key; + struct ovsdb_idl_index *sbrec_port_binding_by_datapath; + struct ovsdb_idl_index *sbrec_port_binding_by_name; + const struct ovsrec_port_table *port_table; + const struct ovsrec_qos_table *qos_table; + const struct sbrec_port_binding_table *port_binding_table; + const struct ovsrec_bridge *br_int; + const struct sbrec_chassis *chassis_rec; + const struct sset *active_tunnels; + const struct ovsrec_bridge_table *bridge_table; + const struct ovsrec_open_vswitch_table *ovs_table; + const struct ovsrec_interface_table *iface_table; +}; + +struct binding_ctx_out { + struct hmap *local_datapaths; + struct sset *local_lports; + struct sset *local_lport_ids; +}; + void binding_register_ovs_idl(struct ovsdb_idl *); -void binding_run(struct ovsdb_idl_txn *ovnsb_idl_txn, - struct ovsdb_idl_txn *ovs_idl_txn, - struct ovsdb_idl_index *sbrec_datapath_binding_by_key, - struct ovsdb_idl_index *sbrec_port_binding_by_datapath, - struct ovsdb_idl_index *sbrec_port_binding_by_name, - const struct ovsrec_port_table *, - const struct ovsrec_qos_table *, - const struct sbrec_port_binding_table *, - const struct ovsrec_bridge *br_int, - const struct sbrec_chassis *, - const struct sset *active_tunnels, - const struct ovsrec_bridge_table *bridge_table, - const struct ovsrec_open_vswitch_table *ovs_table, - struct hmap *local_datapaths, - struct sset *local_lports, struct sset *local_lport_ids); +void binding_run(struct binding_ctx_in *, struct binding_ctx_out *); bool binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, const struct sbrec_port_binding_table *, const struct sbrec_chassis *); diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 2893eaac19..4930d5ffb0 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1003,34 +1003,11 @@ en_runtime_data_cleanup(void *data) } static void -en_runtime_data_run(struct engine_node *node, void *data) +init_binding_ctx(struct engine_node *node, + struct ed_type_runtime_data *rt_data, + struct binding_ctx_in *b_ctx_in, + struct binding_ctx_out *b_ctx_out) { - struct ed_type_runtime_data *rt_data = data; - struct hmap *local_datapaths = &rt_data->local_datapaths; - struct sset *local_lports = &rt_data->local_lports; - struct sset *local_lport_ids = &rt_data->local_lport_ids; - struct sset *active_tunnels = &rt_data->active_tunnels; - - static bool first_run = true; - if (first_run) { - /* don't cleanup since there is no data yet */ - first_run = false; - } else { - struct local_datapath *cur_node, *next_node; - HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node, local_datapaths) { - free(cur_node->peer_ports); - hmap_remove(local_datapaths, &cur_node->hmap_node); - free(cur_node); - } - hmap_clear(local_datapaths); - sset_destroy(local_lports); - sset_destroy(local_lport_ids); - sset_destroy(active_tunnels); - sset_init(local_lports); - sset_init(local_lport_ids); - sset_init(active_tunnels); - } - struct ovsrec_open_vswitch_table *ovs_table = (struct ovsrec_open_vswitch_table *)EN_OVSDB_GET( engine_get_input("OVS_open_vswitch", node)); @@ -1051,19 +1028,6 @@ en_runtime_data_run(struct engine_node *node, void *data) = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); ovs_assert(chassis); - struct ed_type_ofctrl_is_connected *ed_ofctrl_is_connected = - engine_get_input_data("ofctrl_is_connected", node); - if (ed_ofctrl_is_connected->connected) { - /* Calculate the active tunnels only if have an an active - * OpenFlow connection to br-int. - * If we don't have a connection to br-int, it could mean - * ovs-vswitchd is down for some reason and the BFD status - * in the Interface rows could be stale. So its better to - * consider 'active_tunnels' set to be empty if it's not - * connected. */ - bfd_calculate_active_tunnels(br_int, active_tunnels); - } - struct ovsrec_port_table *port_table = (struct ovsrec_port_table *)EN_OVSDB_GET( engine_get_input("OVS_port", node)); @@ -1091,16 +1055,72 @@ en_runtime_data_run(struct engine_node *node, void *data) engine_get_input("SB_port_binding", node), "datapath"); - binding_run(engine_get_context()->ovnsb_idl_txn, - engine_get_context()->ovs_idl_txn, - sbrec_datapath_binding_by_key, - sbrec_port_binding_by_datapath, - sbrec_port_binding_by_name, - port_table, qos_table, pb_table, - br_int, chassis, - active_tunnels, bridge_table, - ovs_table, local_datapaths, - local_lports, local_lport_ids); + b_ctx_in->ovnsb_idl_txn = engine_get_context()->ovnsb_idl_txn; + b_ctx_in->ovs_idl_txn = engine_get_context()->ovs_idl_txn; + b_ctx_in->sbrec_datapath_binding_by_key = sbrec_datapath_binding_by_key; + b_ctx_in->sbrec_port_binding_by_datapath = sbrec_port_binding_by_datapath; + b_ctx_in->sbrec_port_binding_by_name = sbrec_port_binding_by_name; + b_ctx_in->port_table = port_table; + b_ctx_in->qos_table = qos_table; + b_ctx_in->port_binding_table = pb_table; + b_ctx_in->br_int = br_int; + b_ctx_in->chassis_rec = chassis; + b_ctx_in->active_tunnels = &rt_data->active_tunnels; + b_ctx_in->bridge_table = bridge_table; + b_ctx_in->ovs_table = ovs_table; + + b_ctx_out->local_datapaths = &rt_data->local_datapaths; + b_ctx_out->local_lports = &rt_data->local_lports; + b_ctx_out->local_lport_ids = &rt_data->local_lport_ids; +} + +static void +en_runtime_data_run(struct engine_node *node, void *data) +{ + struct ed_type_runtime_data *rt_data = data; + struct hmap *local_datapaths = &rt_data->local_datapaths; + struct sset *local_lports = &rt_data->local_lports; + struct sset *local_lport_ids = &rt_data->local_lport_ids; + struct sset *active_tunnels = &rt_data->active_tunnels; + + static bool first_run = true; + if (first_run) { + /* don't cleanup since there is no data yet */ + first_run = false; + } else { + struct local_datapath *cur_node, *next_node; + HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node, local_datapaths) { + free(cur_node->peer_ports); + hmap_remove(local_datapaths, &cur_node->hmap_node); + free(cur_node); + } + hmap_clear(local_datapaths); + sset_destroy(local_lports); + sset_destroy(local_lport_ids); + sset_destroy(active_tunnels); + sset_init(local_lports); + sset_init(local_lport_ids); + sset_init(active_tunnels); + } + + struct binding_ctx_in b_ctx_in; + struct binding_ctx_out b_ctx_out; + init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out); + + struct ed_type_ofctrl_is_connected *ed_ofctrl_is_connected = + engine_get_input_data("ofctrl_is_connected", node); + if (ed_ofctrl_is_connected->connected) { + /* Calculate the active tunnels only if have an an active + * OpenFlow connection to br-int. + * If we don't have a connection to br-int, it could mean + * ovs-vswitchd is down for some reason and the BFD status + * in the Interface rows could be stale. So its better to + * consider 'active_tunnels' set to be empty if it's not + * connected. */ + bfd_calculate_active_tunnels(b_ctx_in.br_int, active_tunnels); + } + + binding_run(&b_ctx_in, &b_ctx_out); engine_set_node_state(node, EN_UPDATED); } From 0638e0cad56a060a1073b401421cbab6e7756733 Mon Sep 17 00:00:00 2001 From: Numan Siddique Date: Mon, 20 Jan 2020 00:02:54 +0530 Subject: [PATCH 2/9] ovn-controller: Store the local port bindings in the runtime data I-P state. This patch adds a new data structure - 'struct local_binding' which represents a probable local port binding. A new object of this structure is creared for each interface present in the integration bridge (br-int) with the external_ids:iface-id set. This struct has 2 main fields - 'struct ovsrec_interface *' and 'struct sbrec_port_binding *'. These fields are updated during port claim and release. A shash of the local bindings is maintained with the 'iface-id' as the hash key in the runtime data of the incremental processing engine. This patch helps in the upcoming patch to add incremental processing support for OVS interface and SB port binding changes. Signed-off-by: Numan Siddique --- controller/binding.c | 722 ++++++++++++++++++++++-------------- controller/binding.h | 16 +- controller/ovn-controller.c | 46 +-- controller/pinctrl.c | 1 + controller/pinctrl.h | 1 + 5 files changed, 467 insertions(+), 319 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index 007a94866d..66f4f65e30 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -69,47 +69,6 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl) ovsdb_idl_add_column(ovs_idl, &ovsrec_qos_col_type); } -static void -get_local_iface_ids(const struct ovsrec_bridge *br_int, - struct shash *lport_to_iface, - struct sset *local_lports, - struct sset *egress_ifaces) -{ - int i; - - for (i = 0; i < br_int->n_ports; i++) { - const struct ovsrec_port *port_rec = br_int->ports[i]; - const char *iface_id; - int j; - - if (!strcmp(port_rec->name, br_int->name)) { - continue; - } - - for (j = 0; j < port_rec->n_interfaces; j++) { - const struct ovsrec_interface *iface_rec; - - iface_rec = port_rec->interfaces[j]; - iface_id = smap_get(&iface_rec->external_ids, "iface-id"); - int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; - - if (iface_id && ofport > 0) { - shash_add(lport_to_iface, iface_id, iface_rec); - sset_add(local_lports, iface_id); - } - - /* Check if this is a tunnel interface. */ - if (smap_get(&iface_rec->options, "remote_ip")) { - const char *tunnel_iface - = smap_get(&iface_rec->status, "tunnel_egress_iface"); - if (tunnel_iface) { - sset_add(egress_ifaces, tunnel_iface); - } - } - } - } -} - static void add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, struct ovsdb_idl_index *sbrec_port_binding_by_datapath, @@ -441,25 +400,11 @@ static bool is_our_chassis(const struct sbrec_chassis *chassis_rec, const struct sbrec_port_binding *binding_rec, const struct sset *active_tunnels, - const struct ovsrec_interface *iface_rec, const struct sset *local_lports) { - /* Ports of type "virtual" should never be explicitly bound to an OVS - * port in the integration bridge. If that's the case, ignore the binding - * and log a warning. - */ - if (iface_rec && !strcmp(binding_rec->type, "virtual")) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); - VLOG_WARN_RL(&rl, - "Virtual port %s should not be bound to OVS port %s", - binding_rec->logical_port, iface_rec->name); - return false; - } - bool our_chassis = false; - if (iface_rec - || (binding_rec->parent_port && binding_rec->parent_port[0] && - sset_contains(local_lports, binding_rec->parent_port))) { + if (binding_rec->parent_port && binding_rec->parent_port[0] && + sset_contains(local_lports, binding_rec->parent_port)) { /* This port is in our chassis unless it is a localport. */ our_chassis = strcmp(binding_rec->type, "localport"); } else if (!strcmp(binding_rec->type, "l2gateway")) { @@ -481,175 +426,6 @@ is_our_chassis(const struct sbrec_chassis *chassis_rec, return our_chassis; } -/* Updates 'binding_rec' and if the port binding is local also updates the - * local datapaths and ports. - * Updates and returns the array of local virtual ports that will require - * additional processing. - */ -static const struct sbrec_port_binding ** -consider_local_datapath(const struct sbrec_port_binding *binding_rec, - struct hmap *qos_map, - const struct ovsrec_interface *iface_rec, - struct binding_ctx_in *b_ctx_in, - struct binding_ctx_out *b_ctx_out, - const struct sbrec_port_binding **vpbs, - size_t *n_vpbs, size_t *n_allocated_vpbs) -{ - bool our_chassis = is_our_chassis(b_ctx_in->chassis_rec, binding_rec, - b_ctx_in->active_tunnels, iface_rec, - b_ctx_out->local_lports); - if (iface_rec - || (binding_rec->parent_port && binding_rec->parent_port[0] && - sset_contains(b_ctx_out->local_lports, - binding_rec->parent_port))) { - if (binding_rec->parent_port && binding_rec->parent_port[0]) { - /* Add child logical port to the set of all local ports. */ - sset_add(b_ctx_out->local_lports, binding_rec->logical_port); - } - add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, - b_ctx_in->sbrec_port_binding_by_datapath, - b_ctx_in->sbrec_port_binding_by_name, - binding_rec->datapath, false, - b_ctx_out->local_datapaths); - if (iface_rec && qos_map && b_ctx_in->ovs_idl_txn) { - get_qos_params(binding_rec, qos_map); - } - } else if (!strcmp(binding_rec->type, "l2gateway")) { - if (our_chassis) { - sset_add(b_ctx_out->local_lports, binding_rec->logical_port); - add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, - b_ctx_in->sbrec_port_binding_by_datapath, - b_ctx_in->sbrec_port_binding_by_name, - binding_rec->datapath, false, - b_ctx_out->local_datapaths); - } - } else if (!strcmp(binding_rec->type, "chassisredirect")) { - if (ha_chassis_group_contains(binding_rec->ha_chassis_group, - b_ctx_in->chassis_rec)) { - add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, - b_ctx_in->sbrec_port_binding_by_datapath, - b_ctx_in->sbrec_port_binding_by_name, - binding_rec->datapath, false, - b_ctx_out->local_datapaths); - } - } else if (!strcmp(binding_rec->type, "l3gateway")) { - if (our_chassis) { - add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, - b_ctx_in->sbrec_port_binding_by_datapath, - b_ctx_in->sbrec_port_binding_by_name, - binding_rec->datapath, true, - b_ctx_out->local_datapaths); - } - } else if (!strcmp(binding_rec->type, "localnet")) { - /* Add all localnet ports to local_lports so that we allocate ct zones - * for them. */ - sset_add(b_ctx_out->local_lports, binding_rec->logical_port); - if (qos_map && b_ctx_in->ovs_idl_txn) { - get_qos_params(binding_rec, qos_map); - } - } else if (!strcmp(binding_rec->type, "external")) { - if (ha_chassis_group_contains(binding_rec->ha_chassis_group, - b_ctx_in->chassis_rec)) { - add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, - b_ctx_in->sbrec_port_binding_by_datapath, - b_ctx_in->sbrec_port_binding_by_name, - binding_rec->datapath, false, - b_ctx_out->local_datapaths); - } - } - - if (our_chassis - || !strcmp(binding_rec->type, "patch") - || !strcmp(binding_rec->type, "localport") - || !strcmp(binding_rec->type, "vtep") - || !strcmp(binding_rec->type, "localnet")) { - update_local_lport_ids(b_ctx_out->local_lport_ids, binding_rec); - } - - ovs_assert(b_ctx_in->ovnsb_idl_txn); - const char *vif_chassis = smap_get(&binding_rec->options, - "requested-chassis"); - bool can_bind = !vif_chassis || !vif_chassis[0] - || !strcmp(vif_chassis, b_ctx_in->chassis_rec->name) - || !strcmp(vif_chassis, b_ctx_in->chassis_rec->hostname); - - if (can_bind && our_chassis) { - if (binding_rec->chassis != b_ctx_in->chassis_rec) { - if (binding_rec->chassis) { - VLOG_INFO("Changing chassis for lport %s from %s to %s.", - binding_rec->logical_port, - binding_rec->chassis->name, - b_ctx_in->chassis_rec->name); - } else { - VLOG_INFO("Claiming lport %s for this chassis.", - binding_rec->logical_port); - } - for (int i = 0; i < binding_rec->n_mac; i++) { - VLOG_INFO("%s: Claiming %s", - binding_rec->logical_port, binding_rec->mac[i]); - } - sbrec_port_binding_set_chassis(binding_rec, b_ctx_in->chassis_rec); - } - /* Check if the port encap binding, if any, has changed */ - struct sbrec_encap *encap_rec = - sbrec_get_port_encap(b_ctx_in->chassis_rec, iface_rec); - if (encap_rec && binding_rec->encap != encap_rec) { - sbrec_port_binding_set_encap(binding_rec, encap_rec); - } - } else if (binding_rec->chassis == b_ctx_in->chassis_rec) { - if (!strcmp(binding_rec->type, "virtual")) { - if (*n_vpbs == *n_allocated_vpbs) { - vpbs = x2nrealloc(vpbs, n_allocated_vpbs, sizeof *vpbs); - } - vpbs[(*n_vpbs)] = binding_rec; - (*n_vpbs)++; - } else { - VLOG_INFO("Releasing lport %s from this chassis.", - binding_rec->logical_port); - if (binding_rec->encap) { - sbrec_port_binding_set_encap(binding_rec, NULL); - } - sbrec_port_binding_set_chassis(binding_rec, NULL); - } - } else if (our_chassis) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); - VLOG_INFO_RL(&rl, - "Not claiming lport %s, chassis %s " - "requested-chassis %s", - binding_rec->logical_port, b_ctx_in->chassis_rec->name, - vif_chassis); - } - - return vpbs; -} - -static void -consider_local_virtual_port(struct ovsdb_idl_index *sbrec_port_binding_by_name, - const struct sbrec_chassis *chassis_rec, - const struct sbrec_port_binding *binding_rec) -{ - if (binding_rec->virtual_parent) { - const struct sbrec_port_binding *parent = - lport_lookup_by_name(sbrec_port_binding_by_name, - binding_rec->virtual_parent); - if (parent && parent->chassis == chassis_rec) { - return; - } - } - - /* pinctrl module takes care of binding the ports of type 'virtual'. - * Release such ports if their virtual parents are no longer claimed by - * this chassis. - */ - VLOG_INFO("Releasing lport %s from this chassis.", - binding_rec->logical_port); - if (binding_rec->encap) { - sbrec_port_binding_set_encap(binding_rec, NULL); - } - sbrec_port_binding_set_chassis(binding_rec, NULL); - sbrec_port_binding_set_virtual_parent(binding_rec, NULL); -} - static void add_localnet_egress_interface_mappings( const struct sbrec_port_binding *port_binding, @@ -712,6 +488,374 @@ consider_localnet_port(const struct sbrec_port_binding *binding_rec, ld->localnet_port = binding_rec; } +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, + enum local_binding_type type) +{ + struct local_binding *lbinding = xzalloc(sizeof *lbinding); + lbinding->name = xstrdup(name); + lbinding->type = type; + lbinding->pb = pb; + lbinding->iface = iface; + ovs_list_init(&lbinding->children); + return lbinding; +} + +static void +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); +} + +static void +local_binding_destroy(struct local_binding *lbinding) +{ + struct local_binding *c, *next; + LIST_FOR_EACH_SAFE (c, next, node, &lbinding->children) { + ovs_list_remove(&c->node); + free(c->name); + free(c); + } + free(lbinding->name); + free(lbinding); +} + +void +local_bindings_destroy(struct shash *local_bindings) +{ + struct shash_node *node, *next; + SHASH_FOR_EACH_SAFE (node, next, local_bindings) { + struct local_binding *lbinding = node->data; + struct local_binding *c, *n; + LIST_FOR_EACH_SAFE (c, n, node, &lbinding->children) { + ovs_list_remove(&c->node); + free(c->name); + free(c); + } + } + + shash_destroy(local_bindings); +} + +static void +local_binding_add_child(struct local_binding *lbinding, + struct local_binding *child) +{ + struct local_binding *l; + LIST_FOR_EACH (l, node, &lbinding->children) { + if (l == child) { + return; + } + } + + ovs_list_push_back(&lbinding->children, &child->node); +} + +static struct local_binding * +local_binding_find_child(struct local_binding *lbinding, + const char *child_name) +{ + struct local_binding *l; + LIST_FOR_EACH (l, node, &lbinding->children) { + if (!strcmp(l->name, child_name)) { + return l; + } + } + + return NULL; +} + +void +binding_add_vport_to_local_bindings(struct shash *local_bindings, + const struct sbrec_port_binding *parent, + const struct sbrec_port_binding *vport) +{ + struct local_binding *lbinding = local_binding_find(local_bindings, + parent->logical_port); + ovs_assert(lbinding); + struct local_binding *vbinding = + local_binding_find_child(lbinding, vport->logical_port); + if (!vbinding) { + vbinding = local_binding_create(vport->logical_port, lbinding->iface, + vport, BT_VIRTUAL); + local_binding_add_child(lbinding, vbinding); + } else { + vbinding->type = BT_VIRTUAL; + } +} + +static void +claim_lport(const struct sbrec_port_binding *pb, + const struct sbrec_chassis *chassis_rec, + const struct ovsrec_interface *iface_rec) +{ + if (pb->chassis != chassis_rec) { + if (pb->chassis) { + VLOG_INFO("Changing chassis for lport %s from %s to %s.", + pb->logical_port, pb->chassis->name, + chassis_rec->name); + } else { + VLOG_INFO("Claiming lport %s for this chassis.", pb->logical_port); + } + for (int i = 0; i < pb->n_mac; i++) { + VLOG_INFO("%s: Claiming %s", pb->logical_port, pb->mac[i]); + } + + sbrec_port_binding_set_chassis(pb, chassis_rec); + } + + /* Check if the port encap binding, if any, has changed */ + struct sbrec_encap *encap_rec = + sbrec_get_port_encap(chassis_rec, iface_rec); + if (encap_rec && pb->encap != encap_rec) { + sbrec_port_binding_set_encap(pb, encap_rec); + } +} + +static void +release_lport(const struct sbrec_port_binding *pb) +{ + VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port); + if (pb->encap) { + sbrec_port_binding_set_encap(pb, NULL); + } + sbrec_port_binding_set_chassis(pb, NULL); + + if (pb->virtual_parent) { + sbrec_port_binding_set_virtual_parent(pb, NULL); + } +} + +static void +consider_port_binding_for_vif(const struct sbrec_port_binding *pb, + struct binding_ctx_in *b_ctx_in, + enum local_binding_type binding_type, + struct local_binding *lbinding, + struct binding_ctx_out *b_ctx_out, + struct hmap *qos_map) +{ + const char *vif_chassis = smap_get(&pb->options, "requested-chassis"); + bool can_bind = !vif_chassis || !vif_chassis[0] + || !strcmp(vif_chassis, b_ctx_in->chassis_rec->name) + || !strcmp(vif_chassis, b_ctx_in->chassis_rec->hostname); + + /* Ports of type "virtual" should never be explicitly bound to an OVS + * port in the integration bridge. If that's the case, ignore the binding + * and log a warning. + */ + if (!strcmp(pb->type, "virtual") && lbinding && lbinding->iface && + binding_type == BT_VIF) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); + VLOG_WARN_RL(&rl, + "Virtual port %s should not be bound to OVS port %s", + pb->logical_port, lbinding->iface->name); + lbinding->pb = NULL; + return; + } + + if (lbinding && lbinding->pb && can_bind) { + claim_lport(pb, b_ctx_in->chassis_rec, lbinding->iface); + + switch (binding_type) { + case BT_VIF: + lbinding->pb = pb; + break; + case BT_CHILD: + case BT_VIRTUAL: + { + /* Add child logical port to the set of all local ports. */ + sset_add(b_ctx_out->local_lports, pb->logical_port); + struct local_binding *child = + local_binding_find_child(lbinding, pb->logical_port); + if (!child) { + child = local_binding_create(pb->logical_port, lbinding->iface, + pb, binding_type); + local_binding_add_child(lbinding, child); + } else { + ovs_assert(child->type == BT_CHILD || + child->type == BT_VIRTUAL); + child->pb = pb; + child->iface = lbinding->iface; + } + break; + } + default: + OVS_NOT_REACHED(); + } + + add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, + b_ctx_in->sbrec_port_binding_by_datapath, + b_ctx_in->sbrec_port_binding_by_name, + pb->datapath, false, b_ctx_out->local_datapaths); + update_local_lport_ids(b_ctx_out->local_lport_ids, pb); + if (lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { + get_qos_params(pb, qos_map); + } + } else if (lbinding && lbinding->pb && !can_bind) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); + VLOG_INFO_RL(&rl, + "Not claiming lport %s, chassis %s " + "requested-chassis %s", + pb->logical_port, + b_ctx_in->chassis_rec->name, + vif_chassis); + } + + if (pb->chassis == b_ctx_in->chassis_rec) { + if (!lbinding || !lbinding->pb || !can_bind) { + release_lport(pb); + } + } +} + +static void +consider_port_binding(const struct sbrec_port_binding *pb, + struct binding_ctx_in *b_ctx_in, + struct binding_ctx_out *b_ctx_out, + struct hmap *qos_map) +{ + + bool our_chassis = is_our_chassis(b_ctx_in->chassis_rec, pb, + b_ctx_in->active_tunnels, + b_ctx_out->local_lports); + + if (!strcmp(pb->type, "l2gateway")) { + if (our_chassis) { + sset_add(b_ctx_out->local_lports, pb->logical_port); + add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, + b_ctx_in->sbrec_port_binding_by_datapath, + b_ctx_in->sbrec_port_binding_by_name, + pb->datapath, false, + b_ctx_out->local_datapaths); + } + } else if (!strcmp(pb->type, "chassisredirect")) { + if (ha_chassis_group_contains(pb->ha_chassis_group, + b_ctx_in->chassis_rec)) { + add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, + b_ctx_in->sbrec_port_binding_by_datapath, + b_ctx_in->sbrec_port_binding_by_name, + pb->datapath, false, + b_ctx_out->local_datapaths); + } + } else if (!strcmp(pb->type, "l3gateway")) { + if (our_chassis) { + add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, + b_ctx_in->sbrec_port_binding_by_datapath, + b_ctx_in->sbrec_port_binding_by_name, + pb->datapath, true, b_ctx_out->local_datapaths); + } + } else if (!strcmp(pb->type, "localnet")) { + /* Add all localnet ports to local_lports so that we allocate ct zones + * for them. */ + sset_add(b_ctx_out->local_lports, pb->logical_port); + if (qos_map && b_ctx_in->ovs_idl_txn) { + get_qos_params(pb, qos_map); + } + } else if (!strcmp(pb->type, "external")) { + if (ha_chassis_group_contains(pb->ha_chassis_group, + b_ctx_in->chassis_rec)) { + add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, + b_ctx_in->sbrec_port_binding_by_datapath, + b_ctx_in->sbrec_port_binding_by_name, + pb->datapath, false, + b_ctx_out->local_datapaths); + } + } + + if (our_chassis || !strcmp(pb->type, "localnet")) { + update_local_lport_ids(b_ctx_out->local_lport_ids, pb); + } + + ovs_assert(b_ctx_in->ovnsb_idl_txn); + if (our_chassis) { + claim_lport(pb, b_ctx_in->chassis_rec, NULL); + } else if (pb->chassis == b_ctx_in->chassis_rec) { + release_lport(pb); + } +} + +static void +build_local_bindings_from_local_ifaces(struct binding_ctx_in *b_ctx_in, + struct binding_ctx_out *b_ctx_out) +{ + int i; + for (i = 0; i < b_ctx_in->br_int->n_ports; i++) { + const struct ovsrec_port *port_rec = b_ctx_in->br_int->ports[i]; + const char *iface_id; + int j; + + if (!strcmp(port_rec->name, b_ctx_in->br_int->name)) { + continue; + } + + for (j = 0; j < port_rec->n_interfaces; j++) { + const struct ovsrec_interface *iface_rec; + + iface_rec = port_rec->interfaces[j]; + iface_id = smap_get(&iface_rec->external_ids, "iface-id"); + int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; + + if (iface_id && ofport > 0) { + const struct sbrec_port_binding *pb + = lport_lookup_by_name( + b_ctx_in->sbrec_port_binding_by_name, iface_id); + struct local_binding *lbinding = + local_binding_find(b_ctx_out->local_bindings, iface_id); + if (!lbinding) { + lbinding = local_binding_create(iface_id, iface_rec, pb, + BT_VIF); + local_binding_add(b_ctx_out->local_bindings, lbinding); + } else { + lbinding->type = BT_VIF; + lbinding->pb = pb; + } + sset_add(b_ctx_out->local_lports, iface_id); + } + + /* Check if this is a tunnel interface. */ + if (smap_get(&iface_rec->options, "remote_ip")) { + const char *tunnel_iface + = smap_get(&iface_rec->status, "tunnel_egress_iface"); + if (tunnel_iface) { + sset_add(b_ctx_out->egress_ifaces, tunnel_iface); + } + } + } + } + + struct shash_node *node, *next; + SHASH_FOR_EACH_SAFE (node, next, b_ctx_out->local_bindings) { + struct local_binding *lbinding = node->data; + if (!sset_contains(b_ctx_out->local_lports, lbinding->name)) { + local_binding_destroy(lbinding); + shash_delete(b_ctx_out->local_bindings, node); + } + } +} + void binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) { @@ -721,49 +865,62 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) const struct sbrec_port_binding *binding_rec; struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); - struct shash lport_to_iface = SHASH_INITIALIZER(&lport_to_iface); - struct sset egress_ifaces = SSET_INITIALIZER(&egress_ifaces); struct hmap qos_map; hmap_init(&qos_map); if (b_ctx_in->br_int) { - get_local_iface_ids(b_ctx_in->br_int, &lport_to_iface, - b_ctx_out->local_lports, &egress_ifaces); + build_local_bindings_from_local_ifaces(b_ctx_in, b_ctx_out); } - /* Array to store pointers to local virtual ports. It is populated by - * consider_local_datapath. - */ - const struct sbrec_port_binding **vpbs = NULL; - size_t n_vpbs = 0; - size_t n_allocated_vpbs = 0; + struct hmap *qos_map_ptr = + !sset_is_empty(b_ctx_out->egress_ifaces) ? &qos_map : NULL; /* Run through each binding record to see if it is resident on this * chassis and update the binding accordingly. This includes both - * directly connected logical ports and children of those ports. - * Virtual ports are just added to vpbs array and will be processed - * later. This is special case for virtual ports is needed in order to - * make sure we update the virtual_parent port bindings first. + * directly connected logical ports and children of those ports + * (which also includes virtual ports). */ SBREC_PORT_BINDING_TABLE_FOR_EACH (binding_rec, b_ctx_in->port_binding_table) { - const struct ovsrec_interface *iface_rec - = shash_find_data(&lport_to_iface, binding_rec->logical_port); - vpbs = - consider_local_datapath(binding_rec, - sset_is_empty(&egress_ifaces) ? NULL : - &qos_map, iface_rec, b_ctx_in, b_ctx_out, - vpbs, &n_vpbs, &n_allocated_vpbs); - } + if (!strcmp(binding_rec->type, "patch") || + !strcmp(binding_rec->type, "localport") || + !strcmp(binding_rec->type, "vtep")) { + update_local_lport_ids(b_ctx_out->local_lport_ids, binding_rec); + continue; + } - /* Now also update the virtual ports in case their parent ports were - * updated above. - */ - for (size_t i = 0; i < n_vpbs; i++) { - consider_local_virtual_port(b_ctx_in->sbrec_port_binding_by_name, - b_ctx_in->chassis_rec, vpbs[i]); + bool consider_for_vif = false; + struct local_binding *lbinding = NULL; + enum local_binding_type binding_type = BT_VIF; + if (!binding_rec->type[0]) { + if (!binding_rec->parent_port || !binding_rec->parent_port[0]) { + lbinding = local_binding_find(b_ctx_out->local_bindings, + binding_rec->logical_port); + } else { + lbinding = local_binding_find(b_ctx_out->local_bindings, + binding_rec->parent_port); + binding_type = BT_CHILD; + } + + consider_for_vif = true; + } else if (!strcmp(binding_rec->type, "virtual") && + binding_rec->virtual_parent && + binding_rec->virtual_parent[0]) { + lbinding = local_binding_find(b_ctx_out->local_bindings, + binding_rec->virtual_parent); + consider_for_vif = true; + binding_type = BT_VIRTUAL; + } + + if (consider_for_vif) { + consider_port_binding_for_vif(binding_rec, b_ctx_in, + binding_type, lbinding, b_ctx_out, + qos_map_ptr); + } else { + consider_port_binding(binding_rec, b_ctx_in, b_ctx_out, + qos_map_ptr); + } } - free(vpbs); add_ovs_bridge_mappings(b_ctx_in->ovs_table, b_ctx_in->bridge_table, &bridge_mappings); @@ -775,49 +932,39 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) b_ctx_in->port_binding_table) { if (!strcmp(binding_rec->type, "localnet")) { consider_localnet_port(binding_rec, &bridge_mappings, - &egress_ifaces, b_ctx_out->local_datapaths); + b_ctx_out->egress_ifaces, + b_ctx_out->local_datapaths); } } shash_destroy(&bridge_mappings); - if (!sset_is_empty(&egress_ifaces) + if (!sset_is_empty(b_ctx_out->egress_ifaces) && set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, - b_ctx_in->qos_table, &egress_ifaces)) { + b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) { const char *entry; - SSET_FOR_EACH (entry, &egress_ifaces) { + SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { setup_qos(entry, &qos_map); } } - shash_destroy(&lport_to_iface); - sset_destroy(&egress_ifaces); hmap_destroy(&qos_map); } /* Returns true if port-binding changes potentially require flow changes on * the current chassis. Returns false if we are sure there is no impact. */ bool -binding_evaluate_port_binding_changes( - const struct sbrec_port_binding_table *pb_table, - const struct ovsrec_bridge *br_int, - const struct sbrec_chassis *chassis_rec, - struct sset *active_tunnels, - struct sset *local_lports) +binding_evaluate_port_binding_changes(struct binding_ctx_in *b_ctx_in, + struct binding_ctx_out *b_ctx_out) { - if (!chassis_rec) { + if (!b_ctx_in->chassis_rec) { return true; } bool changed = false; const struct sbrec_port_binding *binding_rec; - struct shash lport_to_iface = SHASH_INITIALIZER(&lport_to_iface); - struct sset egress_ifaces = SSET_INITIALIZER(&egress_ifaces); - if (br_int) { - get_local_iface_ids(br_int, &lport_to_iface, local_lports, - &egress_ifaces); - } - SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (binding_rec, pb_table) { + SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (binding_rec, + b_ctx_in->port_binding_table) { /* XXX: currently OVSDB change tracking doesn't support getting old * data when the operation is update, so if a port-binding moved from * this chassis to another, there is no easy way to find out the @@ -829,24 +976,31 @@ binding_evaluate_port_binding_changes( * interface table will be updated, which will trigger recompute. * * - If the port is not a regular VIF, always trigger recompute. */ - if (binding_rec->chassis == chassis_rec) { + if (binding_rec->chassis == b_ctx_in->chassis_rec) { changed = true; break; } - const struct ovsrec_interface *iface_rec - = shash_find_data(&lport_to_iface, binding_rec->logical_port); - if (is_our_chassis(chassis_rec, binding_rec, active_tunnels, iface_rec, - local_lports) || - (strcmp(binding_rec->type, "") && strcmp(binding_rec->type, - "remote"))) { + if (strcmp(binding_rec->type, "")) { + changed = true; + break; + } + + struct local_binding *lbinding = NULL; + if (!binding_rec->parent_port || !binding_rec->parent_port[0]) { + lbinding = local_binding_find(b_ctx_out->local_bindings, + binding_rec->logical_port); + } else { + lbinding = local_binding_find(b_ctx_out->local_bindings, + binding_rec->parent_port); + } + + if (lbinding) { changed = true; break; } } - shash_destroy(&lport_to_iface); - sset_destroy(&egress_ifaces); return changed; } diff --git a/controller/binding.h b/controller/binding.h index d0b8331af5..6bee1d12e0 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -31,6 +31,7 @@ struct ovsrec_open_vswitch_table; struct sbrec_chassis; struct sbrec_port_binding_table; struct sset; +struct sbrec_port_binding; struct binding_ctx_in { struct ovsdb_idl_txn *ovnsb_idl_txn; @@ -51,8 +52,10 @@ struct binding_ctx_in { struct binding_ctx_out { struct hmap *local_datapaths; + struct shash *local_bindings; struct sset *local_lports; struct sset *local_lport_ids; + struct sset *egress_ifaces; }; void binding_register_ovs_idl(struct ovsdb_idl *); @@ -60,11 +63,10 @@ void binding_run(struct binding_ctx_in *, struct binding_ctx_out *); bool binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, const struct sbrec_port_binding_table *, const struct sbrec_chassis *); -bool binding_evaluate_port_binding_changes( - const struct sbrec_port_binding_table *, - const struct ovsrec_bridge *br_int, - const struct sbrec_chassis *, - struct sset *active_tunnels, - struct sset *local_lports); - +bool binding_evaluate_port_binding_changes(struct binding_ctx_in *, + struct binding_ctx_out *); +void local_bindings_destroy(struct shash *local_bindings); +void binding_add_vport_to_local_bindings( + struct shash *local_bindings, const struct sbrec_port_binding *parent, + const struct sbrec_port_binding *vport); #endif /* controller/binding.h */ diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 4930d5ffb0..1052b07744 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -958,6 +958,9 @@ struct ed_type_runtime_data { /* Contains "struct local_datapath" nodes. */ struct hmap local_datapaths; + /* Contains "struct local_bindings" nodes. */ + struct shash local_bindings; + /* Contains the name of each logical port resident on the local * hypervisor. These logical ports include the VIFs (and their child * logical ports, if any) that belong to VMs running on the hypervisor, @@ -969,6 +972,8 @@ struct ed_type_runtime_data { * _ */ struct sset local_lport_ids; struct sset active_tunnels; + + struct sset egress_ifaces; }; static void * @@ -981,6 +986,8 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED, sset_init(&data->local_lports); sset_init(&data->local_lport_ids); sset_init(&data->active_tunnels); + sset_init(&data->egress_ifaces); + shash_init(&data->local_bindings); return data; } @@ -992,6 +999,7 @@ en_runtime_data_cleanup(void *data) sset_destroy(&rt_data->local_lports); sset_destroy(&rt_data->local_lport_ids); sset_destroy(&rt_data->active_tunnels); + sset_init(&rt_data->egress_ifaces); struct local_datapath *cur_node, *next_node; HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node, &rt_data->local_datapaths) { @@ -1000,6 +1008,7 @@ en_runtime_data_cleanup(void *data) free(cur_node); } hmap_destroy(&rt_data->local_datapaths); + local_bindings_destroy(&rt_data->local_bindings); } static void @@ -1072,6 +1081,8 @@ init_binding_ctx(struct engine_node *node, b_ctx_out->local_datapaths = &rt_data->local_datapaths; b_ctx_out->local_lports = &rt_data->local_lports; b_ctx_out->local_lport_ids = &rt_data->local_lport_ids; + b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; + b_ctx_out->local_bindings = &rt_data->local_bindings; } static void @@ -1098,9 +1109,11 @@ en_runtime_data_run(struct engine_node *node, void *data) sset_destroy(local_lports); sset_destroy(local_lport_ids); sset_destroy(active_tunnels); + sset_destroy(&rt_data->egress_ifaces); sset_init(local_lports); sset_init(local_lport_ids); sset_init(active_tunnels); + sset_init(&rt_data->egress_ifaces); } struct binding_ctx_in b_ctx_in; @@ -1129,35 +1142,12 @@ static bool runtime_data_sb_port_binding_handler(struct engine_node *node, void *data) { struct ed_type_runtime_data *rt_data = data; - struct sset *local_lports = &rt_data->local_lports; - struct sset *active_tunnels = &rt_data->active_tunnels; - - struct ovsrec_open_vswitch_table *ovs_table = - (struct ovsrec_open_vswitch_table *)EN_OVSDB_GET( - engine_get_input("OVS_open_vswitch", node)); - struct ovsrec_bridge_table *bridge_table = - (struct ovsrec_bridge_table *)EN_OVSDB_GET( - engine_get_input("OVS_bridge", node)); - const char *chassis_id = chassis_get_id(); - const struct ovsrec_bridge *br_int = get_br_int(bridge_table, ovs_table); - - ovs_assert(br_int && chassis_id); - - struct ovsdb_idl_index *sbrec_chassis_by_name = - engine_ovsdb_node_get_index( - engine_get_input("SB_chassis", node), - "name"); - - const struct sbrec_chassis *chassis - = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); - ovs_assert(chassis); - - struct sbrec_port_binding_table *pb_table = - (struct sbrec_port_binding_table *)EN_OVSDB_GET( - engine_get_input("SB_port_binding", node)); + struct binding_ctx_in b_ctx_in; + struct binding_ctx_out b_ctx_out; + init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out); - bool changed = binding_evaluate_port_binding_changes( - pb_table, br_int, chassis, active_tunnels, local_lports); + bool changed = binding_evaluate_port_binding_changes(&b_ctx_in, + &b_ctx_out); return !changed; } diff --git a/controller/pinctrl.c b/controller/pinctrl.c index a053938ecb..6c2acd2c2a 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -18,6 +18,7 @@ #include "pinctrl.h" +#include "binding.h" #include "coverage.h" #include "csum.h" #include "dirs.h" diff --git a/controller/pinctrl.h b/controller/pinctrl.h index 8fa4baae9e..12fb3b0808 100644 --- a/controller/pinctrl.h +++ b/controller/pinctrl.h @@ -31,6 +31,7 @@ struct sbrec_chassis; struct sbrec_dns_table; struct sbrec_controller_event_table; struct sbrec_service_monitor_table; +struct shash; void pinctrl_init(void); void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, From b7b81e4c48d8159e06f33de2d7b4249e2ce4fd5c Mon Sep 17 00:00:00 2001 From: Numan Siddique Date: Wed, 22 Jan 2020 17:33:49 +0530 Subject: [PATCH 3/9] ovn-controller: I-P for SB port binding and OVS interface in runtime_data. This patch handles SB port binding changes and OVS interface changes incrementally in the runtime_data stage which otherwise would have resulted in calls to binding_run(). Prior to this patch, port binding changes were handled differently. The changes were only evaluated to see if they need full recompute or they can be ignored. With this patch, the runtime data is updated with the changes (both SB port binding and OVS interface) and ports are claimed/released in the handlers itself, avoiding the need to trigger recompute of runtime data stage. Signed-off-by: Numan Siddique --- controller/binding.c | 583 ++++++++++++++++++++++++++++++------ controller/binding.h | 9 +- controller/ovn-controller.c | 61 +++- tests/ovn-performance.at | 13 + 4 files changed, 574 insertions(+), 92 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index 66f4f65e30..187ae4a4bb 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -349,17 +349,6 @@ setup_qos(const char *egress_iface, struct hmap *queue_map) netdev_close(netdev_phy); } -static void -update_local_lport_ids(struct sset *local_lport_ids, - const struct sbrec_port_binding *binding_rec) -{ - char buf[16]; - snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, - binding_rec->datapath->tunnel_key, - binding_rec->tunnel_key); - sset_add(local_lport_ids, buf); -} - /* * Get the encap from the chassis for this port. The interface * may have an external_ids:encap-ip= set; if so we @@ -488,6 +477,28 @@ consider_localnet_port(const struct sbrec_port_binding *binding_rec, ld->localnet_port = binding_rec; } +static void +update_local_lport_ids(struct sset *local_lport_ids, + const struct sbrec_port_binding *binding_rec) +{ + char buf[16]; + snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, + binding_rec->datapath->tunnel_key, + binding_rec->tunnel_key); + sset_add(local_lport_ids, buf); +} + +static void +remove_local_lport_ids(const struct sbrec_port_binding *binding_rec, + struct sset *local_lport_ids) +{ + char buf[16]; + snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, + binding_rec->datapath->tunnel_key, + binding_rec->tunnel_key); + sset_find_and_delete(local_lport_ids, buf); +} + enum local_binding_type { BT_VIF, BT_CHILD, @@ -543,18 +554,21 @@ local_binding_destroy(struct local_binding *lbinding) free(lbinding); } +static +void local_binding_delete(struct shash *local_bindings, + struct local_binding *lbinding) +{ + shash_find_and_delete(local_bindings, lbinding->name); + local_binding_destroy(lbinding); +} + void local_bindings_destroy(struct shash *local_bindings) { struct shash_node *node, *next; SHASH_FOR_EACH_SAFE (node, next, local_bindings) { struct local_binding *lbinding = node->data; - struct local_binding *c, *n; - LIST_FOR_EACH_SAFE (c, n, node, &lbinding->children) { - ovs_list_remove(&c->node); - free(c->name); - free(c); - } + local_binding_destroy(lbinding); } shash_destroy(local_bindings); @@ -607,10 +621,11 @@ binding_add_vport_to_local_bindings(struct shash *local_bindings, } } -static void +static bool claim_lport(const struct sbrec_port_binding *pb, const struct sbrec_chassis *chassis_rec, - const struct ovsrec_interface *iface_rec) + const struct ovsrec_interface *iface_rec, + bool cant_update_sb) { if (pb->chassis != chassis_rec) { if (pb->chassis) { @@ -624,6 +639,9 @@ claim_lport(const struct sbrec_port_binding *pb, VLOG_INFO("%s: Claiming %s", pb->logical_port, pb->mac[i]); } + if (cant_update_sb) { + return false; + } sbrec_port_binding_set_chassis(pb, chassis_rec); } @@ -631,25 +649,74 @@ claim_lport(const struct sbrec_port_binding *pb, struct sbrec_encap *encap_rec = sbrec_get_port_encap(chassis_rec, iface_rec); if (encap_rec && pb->encap != encap_rec) { + if (cant_update_sb) { + return false; + } sbrec_port_binding_set_encap(pb, encap_rec); } + + return true; } -static void -release_lport(const struct sbrec_port_binding *pb) +static bool +release_lport(const struct sbrec_port_binding *pb, bool cant_update_sb) { VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port); if (pb->encap) { - sbrec_port_binding_set_encap(pb, NULL); + if (pb->encap) { + if (cant_update_sb) { + return false; + } + sbrec_port_binding_set_encap(pb, NULL); + } + } + + if (pb->chassis) { + if (cant_update_sb) { + return false; + } + sbrec_port_binding_set_chassis(pb, NULL); } - sbrec_port_binding_set_chassis(pb, NULL); if (pb->virtual_parent) { + if (cant_update_sb) { + return false; + } sbrec_port_binding_set_virtual_parent(pb, NULL); } + + return true; } -static void +static bool +release_local_binding_children(struct local_binding *lbinding, + bool cant_update_sb) +{ + struct local_binding *l; + LIST_FOR_EACH (l, node, &lbinding->children) { + if (!release_lport(l->pb, cant_update_sb)) { + return false; + } + } + + return true; +} + +static bool +release_local_binding(struct local_binding *lbinding, bool cant_update_sb) +{ + if (!release_local_binding_children(lbinding, cant_update_sb)) { + return false; + } + + if (!release_lport(lbinding->pb, cant_update_sb)) { + return false; + } + + return true; +} + +static bool consider_port_binding_for_vif(const struct sbrec_port_binding *pb, struct binding_ctx_in *b_ctx_in, enum local_binding_type binding_type, @@ -673,11 +740,14 @@ consider_port_binding_for_vif(const struct sbrec_port_binding *pb, "Virtual port %s should not be bound to OVS port %s", pb->logical_port, lbinding->iface->name); lbinding->pb = NULL; - return; + return false; } if (lbinding && lbinding->pb && can_bind) { - claim_lport(pb, b_ctx_in->chassis_rec, lbinding->iface); + if (!claim_lport(pb, b_ctx_in->chassis_rec, lbinding->iface, + !b_ctx_in->ovnsb_idl_txn)) { + return false; + } switch (binding_type) { case BT_VIF: @@ -726,22 +796,26 @@ consider_port_binding_for_vif(const struct sbrec_port_binding *pb, if (pb->chassis == b_ctx_in->chassis_rec) { if (!lbinding || !lbinding->pb || !can_bind) { - release_lport(pb); + if (!release_lport(pb, !b_ctx_in->ovnsb_idl_txn)) { + return false; + } } } + + return true; } -static void +static bool consider_port_binding(const struct sbrec_port_binding *pb, struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out, struct hmap *qos_map) { - bool our_chassis = is_our_chassis(b_ctx_in->chassis_rec, pb, b_ctx_in->active_tunnels, b_ctx_out->local_lports); + bool success = true; if (!strcmp(pb->type, "l2gateway")) { if (our_chassis) { sset_add(b_ctx_out->local_lports, pb->logical_port); @@ -789,12 +863,14 @@ consider_port_binding(const struct sbrec_port_binding *pb, update_local_lport_ids(b_ctx_out->local_lport_ids, pb); } - ovs_assert(b_ctx_in->ovnsb_idl_txn); if (our_chassis) { - claim_lport(pb, b_ctx_in->chassis_rec, NULL); + success = claim_lport(pb, b_ctx_in->chassis_rec, NULL, + !b_ctx_in->ovnsb_idl_txn); } else if (pb->chassis == b_ctx_in->chassis_rec) { - release_lport(pb); + success = release_lport(pb, !b_ctx_in->ovnsb_idl_txn); } + + return success; } static void @@ -833,6 +909,8 @@ build_local_bindings_from_local_ifaces(struct binding_ctx_in *b_ctx_in, lbinding->pb = pb; } sset_add(b_ctx_out->local_lports, iface_id); + smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, + iface_id); } /* Check if this is a tunnel interface. */ @@ -950,60 +1028,6 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) hmap_destroy(&qos_map); } -/* Returns true if port-binding changes potentially require flow changes on - * the current chassis. Returns false if we are sure there is no impact. */ -bool -binding_evaluate_port_binding_changes(struct binding_ctx_in *b_ctx_in, - struct binding_ctx_out *b_ctx_out) -{ - if (!b_ctx_in->chassis_rec) { - return true; - } - - bool changed = false; - - const struct sbrec_port_binding *binding_rec; - SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (binding_rec, - b_ctx_in->port_binding_table) { - /* XXX: currently OVSDB change tracking doesn't support getting old - * data when the operation is update, so if a port-binding moved from - * this chassis to another, there is no easy way to find out the - * change. To workaround this problem, we just makes sure if - * any port *related to* this chassis has any change, then trigger - * recompute. - * - * - If a regular VIF is unbound from this chassis, the local ovsdb - * interface table will be updated, which will trigger recompute. - * - * - If the port is not a regular VIF, always trigger recompute. */ - if (binding_rec->chassis == b_ctx_in->chassis_rec) { - changed = true; - break; - } - - if (strcmp(binding_rec->type, "")) { - changed = true; - break; - } - - struct local_binding *lbinding = NULL; - if (!binding_rec->parent_port || !binding_rec->parent_port[0]) { - lbinding = local_binding_find(b_ctx_out->local_bindings, - binding_rec->logical_port); - } else { - lbinding = local_binding_find(b_ctx_out->local_bindings, - binding_rec->parent_port); - } - - if (lbinding) { - changed = true; - break; - } - } - - return changed; -} - /* Returns true if the database is all cleaned up, false if more work is * required. */ bool @@ -1038,3 +1062,390 @@ binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, return !any_changes; } + +static void +add_local_datapath_peer_port(const struct sbrec_port_binding *pb, + struct binding_ctx_in *b_ctx_in, + struct binding_ctx_out *b_ctx_out, + struct local_datapath *ld) +{ + bool present = false; + for (size_t i = 0; i < ld->n_peer_ports; i++) { + if (ld->peer_ports[i].local == pb) { + present = true; + break; + } + } + + const char *peer_name = smap_get(&pb->options, "peer"); + if (strcmp(pb->type, "patch") || !peer_name) { + return; + } + + const struct sbrec_port_binding *peer; + peer = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, + peer_name); + + if (!peer || !peer->datapath) { + return; + } + + if (!present) { + ld->n_peer_ports++; + if (ld->n_peer_ports > ld->n_allocated_peer_ports) { + ld->peer_ports = + x2nrealloc(ld->peer_ports, + &ld->n_allocated_peer_ports, + sizeof *ld->peer_ports); + } + ld->peer_ports[ld->n_peer_ports - 1].local = pb; + ld->peer_ports[ld->n_peer_ports - 1].remote = peer; + } + + struct local_datapath *peer_ld = + get_local_datapath(b_ctx_out->local_datapaths, + peer->datapath->tunnel_key); + if (!peer_ld) { + add_local_datapath__(b_ctx_in->sbrec_datapath_binding_by_key, + b_ctx_in->sbrec_port_binding_by_datapath, + b_ctx_in->sbrec_port_binding_by_name, + peer->datapath, false, + 1, b_ctx_out->local_datapaths); + return; + } + + for (size_t i = 0; i < peer_ld->n_peer_ports; i++) { + if (peer_ld->peer_ports[i].local == peer) { + return; + } + } + + peer_ld->n_peer_ports++; + if (peer_ld->n_peer_ports > peer_ld->n_allocated_peer_ports) { + peer_ld->peer_ports = + x2nrealloc(peer_ld->peer_ports, + &peer_ld->n_allocated_peer_ports, + sizeof *peer_ld->peer_ports); + } + peer_ld->peer_ports[peer_ld->n_peer_ports - 1].local = peer; + peer_ld->peer_ports[peer_ld->n_peer_ports - 1].remote = pb; +} + +static void +remove_local_datapath_peer_port(const struct sbrec_port_binding *pb, + struct local_datapath *ld, + struct hmap *local_datapaths) +{ + size_t i =0; + for (i = 0; i < ld->n_peer_ports; i++) { + if (ld->peer_ports[i].local == pb) { + break; + } + } + + if (i == ld->n_peer_ports) { + return; + } + + const struct sbrec_port_binding *peer = ld->peer_ports[i].remote; + + ld->peer_ports[i].local = ld->peer_ports[ld->n_peer_ports - 1].local; + ld->peer_ports[i].remote = ld->peer_ports[ld->n_peer_ports - 1].remote; + ld->n_peer_ports--; + + struct local_datapath *peer_ld = + get_local_datapath(local_datapaths, peer->datapath->tunnel_key); + if (peer_ld) { + remove_local_datapath_peer_port(peer, peer_ld, local_datapaths); + } +} + +static void +update_local_datapath_for_pb(const struct sbrec_port_binding *pb, + struct binding_ctx_in *b_ctx_in, + struct binding_ctx_out *b_ctx_out, + struct local_datapath *ld) +{ + if (!strcmp(pb->type, "patch")) { + add_local_datapath_peer_port(pb, b_ctx_in, b_ctx_out, ld); + } else if (!strcmp(pb->type, "localnet")) { + struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); + add_ovs_bridge_mappings(b_ctx_in->ovs_table, b_ctx_in->bridge_table, + &bridge_mappings); + consider_localnet_port(pb, &bridge_mappings, b_ctx_out->egress_ifaces, + b_ctx_out->local_datapaths); + shash_destroy(&bridge_mappings); + } else if (!strcmp(pb->type, "l3gateway")) { + const char *chassis_id = smap_get(&pb->options, + "l3gateway-chassis"); + if (chassis_id && !strcmp(chassis_id, b_ctx_in->chassis_rec->name)) { + ld->has_local_l3gateway = true; + } + } + + if (!strcmp(pb->type, "patch") || + !strcmp(pb->type, "localport") || + !strcmp(pb->type, "vtep")) { + update_local_lport_ids(b_ctx_out->local_lport_ids, pb); + } +} + +static void +remove_pb_from_local_datapath(const struct sbrec_port_binding *pb, + const struct sbrec_chassis *chassis_rec, + struct binding_ctx_out *b_ctx_out, + struct local_datapath *ld) +{ + remove_local_lport_ids(pb, b_ctx_out->local_lport_ids); + if (!strcmp(pb->type, "patch") || + !strcmp(pb->type, "l3gateway")) { + remove_local_datapath_peer_port(pb, ld, b_ctx_out->local_datapaths); + } else if (!strcmp(pb->type, "localnet")) { + if (ld->localnet_port && !strcmp(ld->localnet_port->logical_port, + pb->logical_port)) { + ld->localnet_port = NULL; + } + } else if (!strcmp(pb->type, "l3gateway")) { + const char *chassis_id = smap_get(&pb->options, + "l3gateway-chassis"); + if (chassis_id && !strcmp(chassis_id, chassis_rec->name)) { + ld->has_local_l3gateway = false; + } + } +} + +/* Returns true if the ovs interface changes were handled successfully, + * false otherwise. + */ +bool +binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, + struct binding_ctx_out *b_ctx_out, + bool *changed) +{ + if (!b_ctx_in->chassis_rec) { + return false; + } + + bool handled = true; + *changed = false; + + struct hmap qos_map = HMAP_INITIALIZER(&qos_map); + struct hmap *qos_map_ptr = + sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; + + const struct ovsrec_interface *iface_rec; + OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec, + b_ctx_in->iface_table) { + const char *iface_id = smap_get(&iface_rec->external_ids, "iface-id"); + const char *old_iface_id = smap_get(b_ctx_out->local_iface_ids, + iface_rec->name); + if (iface_rec->type && iface_rec->type[0] && + strcmp(iface_rec->type, "internal")) { + /* Right now are not handling ovs_interface changes of + * other types. This can be enhanced to handle of + * types - patch and tunnel. */ + handled = false; + goto out; + } + + struct local_binding *lbinding = NULL; + struct local_binding *claim_lbinding = NULL; + const char *cleared_iface_id = NULL; + int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; + if (!ovsrec_interface_is_deleted(iface_rec)) { + if (iface_id) { + /* Check if iface_id is changed. If so we need to + * release the old port binding and associate this + * inteface to new port binding. */ + if (old_iface_id && strcmp(iface_id, old_iface_id)) { + cleared_iface_id = old_iface_id; + } + } else if (old_iface_id) { + cleared_iface_id = old_iface_id; + } + } else { + cleared_iface_id = iface_id; + iface_id = NULL; + } + + if (cleared_iface_id) { + lbinding = local_binding_find(b_ctx_out->local_bindings, + cleared_iface_id); + if (lbinding && lbinding->pb && + lbinding->pb->chassis == b_ctx_in->chassis_rec) { + + if (!release_local_binding(lbinding, + !b_ctx_in->ovnsb_idl_txn)) { + handled = false; + goto out; + } + struct local_datapath *ld = + get_local_datapath(b_ctx_out->local_datapaths, + lbinding->pb->datapath->tunnel_key); + if (ld) { + remove_pb_from_local_datapath(lbinding->pb, + b_ctx_in->chassis_rec, + b_ctx_out, ld); + } + local_binding_delete(b_ctx_out->local_bindings, lbinding); + *changed = true; + } + + sset_find_and_delete(b_ctx_out->local_lports, cleared_iface_id); + smap_remove(b_ctx_out->local_iface_ids, iface_rec->name); + } + + if (iface_id && ofport > 0) { + *changed = true; + sset_add(b_ctx_out->local_lports, iface_id); + smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, + iface_id); + claim_lbinding = + local_binding_find(b_ctx_out->local_bindings, iface_id); + if (!claim_lbinding) { + claim_lbinding = local_binding_create(iface_id, iface_rec, + NULL, BT_VIF); + local_binding_add(b_ctx_out->local_bindings, claim_lbinding); + } else { + claim_lbinding->iface = iface_rec; + } + + if (!claim_lbinding->pb || + strcmp(claim_lbinding->name, + claim_lbinding->pb->logical_port)) { + claim_lbinding->pb = + lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, + claim_lbinding->name); + if (claim_lbinding->pb && + !strcmp(claim_lbinding->pb->type, "virtual")) { + claim_lbinding->pb = NULL; + } + } + + if (claim_lbinding->pb) { + if (!consider_port_binding_for_vif(claim_lbinding->pb, + b_ctx_in, BT_VIF, + claim_lbinding, + b_ctx_out, qos_map_ptr)) { + handled = false; + goto out; + } + } + } + } + + if (qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn, + b_ctx_in->port_table, + b_ctx_in->qos_table, + b_ctx_out->egress_ifaces)) { + const char *entry; + SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { + setup_qos(entry, &qos_map); + } + } + + hmap_destroy(&qos_map); +out: + return handled; +} + +/* Returns true if the port binding changes resulted in local binding + * updates, false otherwise. + */ +bool +binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in, + struct binding_ctx_out *b_ctx_out, + bool *changed) +{ + bool handled = true; + struct hmap qos_map = HMAP_INITIALIZER(&qos_map); + struct hmap *qos_map_ptr = + sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; + + *changed = false; + + const struct sbrec_port_binding *pb; + SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, + b_ctx_in->port_binding_table) { + bool consider_for_vif = false; + struct local_binding *lbinding = NULL; + enum local_binding_type binding_type = BT_VIF; + bool is_deleted = sbrec_port_binding_is_deleted(pb); + if (!pb->type[0]) { + if (!pb->parent_port || !pb->parent_port[0]) { + lbinding = local_binding_find(b_ctx_out->local_bindings, + pb->logical_port); + } else { + lbinding = local_binding_find(b_ctx_out->local_bindings, + pb->parent_port); + binding_type = BT_CHILD; + } + + consider_for_vif = true; + } else if (!strcmp(pb->type, "virtual") && + pb->virtual_parent && pb->virtual_parent[0]) { + lbinding = local_binding_find(b_ctx_out->local_bindings, + pb->virtual_parent); + consider_for_vif = true; + binding_type = BT_VIRTUAL; + } + + if (is_deleted) { + if (lbinding) { + lbinding->pb = NULL; + if (binding_type == BT_VIF && + !release_local_binding_children( + lbinding, !b_ctx_in->ovnsb_idl_txn)) { + handled = false; + break; + } + *changed = true; + } + + struct local_datapath *ld = + get_local_datapath(b_ctx_out->local_datapaths, + pb->datapath->tunnel_key); + if (ld) { + remove_pb_from_local_datapath(pb, b_ctx_in->chassis_rec, + b_ctx_out, ld); + } + } else { + if (consider_for_vif) { + if (lbinding) { + lbinding->pb = pb; + bool claimed = (pb->chassis == b_ctx_in->chassis_rec); + if (!consider_port_binding_for_vif( + pb, b_ctx_in, binding_type, lbinding, b_ctx_out, + qos_map_ptr)) { + handled = false; + break; + } + bool now_claimed = (pb->chassis == b_ctx_in->chassis_rec); + if (!claimed && now_claimed) { + *changed = true; + } + } + } else { + if (!consider_port_binding(pb, b_ctx_in, b_ctx_out, + qos_map_ptr)) { + handled = false; + break; + } + struct local_datapath *ld = + get_local_datapath(b_ctx_out->local_datapaths, + pb->datapath->tunnel_key); + if (ld) { + update_local_datapath_for_pb(pb, b_ctx_in, b_ctx_out, ld); + } + *changed = true; + if (!strcmp(pb->type, "patch") || + !strcmp(pb->type, "localport") || + !strcmp(pb->type, "vtep")) { + update_local_lport_ids(b_ctx_out->local_lport_ids, pb); + } + } + } + } + + return handled; +} diff --git a/controller/binding.h b/controller/binding.h index 6bee1d12e0..fda6807235 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -56,6 +56,7 @@ struct binding_ctx_out { struct sset *local_lports; struct sset *local_lport_ids; struct sset *egress_ifaces; + struct smap *local_iface_ids; }; void binding_register_ovs_idl(struct ovsdb_idl *); @@ -63,10 +64,14 @@ void binding_run(struct binding_ctx_in *, struct binding_ctx_out *); bool binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, const struct sbrec_port_binding_table *, const struct sbrec_chassis *); -bool binding_evaluate_port_binding_changes(struct binding_ctx_in *, - struct binding_ctx_out *); void local_bindings_destroy(struct shash *local_bindings); void binding_add_vport_to_local_bindings( struct shash *local_bindings, const struct sbrec_port_binding *parent, const struct sbrec_port_binding *vport); +bool binding_handle_ovs_interface_changes(struct binding_ctx_in *, + struct binding_ctx_out *, + bool *changed); +bool binding_handle_port_binding_changes(struct binding_ctx_in *, + struct binding_ctx_out *, + bool *changed); #endif /* controller/binding.h */ diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 1052b07744..c0f198d878 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -753,6 +753,7 @@ enum sb_engine_node { OVS_NODE(open_vswitch, "open_vswitch") \ OVS_NODE(bridge, "bridge") \ OVS_NODE(port, "port") \ + OVS_NODE(interface, "interface") \ OVS_NODE(qos, "qos") enum ovs_engine_node { @@ -974,6 +975,7 @@ struct ed_type_runtime_data { struct sset active_tunnels; struct sset egress_ifaces; + struct smap local_iface_ids; }; static void * @@ -988,6 +990,7 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED, sset_init(&data->active_tunnels); sset_init(&data->egress_ifaces); shash_init(&data->local_bindings); + smap_init(&data->local_iface_ids); return data; } @@ -1000,6 +1003,7 @@ en_runtime_data_cleanup(void *data) sset_destroy(&rt_data->local_lport_ids); sset_destroy(&rt_data->active_tunnels); sset_init(&rt_data->egress_ifaces); + smap_destroy(&rt_data->local_iface_ids); struct local_datapath *cur_node, *next_node; HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node, &rt_data->local_datapaths) { @@ -1041,6 +1045,10 @@ init_binding_ctx(struct engine_node *node, (struct ovsrec_port_table *)EN_OVSDB_GET( engine_get_input("OVS_port", node)); + struct ovsrec_interface_table *iface_table = + (struct ovsrec_interface_table *)EN_OVSDB_GET( + engine_get_input("OVS_interface", node)); + struct ovsrec_qos_table *qos_table = (struct ovsrec_qos_table *)EN_OVSDB_GET( engine_get_input("OVS_qos", node)); @@ -1070,6 +1078,7 @@ init_binding_ctx(struct engine_node *node, b_ctx_in->sbrec_port_binding_by_datapath = sbrec_port_binding_by_datapath; b_ctx_in->sbrec_port_binding_by_name = sbrec_port_binding_by_name; b_ctx_in->port_table = port_table; + b_ctx_in->iface_table = iface_table; b_ctx_in->qos_table = qos_table; b_ctx_in->port_binding_table = pb_table; b_ctx_in->br_int = br_int; @@ -1083,6 +1092,7 @@ init_binding_ctx(struct engine_node *node, b_ctx_out->local_lport_ids = &rt_data->local_lport_ids; b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; b_ctx_out->local_bindings = &rt_data->local_bindings; + b_ctx_out->local_iface_ids = &rt_data->local_iface_ids; } static void @@ -1110,10 +1120,12 @@ en_runtime_data_run(struct engine_node *node, void *data) sset_destroy(local_lport_ids); sset_destroy(active_tunnels); sset_destroy(&rt_data->egress_ifaces); + smap_destroy(&rt_data->local_iface_ids); sset_init(local_lports); sset_init(local_lport_ids); sset_init(active_tunnels); sset_init(&rt_data->egress_ifaces); + smap_init(&rt_data->local_iface_ids); } struct binding_ctx_in b_ctx_in; @@ -1138,6 +1150,34 @@ en_runtime_data_run(struct engine_node *node, void *data) engine_set_node_state(node, EN_UPDATED); } +static bool +runtime_data_ovs_interface_handler(struct engine_node *node, void *data) +{ + struct ed_type_runtime_data *rt_data = data; + struct binding_ctx_in b_ctx_in; + struct binding_ctx_out b_ctx_out; + init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out); + + bool changed = false; + if (!binding_handle_ovs_interface_changes(&b_ctx_in, &b_ctx_out, + &changed)) { + return false; + } + + if (changed) { + engine_set_node_state(node, EN_UPDATED); + } + + return true; +} + +static bool +runtime_data_noop_handler(struct engine_node *node OVS_UNUSED, + void *data OVS_UNUSED) +{ + return true; +} + static bool runtime_data_sb_port_binding_handler(struct engine_node *node, void *data) { @@ -1145,11 +1185,21 @@ runtime_data_sb_port_binding_handler(struct engine_node *node, void *data) struct binding_ctx_in b_ctx_in; struct binding_ctx_out b_ctx_out; init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out); + if (!b_ctx_in.chassis_rec) { + return false; + } + + bool changed = false; + if (!binding_handle_port_binding_changes(&b_ctx_in, &b_ctx_out, + &changed)) { + return false; + } - bool changed = binding_evaluate_port_binding_changes(&b_ctx_in, - &b_ctx_out); + if (changed) { + engine_set_node_state(node, EN_UPDATED); + } - return !changed; + return true; } /* Connection tracking zones. */ @@ -1891,7 +1941,10 @@ main(int argc, char *argv[]) engine_add_input(&en_runtime_data, &en_ovs_open_vswitch, NULL); engine_add_input(&en_runtime_data, &en_ovs_bridge, NULL); - engine_add_input(&en_runtime_data, &en_ovs_port, NULL); + engine_add_input(&en_runtime_data, &en_ovs_port, + runtime_data_noop_handler); + engine_add_input(&en_runtime_data, &en_ovs_interface, + runtime_data_ovs_interface_handler); engine_add_input(&en_runtime_data, &en_ovs_qos, NULL); engine_add_input(&en_runtime_data, &en_sb_chassis, NULL); diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at index a8a15f8fe6..5dfc6f7ca2 100644 --- a/tests/ovn-performance.at +++ b/tests/ovn-performance.at @@ -239,6 +239,19 @@ for i in 1 2; do ovn_attach n1 br-phys 192.168.0.$i done +# Wait for the tunnel ports to be created and up. +# Otherwise this may affect the lflow_run count. + +OVS_WAIT_UNTIL([ + test $(as hv1 ovs-vsctl list interface ovn-hv2-0 | \ +grep tunnel_egress_iface_carrier=up | wc -l) -eq 1 +]) + +OVS_WAIT_UNTIL([ + test $(as hv2 ovs-vsctl list interface ovn-hv1-0 | \ +grep tunnel_egress_iface_carrier=up | wc -l) -eq 1 +]) + # Add router lr1 OVN_CONTROLLER_EXPECT_HIT( [hv1 hv2], [lflow_run], From 0582998c673b1a5f2a3fca372def8f3d3fbaa811 Mon Sep 17 00:00:00 2001 From: Numan Siddique Date: Mon, 10 Feb 2020 20:28:44 +0530 Subject: [PATCH 4/9] ovn-controller: I-P for datapath binding This patch adds partial support of incremental processing of datapath binding. If a datapath is deleted, then a full recompute is triggered if that datapath is present in the 'local_datapaths' hmap of runtime data. Signed-off-by: Numan Siddique --- controller/ovn-controller.c | 26 +++++++++++++++++++++++++- tests/ovn-performance.at | 6 +++--- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index c0f198d878..d2bec841ad 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1202,6 +1202,29 @@ runtime_data_sb_port_binding_handler(struct engine_node *node, void *data) return true; } +static bool +runtime_data_sb_datapath_binding_handler(struct engine_node *node OVS_UNUSED, + void *data OVS_UNUSED) +{ + struct sbrec_datapath_binding_table *dp_table = + (struct sbrec_datapath_binding_table *)EN_OVSDB_GET( + engine_get_input("SB_datapath_binding", node)); + const struct sbrec_datapath_binding *dp; + struct ed_type_runtime_data *rt_data = data; + + SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_TRACKED (dp, dp_table) { + if (sbrec_datapath_binding_is_deleted(dp)) { + if (get_local_datapath(&rt_data->local_datapaths, + dp->tunnel_key)) { + return false; + } + } + } + + engine_set_node_state(node, EN_VALID); + return true; +} + /* Connection tracking zones. */ struct ed_type_ct_zones { unsigned long bitmap[BITMAP_N_LONGS(MAX_CT_ZONES)]; @@ -1948,7 +1971,8 @@ main(int argc, char *argv[]) engine_add_input(&en_runtime_data, &en_ovs_qos, NULL); engine_add_input(&en_runtime_data, &en_sb_chassis, NULL); - engine_add_input(&en_runtime_data, &en_sb_datapath_binding, NULL); + engine_add_input(&en_runtime_data, &en_sb_datapath_binding, + runtime_data_sb_datapath_binding_handler); engine_add_input(&en_runtime_data, &en_sb_port_binding, runtime_data_sb_port_binding_handler); diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at index 5dfc6f7ca2..5cc1960b64 100644 --- a/tests/ovn-performance.at +++ b/tests/ovn-performance.at @@ -253,7 +253,7 @@ grep tunnel_egress_iface_carrier=up | wc -l) -eq 1 ]) # Add router lr1 -OVN_CONTROLLER_EXPECT_HIT( +OVN_CONTROLLER_EXPECT_NO_HIT( [hv1 hv2], [lflow_run], [ovn-nbctl --wait=hv lr-add lr1] ) @@ -264,7 +264,7 @@ for i in 1 2; do lrp=lr1-$ls # Add switch $ls - OVN_CONTROLLER_EXPECT_HIT( + OVN_CONTROLLER_EXPECT_NO_HIT( [hv1 hv2], [lflow_run], [ovn-nbctl --wait=hv ls-add $ls] ) @@ -427,7 +427,7 @@ for i in 1 2; do done # Delete router lr1 -OVN_CONTROLLER_EXPECT_HIT( +OVN_CONTROLLER_EXPECT_NO_HIT( [hv1 hv2], [lflow_run], [ovn-nbctl --wait=hv lr-del lr1] ) From 0d4c2c37cd71e59ce536e81503f0f0523ce429f8 Mon Sep 17 00:00:00 2001 From: Numan Siddique Date: Fri, 29 Nov 2019 13:30:38 +0530 Subject: [PATCH 5/9] ofctrl_check_and_add_flow: Replace the actions of an existing flow if actions have changed. If ofctrl_check_and_add_flow(F') is called where flow F' has match-actions (M, A2) and if there already exists a flow F with match-actions (M, A1) in the desired flow table, then this function doesn't update the existing flow F with new actions actions A2. This patch is required for the upcoming patch in this series which adds incremental processing for OVS interface in the flow output stage. Since we will be not be clearing the flow output data if these changes are handled incrementally, some of the existing flows will be updated with new actions. One such example would be flows in physical table OFTABLE_LOCAL_OUTPUT (table 33). And this patch is required to update such flows. Otherwise we will have incomplete actions installed. Signed-off-by: Numan Siddique --- controller/ofctrl.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 36e39ba06e..2df7400bef 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -667,14 +667,23 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table *flow_table, ovn_flow_log(f, "ofctrl_add_flow"); - if (ovn_flow_lookup(&flow_table->match_flow_table, f, true)) { - if (log_duplicate_flow) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); - if (!VLOG_DROP_DBG(&rl)) { - char *s = ovn_flow_to_string(f); - VLOG_DBG("dropping duplicate flow: %s", s); - free(s); + struct ovn_flow *existing_f = + ovn_flow_lookup(&flow_table->match_flow_table, f, true); + if (existing_f) { + if (ofpacts_equal(f->ofpacts, f->ofpacts_len, + existing_f->ofpacts, existing_f->ofpacts_len)) { + if (log_duplicate_flow) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); + if (!VLOG_DROP_DBG(&rl)) { + char *s = ovn_flow_to_string(f); + VLOG_DBG("dropping duplicate flow: %s", s); + free(s); + } } + } else { + free(existing_f->ofpacts); + existing_f->ofpacts = xmemdup(f->ofpacts, f->ofpacts_len); + existing_f->ofpacts_len = f->ofpacts_len; } ovn_flow_destroy(f); return; From 030259b279509e81ee497fcc60cca0a846c94bec Mon Sep 17 00:00:00 2001 From: Numan Siddique Date: Sun, 23 Feb 2020 00:44:33 +0530 Subject: [PATCH 6/9] ovn-controller: I-P for ct zone and OVS interface changes in flow output stage. 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 --- controller/binding.c | 22 ---------- controller/lflow.c | 13 ++++++ controller/lflow.h | 2 + controller/ovn-controller.c | 84 ++++++++++++++++++++++++++++++++++++- controller/ovn-controller.h | 22 ++++++++++ controller/physical.c | 51 ++++++++++++++++++++++ controller/physical.h | 5 ++- 7 files changed, 174 insertions(+), 25 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index 187ae4a4bb..2f8a8a01c2 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); -} - static void local_binding_destroy(struct local_binding *lbinding) { diff --git a/controller/lflow.c b/controller/lflow.c index 01214a3a64..512258cd3b 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; +} diff --git a/controller/lflow.h b/controller/lflow.h index f02f709d7d..fa54d4de28 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 d2bec841ad..127c4e2bb2 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1368,8 +1368,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; @@ -1377,12 +1382,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, @@ -1645,6 +1652,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 @@ -1655,6 +1664,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); @@ -1790,6 +1807,62 @@ 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 ct_zones_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->ct_zones_changed) { + pfc->ct_zones_changed = false; + physical_run(&p_ctx, &fo->flow_table); + return true; + } + + return physical_handle_ovs_iface_changes(&p_ctx, &fo->flow_table); +} + +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) +{ + engine_set_node_state(node, EN_UPDATED); +} + +static bool +physical_flow_changes_ct_zones_handler(struct engine_node *node OVS_UNUSED, + void *data OVS_UNUSED) +{ + struct ed_type_physical_flow_changes *pfc = data; + pfc->ct_zones_changed = true; + engine_set_node_state(node, EN_UPDATED); + return false; +} + struct ovn_controller_exit_args { bool *exiting; bool *restart; @@ -1912,6 +1985,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"); @@ -1931,13 +2005,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, + physical_flow_changes_ct_zones_handler); + engine_add_input(&en_physical_flow_changes, &en_ovs_interface, + NULL); + 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 5d9466880b..40c358cef0 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 144aeb7bdd..03eb677d1c 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); + } 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 dadf84ea1b..9ca34436a5 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 */ From 9a35b3bb5b5a05240607b3e28591082a554d516c Mon Sep 17 00:00:00 2001 From: Numan Siddique Date: Wed, 1 Apr 2020 19:02:42 +0530 Subject: [PATCH 7/9] I-P engine: Provide the option for an engine to store changed data With this patch, an engine node which is an input to another engine node can provide the tracked data. The parent of this engine node can handle this tracked data incrementally. At the end of the engine_run(), the tracked data of the nodes are cleared. Signed-off-by: Numan Siddique --- lib/inc-proc-eng.c | 30 +++++++++++++++++++++++++++++- lib/inc-proc-eng.h | 20 ++++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/lib/inc-proc-eng.c b/lib/inc-proc-eng.c index 9b1479a1ce..8d63feac74 100644 --- a/lib/inc-proc-eng.c +++ b/lib/inc-proc-eng.c @@ -125,6 +125,11 @@ engine_cleanup(void) engine_nodes[i]->cleanup(engine_nodes[i]->data); } free(engine_nodes[i]->data); + + if (engine_nodes[i]->clear_tracked_data) { + engine_nodes[i]->clear_tracked_data(engine_nodes[i]->tracked_data); + } + free(engine_nodes[i]->tracked_data); } free(engine_nodes); engine_nodes = NULL; @@ -222,6 +227,23 @@ engine_node_changed(struct engine_node *node) return node->state == EN_UPDATED; } +void * +engine_get_tracked_data(struct engine_node *node) +{ + if (engine_node_valid(node)) { + return node->tracked_data; + } + + return NULL; +} + +void * +engine_get_input_tracked_data(const char *input_name, struct engine_node *node) +{ + struct engine_node *input_node = engine_get_input(input_name, node); + return engine_get_tracked_data(input_node); +} + bool engine_has_run(void) { @@ -370,7 +392,13 @@ engine_run(bool recompute_allowed) if (engine_nodes[i]->state == EN_ABORTED) { engine_run_aborted = true; - return; + break; + } + } + + for (size_t i = 0; i < engine_n_nodes; i++) { + if (engine_nodes[i]->clear_tracked_data) { + engine_nodes[i]->clear_tracked_data(engine_nodes[i]->tracked_data); } } } diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h index 780c3cd229..4b5e69edb9 100644 --- a/lib/inc-proc-eng.h +++ b/lib/inc-proc-eng.h @@ -123,6 +123,15 @@ struct engine_node { */ void *data; + /* A pointer to node internal tracked data. The tracke data can be + * used by en engine node to provide the changed data during the + * engine run if its an input to other engine node. This data can + * be accessed during the engine run using the function + * engine_get_tracked_data(). This data can be cleared by + * calling the clean_tracked_data() engine node function. + */ + void *tracked_data; + /* State of the node after the last engine run. */ enum engine_node_state state; @@ -149,6 +158,9 @@ struct engine_node { * doesn't store pointers to DB records it's still safe to use). */ bool (*is_valid)(struct engine_node *); + + /* Method to clear up tracked data if any. It may be NULL. */ + void (*clear_tracked_data)(void *tracked_data); }; /* Initialize the data for the engine nodes. It calls each node's @@ -183,6 +195,12 @@ struct engine_node * engine_get_input(const char *input_name, /* Get the data from the input node with for */ void *engine_get_input_data(const char *input_name, struct engine_node *); +void *engine_get_tracked_data(struct engine_node *); + +/* Get the tracked data from the input node with for */ +void *engine_get_input_tracked_data(const char *input_name, + struct engine_node *); + /* Add an input (dependency) for , with corresponding change_handler, * which can be NULL. If the change_handler is NULL, the engine will not * be able to process the change incrementally, and will fall back to call @@ -270,11 +288,13 @@ void engine_ovsdb_node_add_index(struct engine_node *, const char *name, struct engine_node en_##NAME = { \ .name = NAME_STR, \ .data = NULL, \ + .tracked_data = NULL, \ .state = EN_STALE, \ .init = en_##NAME##_init, \ .run = en_##NAME##_run, \ .cleanup = en_##NAME##_cleanup, \ .is_valid = en_##NAME##_is_valid, \ + .clear_tracked_data = NULL, \ }; #define ENGINE_NODE_CUSTOM_DATA(NAME, NAME_STR) \ From c1775eb88f9ba1cd886d9e33a82faa85fbab883e Mon Sep 17 00:00:00 2001 From: Numan Siddique Date: Wed, 1 Apr 2020 23:18:33 +0530 Subject: [PATCH 8/9] I-P engine: Handle runtime data changes in flow output engine In order to handle runtime data changes incrementally, the flow outut runtime data handle should know the changed runtime data. Runtime data now tracks the changed data for any OVS interface and SB port binding changes. The tracked data contains a hmap of tracked datapaths (which changed during runtime data processing. The flow outout runtime_data handler in this patch doesn't do much with the tracked data. It returns false if there is tracked data available so that flow_output run is called. If no tracked data is available then there is no need for flow computation and the handler returns true. Next patch in the series processes the tracked data incrementally. Co-Authored-by: Venkata Anil Signed-off-by: Venkata Anil Signed-off-by: Numan Siddique --- controller/binding.c | 156 +++++++++++++++++++++++++++++++++--- controller/binding.h | 6 ++ controller/ovn-controller.c | 62 +++++++++++++- controller/ovn-controller.h | 16 +++- tests/ovn-performance.at | 24 +++--- 5 files changed, 237 insertions(+), 27 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index 2f8a8a01c2..fe955504d2 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -69,13 +69,20 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl) ovsdb_idl_add_column(ovs_idl, &ovsrec_qos_col_type); } +static struct tracked_binding_datapath *tracked_binding_datapath_create( + struct hmap *tracked_dps, const struct sbrec_datapath_binding *, + bool is_new); +static struct tracked_binding_datapath *tracked_binding_datapath_find( + struct hmap *, const struct sbrec_datapath_binding *); + static void add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, struct ovsdb_idl_index *sbrec_port_binding_by_datapath, struct ovsdb_idl_index *sbrec_port_binding_by_name, const struct sbrec_datapath_binding *datapath, bool has_local_l3gateway, int depth, - struct hmap *local_datapaths) + struct hmap *local_datapaths, + struct hmap *updated_dp_bindings) { uint32_t dp_key = datapath->tunnel_key; struct local_datapath *ld = get_local_datapath(local_datapaths, dp_key); @@ -92,6 +99,11 @@ add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, ld->localnet_port = NULL; ld->has_local_l3gateway = has_local_l3gateway; + if (updated_dp_bindings && + !tracked_binding_datapath_find(updated_dp_bindings, datapath)) { + tracked_binding_datapath_create(updated_dp_bindings, datapath, true); + } + if (depth >= 100) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); VLOG_WARN_RL(&rl, "datapaths nested too deep"); @@ -124,7 +136,8 @@ add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, sbrec_port_binding_by_datapath, sbrec_port_binding_by_name, peer->datapath, false, - depth + 1, local_datapaths); + depth + 1, local_datapaths, + updated_dp_bindings); } ld->n_peer_ports++; if (ld->n_peer_ports > ld->n_allocated_peer_ports) { @@ -147,12 +160,14 @@ add_local_datapath(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, struct ovsdb_idl_index *sbrec_port_binding_by_datapath, struct ovsdb_idl_index *sbrec_port_binding_by_name, const struct sbrec_datapath_binding *datapath, - bool has_local_l3gateway, struct hmap *local_datapaths) + bool has_local_l3gateway, struct hmap *local_datapaths, + struct hmap *updated_dp_bindings) { add_local_datapath__(sbrec_datapath_binding_by_key, sbrec_port_binding_by_datapath, sbrec_port_binding_by_name, - datapath, has_local_l3gateway, 0, local_datapaths); + datapath, has_local_l3gateway, 0, local_datapaths, + updated_dp_bindings); } static void @@ -580,6 +595,71 @@ local_binding_find_child(struct local_binding *lbinding, return NULL; } +static struct tracked_binding_datapath * +tracked_binding_datapath_create(struct hmap *tracked_datapaths, + const struct sbrec_datapath_binding *dp, + bool is_new) +{ + struct tracked_binding_datapath *t_dp = xzalloc(sizeof *t_dp); + t_dp->dp = dp; + t_dp->is_new = is_new; + ovs_list_init(&t_dp->lports_head); + hmap_insert(tracked_datapaths, &t_dp->node, uuid_hash(&dp->header_.uuid)); + return t_dp; +} + +static struct tracked_binding_datapath * +tracked_binding_datapath_find(struct hmap *tracked_datapaths, + const struct sbrec_datapath_binding *dp) +{ + struct tracked_binding_datapath *t_dp; + size_t hash = uuid_hash(&dp->header_.uuid); + HMAP_FOR_EACH_WITH_HASH (t_dp, node, hash, tracked_datapaths) { + if (uuid_equals(&t_dp->dp->header_.uuid, &dp->header_.uuid)) { + return t_dp; + } + } + + return NULL; +} + +static void +tracked_binding_datapath_lport_add(struct hmap *tracked_datapaths, + const struct sbrec_port_binding *pb, + bool deleted) +{ + if (!tracked_datapaths) { + return; + } + + struct tracked_binding_datapath *tracked_dp = + tracked_binding_datapath_find(tracked_datapaths, pb->datapath); + if (!tracked_dp) { + tracked_dp = tracked_binding_datapath_create(tracked_datapaths, + pb->datapath, false); + } + struct tracked_binding_lport *lport = xmalloc(sizeof *lport); + lport->pb = pb; + lport->deleted = deleted; + ovs_list_push_back(&tracked_dp->lports_head, &lport->list_node); +} + +void +binding_tracked_dp_destroy(struct hmap *tracked_datapaths) +{ + struct tracked_binding_datapath *t_dp; + HMAP_FOR_EACH_POP (t_dp, node, tracked_datapaths) { + struct tracked_binding_lport *lport, *next; + LIST_FOR_EACH_SAFE (lport, next, list_node, &t_dp->lports_head) { + ovs_list_remove(&lport->list_node); + free(lport); + } + free(t_dp); + } + + hmap_destroy(tracked_datapaths); +} + void binding_add_vport_to_local_bindings(struct shash *local_bindings, const struct sbrec_port_binding *parent, @@ -668,6 +748,7 @@ release_lport(const struct sbrec_port_binding *pb, bool cant_update_sb) static bool release_local_binding_children(struct local_binding *lbinding, + struct hmap *tracked_dp_bindings, bool cant_update_sb) { struct local_binding *l; @@ -675,15 +756,23 @@ release_local_binding_children(struct local_binding *lbinding, if (!release_lport(l->pb, cant_update_sb)) { return false; } + if (tracked_dp_bindings) { + tracked_binding_datapath_lport_add(tracked_dp_bindings, l->pb, + true); + } } return true; } static bool -release_local_binding(struct local_binding *lbinding, bool cant_update_sb) +release_local_binding(struct local_binding *lbinding, + struct hmap *tracked_dp_bindings, + bool cant_update_sb) { - if (!release_local_binding_children(lbinding, cant_update_sb)) { + if (!release_local_binding_children(lbinding, + tracked_dp_bindings, + cant_update_sb)) { return false; } @@ -691,6 +780,10 @@ release_local_binding(struct local_binding *lbinding, bool cant_update_sb) return false; } + if (tracked_dp_bindings) { + tracked_binding_datapath_lport_add(tracked_dp_bindings, lbinding->pb, + true); + } return true; } @@ -742,6 +835,10 @@ consider_port_binding_for_vif(const struct sbrec_port_binding *pb, child = local_binding_create(pb->logical_port, lbinding->iface, pb, binding_type); local_binding_add_child(lbinding, child); + if (b_ctx_out->tracked_dp_bindings) { + tracked_binding_datapath_lport_add( + b_ctx_out->tracked_dp_bindings, pb, false); + } } else { ovs_assert(child->type == BT_CHILD || child->type == BT_VIRTUAL); @@ -757,7 +854,8 @@ consider_port_binding_for_vif(const struct sbrec_port_binding *pb, add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, b_ctx_in->sbrec_port_binding_by_datapath, b_ctx_in->sbrec_port_binding_by_name, - pb->datapath, false, b_ctx_out->local_datapaths); + pb->datapath, false, b_ctx_out->local_datapaths, + b_ctx_out->tracked_dp_bindings); update_local_lport_ids(b_ctx_out->local_lport_ids, pb); if (lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { get_qos_params(pb, qos_map); @@ -801,7 +899,8 @@ consider_port_binding(const struct sbrec_port_binding *pb, b_ctx_in->sbrec_port_binding_by_datapath, b_ctx_in->sbrec_port_binding_by_name, pb->datapath, false, - b_ctx_out->local_datapaths); + b_ctx_out->local_datapaths, + b_ctx_out->tracked_dp_bindings); } } else if (!strcmp(pb->type, "chassisredirect")) { if (ha_chassis_group_contains(pb->ha_chassis_group, @@ -810,14 +909,16 @@ consider_port_binding(const struct sbrec_port_binding *pb, b_ctx_in->sbrec_port_binding_by_datapath, b_ctx_in->sbrec_port_binding_by_name, pb->datapath, false, - b_ctx_out->local_datapaths); + b_ctx_out->local_datapaths, + b_ctx_out->tracked_dp_bindings); } } else if (!strcmp(pb->type, "l3gateway")) { if (our_chassis) { add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, b_ctx_in->sbrec_port_binding_by_datapath, b_ctx_in->sbrec_port_binding_by_name, - pb->datapath, true, b_ctx_out->local_datapaths); + pb->datapath, true, b_ctx_out->local_datapaths, + b_ctx_out->tracked_dp_bindings); } } else if (!strcmp(pb->type, "localnet")) { /* Add all localnet ports to local_lports so that we allocate ct zones @@ -833,7 +934,8 @@ consider_port_binding(const struct sbrec_port_binding *pb, b_ctx_in->sbrec_port_binding_by_datapath, b_ctx_in->sbrec_port_binding_by_name, pb->datapath, false, - b_ctx_out->local_datapaths); + b_ctx_out->local_datapaths, + b_ctx_out->tracked_dp_bindings); } } @@ -972,6 +1074,9 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) consider_port_binding_for_vif(binding_rec, b_ctx_in, binding_type, lbinding, b_ctx_out, qos_map_ptr); + if (lbinding && lbinding->pb && + lbinding->pb->chassis == b_ctx_in->chassis_rec) { + } } else { consider_port_binding(binding_rec, b_ctx_in, b_ctx_out, qos_map_ptr); @@ -1088,7 +1193,8 @@ add_local_datapath_peer_port(const struct sbrec_port_binding *pb, b_ctx_in->sbrec_port_binding_by_datapath, b_ctx_in->sbrec_port_binding_by_name, peer->datapath, false, - 1, b_ctx_out->local_datapaths); + 1, b_ctx_out->local_datapaths, + b_ctx_out->tracked_dp_bindings); return; } @@ -1211,6 +1317,7 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, struct hmap *qos_map_ptr = sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; + *b_ctx_out->local_lports_changed = false; const struct ovsrec_interface *iface_rec; OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec, b_ctx_in->iface_table) { @@ -1253,6 +1360,7 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, lbinding->pb->chassis == b_ctx_in->chassis_rec) { if (!release_local_binding(lbinding, + b_ctx_out->tracked_dp_bindings, !b_ctx_in->ovnsb_idl_txn)) { handled = false; goto out; @@ -1265,12 +1373,14 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, b_ctx_in->chassis_rec, b_ctx_out, ld); } + local_binding_delete(b_ctx_out->local_bindings, lbinding); *changed = true; } sset_find_and_delete(b_ctx_out->local_lports, cleared_iface_id); smap_remove(b_ctx_out->local_iface_ids, iface_rec->name); + *b_ctx_out->local_lports_changed = true; } if (iface_id && ofport > 0) { @@ -1278,6 +1388,7 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, sset_add(b_ctx_out->local_lports, iface_id); smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id); + *b_ctx_out->local_lports_changed = true; claim_lbinding = local_binding_find(b_ctx_out->local_bindings, iface_id); if (!claim_lbinding) { @@ -1301,6 +1412,8 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, } if (claim_lbinding->pb) { + bool claimed = + (claim_lbinding->pb->chassis == b_ctx_in->chassis_rec); if (!consider_port_binding_for_vif(claim_lbinding->pb, b_ctx_in, BT_VIF, claim_lbinding, @@ -1308,6 +1421,14 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, handled = false; goto out; } + bool now_claimed = (claim_lbinding->pb && ( + claim_lbinding->pb->chassis == b_ctx_in->chassis_rec)); + if (!claimed && now_claimed) { + /* Add this to the updated tracked datapath bindings. */ + tracked_binding_datapath_lport_add( + b_ctx_out->tracked_dp_bindings, claim_lbinding->pb, + false); + } } } } @@ -1371,9 +1492,13 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in, if (is_deleted) { if (lbinding) { lbinding->pb = NULL; + /* Add this to the deleted tracked datapath bindings. */ + tracked_binding_datapath_lport_add( + b_ctx_out->tracked_dp_bindings, pb, true); if (binding_type == BT_VIF && !release_local_binding_children( - lbinding, !b_ctx_in->ovnsb_idl_txn)) { + lbinding, b_ctx_out->tracked_dp_bindings, + !b_ctx_in->ovnsb_idl_txn)) { handled = false; break; } @@ -1401,6 +1526,11 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in, bool now_claimed = (pb->chassis == b_ctx_in->chassis_rec); if (!claimed && now_claimed) { *changed = true; + + /* Add this to the updated tracked datapath + * bindings. */ + tracked_binding_datapath_lport_add( + b_ctx_out->tracked_dp_bindings, pb, false); } } } else { diff --git a/controller/binding.h b/controller/binding.h index fda6807235..2f79dd63cc 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -18,6 +18,9 @@ #define OVN_BINDING_H 1 #include +#include "openvswitch/hmap.h" +#include "openvswitch/uuid.h" +#include "openvswitch/list.h" struct hmap; struct ovsdb_idl; @@ -57,6 +60,8 @@ struct binding_ctx_out { struct sset *local_lport_ids; struct sset *egress_ifaces; struct smap *local_iface_ids; + struct hmap *tracked_dp_bindings; + bool *local_lports_changed; }; void binding_register_ovs_idl(struct ovsdb_idl *); @@ -74,4 +79,5 @@ bool binding_handle_ovs_interface_changes(struct binding_ctx_in *, bool binding_handle_port_binding_changes(struct binding_ctx_in *, struct binding_ctx_out *, bool *changed); +void binding_tracked_dp_destroy(struct hmap *tracked_datapaths); #endif /* controller/binding.h */ diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 127c4e2bb2..4682dd72c6 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -978,6 +978,23 @@ struct ed_type_runtime_data { struct smap local_iface_ids; }; +struct ed_type_runtime_tracked_data { + struct hmap tracked_dp_bindings; + bool local_lports_changed; + bool tracked; +}; + +static void +en_runtime_clear_tracked_data(void *tracked_data) +{ + struct ed_type_runtime_tracked_data *data = tracked_data; + + binding_tracked_dp_destroy(&data->tracked_dp_bindings); + hmap_init(&data->tracked_dp_bindings); + data->local_lports_changed = false; + data->tracked = false; +} + static void * en_runtime_data_init(struct engine_node *node OVS_UNUSED, struct engine_arg *arg OVS_UNUSED) @@ -991,6 +1008,13 @@ en_runtime_data_init(struct engine_node *node OVS_UNUSED, sset_init(&data->egress_ifaces); shash_init(&data->local_bindings); smap_init(&data->local_iface_ids); + + struct ed_type_runtime_tracked_data *tracked_data = + xzalloc(sizeof *tracked_data); + hmap_init(&tracked_data->tracked_dp_bindings); + node->tracked_data = tracked_data; + node->clear_tracked_data = en_runtime_clear_tracked_data; + return data; } @@ -1093,6 +1117,8 @@ init_binding_ctx(struct engine_node *node, b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; b_ctx_out->local_bindings = &rt_data->local_bindings; b_ctx_out->local_iface_ids = &rt_data->local_iface_ids; + b_ctx_out->tracked_dp_bindings = NULL; + b_ctx_out->local_lports_changed = NULL; } static void @@ -1104,6 +1130,8 @@ en_runtime_data_run(struct engine_node *node, void *data) struct sset *local_lport_ids = &rt_data->local_lport_ids; struct sset *active_tunnels = &rt_data->active_tunnels; + en_runtime_clear_tracked_data(node->tracked_data); + static bool first_run = true; if (first_run) { /* don't cleanup since there is no data yet */ @@ -1154,9 +1182,13 @@ static bool runtime_data_ovs_interface_handler(struct engine_node *node, void *data) { struct ed_type_runtime_data *rt_data = data; + struct ed_type_runtime_tracked_data *tracked_data = node->tracked_data; struct binding_ctx_in b_ctx_in; struct binding_ctx_out b_ctx_out; init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out); + tracked_data->tracked = true; + b_ctx_out.tracked_dp_bindings = &tracked_data->tracked_dp_bindings; + b_ctx_out.local_lports_changed = &tracked_data->local_lports_changed; bool changed = false; if (!binding_handle_ovs_interface_changes(&b_ctx_in, &b_ctx_out, @@ -1189,6 +1221,10 @@ runtime_data_sb_port_binding_handler(struct engine_node *node, void *data) return false; } + struct ed_type_runtime_tracked_data *tracked_data = node->tracked_data; + tracked_data->tracked = true; + b_ctx_out.tracked_dp_bindings = &tracked_data->tracked_dp_bindings; + bool changed = false; if (!binding_handle_port_binding_changes(&b_ctx_in, &b_ctx_out, &changed)) { @@ -1863,6 +1899,29 @@ physical_flow_changes_ct_zones_handler(struct engine_node *node OVS_UNUSED, return false; } +static bool +flow_output_runtime_data_handler(struct engine_node *node, + void *data OVS_UNUSED) +{ + struct ed_type_runtime_tracked_data *tracked_data = + engine_get_input_tracked_data("runtime_data", node); + + if (!tracked_data || !tracked_data->tracked) { + 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 (tracked_data->local_lports_changed) { + engine_set_node_state(node, EN_UPDATED); + } + return true; +} + struct ovn_controller_exit_args { bool *exiting; bool *restart; @@ -2014,7 +2073,8 @@ main(int argc, char *argv[]) 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_runtime_data, + flow_output_runtime_data_handler); 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); diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h index 40c358cef0..6c40a64922 100644 --- a/controller/ovn-controller.h +++ b/controller/ovn-controller.h @@ -84,7 +84,6 @@ struct local_binding { enum local_binding_type type; const struct ovsrec_interface *iface; const struct sbrec_port_binding *pb; - struct ovs_list children; }; @@ -94,6 +93,21 @@ local_binding_find(struct shash *local_bindings, const char *name) return shash_find_data(local_bindings, name); } +/* Represents a tracked binding logical port. */ +struct tracked_binding_lport { + const struct sbrec_port_binding *pb; + struct ovs_list list_node; + bool deleted; +}; + +/* Represent a tracked binding datapath. */ +struct tracked_binding_datapath { + struct hmap_node node; + const struct sbrec_datapath_binding *dp; + bool is_new; + struct ovs_list lports_head; /* List of struct tracked_binding_lport. */ +}; + const struct ovsrec_bridge *get_bridge(const struct ovsrec_bridge_table *, const char *br_name); diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at index 5cc1960b64..6df42ce3a1 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,15 +282,15 @@ 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] ) - OVN_CONTROLLER_EXPECT_HIT( + OVN_CONTROLLER_EXPECT_NO_HIT( [hv1 hv2], [lflow_run], [ovn-nbctl --wait=hv lsp-set-options $lsp router-port=$lrp] ) - OVN_CONTROLLER_EXPECT_HIT( + OVN_CONTROLLER_EXPECT_NO_HIT( [hv1 hv2], [lflow_run], [ovn-nbctl --wait=hv lsp-set-addresses $lsp router] ) @@ -416,15 +416,15 @@ OVN_CONTROLLER_EXPECT_NO_HIT( [ovn-nbctl --wait=hv destroy Port_Group pg1] ) -for i in 1 2; do - ls=ls$i +OVN_CONTROLLER_EXPECT_HIT( + [hv1 hv2], [lflow_run], + [ovn-nbctl --wait=hv ls-del ls1] +) - # Delete switch $ls - OVN_CONTROLLER_EXPECT_HIT( - [hv1 hv2], [lflow_run], - [ovn-nbctl --wait=hv ls-del $ls] - ) -done +OVN_CONTROLLER_EXPECT_NO_HIT( + [hv1 hv2], [lflow_run], + [ovn-nbctl --wait=hv ls-del ls2] +) # Delete router lr1 OVN_CONTROLLER_EXPECT_NO_HIT( From dbd65b4ae581e6c412f2f39530460c9fa149e7b1 Mon Sep 17 00:00:00 2001 From: Venkata Anil Date: Fri, 10 Apr 2020 16:30:52 +0530 Subject: [PATCH 9/9] I-P: Use the tracked runtime data changes for flow calculation. 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 Signed-off-by: Venkata Anil Signed-off-by: Numan Siddique --- controller/lflow.c | 69 ++++++++++++++++++++- controller/lflow.h | 12 +++- controller/ovn-controller.c | 116 +++++++++++++++++++----------------- controller/physical.h | 3 +- tests/ovn-performance.at | 8 +-- 5 files changed, 146 insertions(+), 62 deletions(-) diff --git a/controller/lflow.c b/controller/lflow.c index 512258cd3b..63c2e60634 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -258,7 +258,7 @@ lflow_resource_destroy_lflow(struct lflow_resource_ref *lfrr, /* Adds the logical flows from the Logical_Flow table to flow tables. */ static void add_logical_flows(struct lflow_ctx_in *l_ctx_in, - struct lflow_ctx_out *l_ctx_out) + struct lflow_ctx_out *l_ctx_out) { const struct sbrec_logical_flow *lflow; @@ -649,6 +649,8 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, int64_t dp_id = lflow->logical_datapath->tunnel_key; char buf[16]; snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, dp_id, port_id); + lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTBINDING, buf, + &lflow->header_.uuid); if (!sset_contains(l_ctx_in->local_lport_ids, buf)) { VLOG_DBG("lflow "UUID_FMT " port %s in match is not local, skip", @@ -860,3 +862,68 @@ lflow_evaluate_pb_changes(const struct sbrec_port_binding_table *pb_table) return true; } + +bool +lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp, + struct lflow_ctx_in *l_ctx_in, + struct lflow_ctx_out *l_ctx_out) +{ + bool handled = true; + struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); + struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); + const struct sbrec_dhcp_options *dhcp_opt_row; + SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, + l_ctx_in->dhcp_options_table) { + dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code, + dhcp_opt_row->type); + } + + + const struct sbrec_dhcpv6_options *dhcpv6_opt_row; + SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row, + l_ctx_in->dhcpv6_options_table) { + dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, dhcpv6_opt_row->code, + dhcpv6_opt_row->type); + } + + struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts); + nd_ra_opts_init(&nd_ra_opts); + + struct controller_event_options controller_event_opts; + controller_event_opts_init(&controller_event_opts); + + struct sbrec_logical_flow *lf_row = sbrec_logical_flow_index_init_row( + l_ctx_in->sbrec_logical_flow_by_logical_datapath); + sbrec_logical_flow_index_set_logical_datapath(lf_row, dp); + + const struct sbrec_logical_flow *lflow; + SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL( + lflow, lf_row, l_ctx_in->sbrec_logical_flow_by_logical_datapath) { + if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts, + &nd_ra_opts, &controller_event_opts, + l_ctx_in, l_ctx_out)) { + handled = false; + break; + } + } + + dhcp_opts_destroy(&dhcp_opts); + dhcp_opts_destroy(&dhcpv6_opts); + nd_ra_opts_destroy(&nd_ra_opts); + controller_event_opts_destroy(&controller_event_opts); + return handled; +} + +bool +lflow_handle_flows_for_lport(const struct sbrec_port_binding *pb, + struct lflow_ctx_in *l_ctx_in, + struct lflow_ctx_out *l_ctx_out) +{ + int64_t dp_id = pb->datapath->tunnel_key; + char pb_ref_name[16]; + snprintf(pb_ref_name, sizeof(pb_ref_name), "%"PRId64"_%"PRId64, + dp_id, pb->tunnel_key); + bool changed = true; + return lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb_ref_name, + l_ctx_in, l_ctx_out, &changed); +} diff --git a/controller/lflow.h b/controller/lflow.h index fa54d4de28..53ecc45b46 100644 --- a/controller/lflow.h +++ b/controller/lflow.h @@ -49,6 +49,8 @@ struct sbrec_dhcpv6_options_table; struct sbrec_logical_flow_table; struct sbrec_mac_binding_table; struct sbrec_port_binding_table; +struct sbrec_datapath_binding; +struct sbrec_port_binding; struct simap; struct sset; struct uuid; @@ -73,7 +75,8 @@ struct uuid; enum ref_type { REF_TYPE_ADDRSET, - REF_TYPE_PORTGROUP + REF_TYPE_PORTGROUP, + REF_TYPE_PORTBINDING }; /* Maintains the relationship for a pair of named resource and @@ -118,6 +121,7 @@ void lflow_resource_clear(struct lflow_resource_ref *); struct lflow_ctx_in { struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath; + struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath; struct ovsdb_idl_index *sbrec_port_binding_by_name; const struct sbrec_dhcp_options_table *dhcp_options_table; const struct sbrec_dhcpv6_options_table *dhcpv6_options_table; @@ -159,4 +163,10 @@ void lflow_destroy(void); void lflow_expr_destroy(struct hmap *lflow_expr_cache); +bool lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *, + struct lflow_ctx_in *, + struct lflow_ctx_out *); +bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *, + struct lflow_ctx_in *, + struct lflow_ctx_out *); #endif /* controller/lflow.h */ diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 4682dd72c6..a62b2d3e30 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1439,6 +1439,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), @@ -1486,6 +1491,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; @@ -1640,7 +1647,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); @@ -1648,54 +1656,9 @@ flow_output_sb_port_binding_handler(struct engine_node *node, void *data) struct ed_type_flow_output *fo = data; struct ovn_desired_flow_table *flow_table = &fo->flow_table; - /* XXX: now we handle port-binding changes for physical flow processing - * only, but port-binding change can have impact to logical flow - * processing, too, in below circumstances: - * - * - When a port-binding for a lport is inserted/deleted but the lflow - * using that lport doesn't change. - * - * This can happen only when the lport name is used by ACL match - * condition, which is specified by user. Even in that case, if the port - * is actually bound on the current chassis it will trigger recompute on - * that chassis since ovs interface would be updated. So the only - * situation this would have real impact is when user defines an ACL - * that includes lport that is not on current chassis, and there is a - * port-binding creation/deletion related to that lport.e.g.: an ACL is - * defined: - * - * to-lport 1000 'outport=="A" && inport=="B"' allow-related - * - * If "A" is on current chassis, but "B" is lport that hasn't been - * created yet. When a lport "B" is created and bound on another - * chassis, the ACL will not take effect on the current chassis until a - * recompute is triggered later. This case doesn't seem to be a problem - * for real world use cases because usually lport is created before - * being referenced by name in ACLs. - * - * - When is_chassis_resident() is used in lflow. In this case the - * port binding is not a regular VIF. It can be either "patch" or - * "external", with ha-chassis-group assigned. In current - * "runtime_data" handling, port-binding changes for these types always - * trigger recomputing. So it is fine even if we do not handle it here. - * (due to the ovsdb tracking support for referenced table changes, - * ha-chassis-group changes will appear as port-binding change). - * - * - When a mac-binding doesn't change but the port-binding related to - * that mac-binding is deleted. In this case the neighbor flow generated - * for the mac-binding should be deleted. This would not cause any real - * issue for now, since the port-binding related to mac-binding is - * always logical router port, and any change to logical router port - * would just trigger recompute. - * - * - When a port binding of type 'remote' is updated by ovn-ic. - * - * Although there is no correctness issue so far (except the unusual ACL - * use case, which doesn't seem to be a real problem), it might be better - * to handle this more gracefully, without the need to consider these - * tricky scenarios. One approach is to maintain a mapping between lport - * names and the lflows that uses them, and reprocess the related lflows - * when related port-bindings change. + /* We handle port-binding changes for physical flow processing + * only. flow_output runtime data handler takes care of processing + * logical flows for any port binding changes. */ struct physical_ctx p_ctx; init_physical_ctx(node, rt_data, &p_ctx); @@ -1795,6 +1758,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(); } @@ -1910,16 +1875,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 +1977,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 +2129,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 9ca34436a5..481f039013 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 6df42ce3a1..a12757e18c 100644 --- a/tests/ovn-performance.at +++ b/tests/ovn-performance.at @@ -353,8 +353,8 @@ for i in 1 2; do ) # Bind port $lp and wait for it to come up - OVN_CONTROLLER_EXPECT_HIT_COND( - [hv$i hv$j], [lflow_run], [>0 =0], + OVN_CONTROLLER_EXPECT_NO_HIT( + [hv$i hv$j], [lflow_run], [as hv$i ovs-vsctl add-port br-int $vif -- set Interface $vif external-ids:iface-id=$lp && ovn-nbctl wait-until Logical_Switch_Port $lp 'up=true' && ovn-nbctl --wait=hv sync] @@ -404,8 +404,8 @@ for i in 1 2; do lp=lp$i # Delete port $lp - OVN_CONTROLLER_EXPECT_HIT_COND( - [hv$i hv$j], [lflow_run], [>0 =0], + OVN_CONTROLLER_EXPECT_NO_HIT( + [hv$i hv$j], [lflow_run], [ovn-nbctl --wait=hv lsp-del $lp] ) done