Message ID | 20200511094555.915049-1-numans@ovn.org |
---|---|
State | Superseded |
Headers | show |
Series | Incremental processing improvements. | expand |
On 5/11/20 11:45 AM, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > 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. Hi Numan, This is not a full review as I only went through half of the changes in this patch but please find some initial comments inline. Thanks, Dumitru > > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > controller/binding.c | 602 ++++++++++++++++++++++++++++++------ > controller/binding.h | 9 +- > controller/ovn-controller.c | 61 +++- > tests/ovn-performance.at | 13 + > 4 files changed, 593 insertions(+), 92 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index e35440c78..662424518 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=<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, > @@ -530,18 +541,34 @@ 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); > +} > + > +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); > @@ -594,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) I think "sb_readonly" would be more explicit than "cant_update_sb". > { > if (pb->chassis != chassis_rec) { > if (pb->chassis) { > @@ -611,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); > } > > @@ -618,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) Same here. > { > VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port); > if (pb->encap) { > - sbrec_port_binding_set_encap(pb, NULL); > + if (pb->encap) { I guess this is a typo, i.e., "if (pb->encap) { 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, > @@ -644,6 +724,12 @@ consider_port_binding_for_vif(const struct sbrec_port_binding *pb, > struct binding_ctx_out *b_ctx_out, > struct hmap *qos_map) > { > + if (pb->type[0] && strcmp(pb->type, "virtual")) { > + /* The port binding type should be empty or 'virtual' > + * to consider it for port binding here. */ > + return true; > + } In binding_run() we do the check outside consider_port_binding_for_vif() but in binding_handle_ovs_interface_changes() we consider_port_binding_for_vif() check the PB type. It would be nice to decide on one of the two ways. > + > 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) > @@ -660,11 +746,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: > @@ -712,22 +801,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; We don't really need the "success" variable, we can just return claim_lport()/release_lport() below or false at the end of the function. > if (!strcmp(pb->type, "l2gateway")) { > if (our_chassis) { > sset_add(b_ctx_out->local_lports, pb->logical_port); > @@ -775,12 +868,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 > @@ -813,6 +908,8 @@ build_local_bindings_from_local_ifaces(struct binding_ctx_in *b_ctx_in, > BT_VIF); > local_binding_add(b_ctx_out->local_bindings, lbinding); > 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. */ > @@ -921,60 +1018,6 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > hmap_destroy(&qos_map); Unrelated to your change but I think hmap_destroy(&qos_map) above is not enough at it will not free the memory allocated for the nodes. > } > > -/* 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 > @@ -1009,3 +1052,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: I think hmap_destroy(&qos_map) is not enough because it will not free the nodes in the qos_map, if any. Also, the "out" label should be before cleaning up the qos_map otherwise we'll leak memory when consider_port_binding_for_vif() returns false. > + 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 6bee1d12e..fda680723 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 36a1cadb9..37ffc399c 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 > @@ -1111,11 +1121,13 @@ 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); > shash_init(&rt_data->local_bindings); > + smap_init(&rt_data->local_iface_ids); > } > > struct binding_ctx_in b_ctx_in; > @@ -1140,6 +1152,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) > { > @@ -1147,11 +1187,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. */ > @@ -1893,7 +1943,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 a8a15f8fe..5dfc6f7ca 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], >
On 5/11/20 5:46 PM, Dumitru Ceara wrote: > On 5/11/20 11:45 AM, numans@ovn.org wrote: >> From: Numan Siddique <numans@ovn.org> >> >> 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. > > Hi Numan, > > This is not a full review as I only went through half of the changes in > this patch but please find some initial comments inline. > > Thanks, > Dumitru > >> >> Signed-off-by: Numan Siddique <numans@ovn.org> >> --- >> controller/binding.c | 602 ++++++++++++++++++++++++++++++------ >> controller/binding.h | 9 +- >> controller/ovn-controller.c | 61 +++- >> tests/ovn-performance.at | 13 + >> 4 files changed, 593 insertions(+), 92 deletions(-) >> >> diff --git a/controller/binding.c b/controller/binding.c >> index e35440c78..662424518 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=<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, >> @@ -530,18 +541,34 @@ 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); >> +} >> + >> +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); >> @@ -594,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) > > I think "sb_readonly" would be more explicit than "cant_update_sb". > >> { >> if (pb->chassis != chassis_rec) { >> if (pb->chassis) { >> @@ -611,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); >> } >> >> @@ -618,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) > > Same here. > >> { >> VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port); >> if (pb->encap) { >> - sbrec_port_binding_set_encap(pb, NULL); >> + if (pb->encap) { > > I guess this is a typo, i.e., "if (pb->encap) { 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, >> @@ -644,6 +724,12 @@ consider_port_binding_for_vif(const struct sbrec_port_binding *pb, >> struct binding_ctx_out *b_ctx_out, >> struct hmap *qos_map) >> { >> + if (pb->type[0] && strcmp(pb->type, "virtual")) { >> + /* The port binding type should be empty or 'virtual' >> + * to consider it for port binding here. */ >> + return true; >> + } > > In binding_run() we do the check outside consider_port_binding_for_vif() > but in binding_handle_ovs_interface_changes() we > consider_port_binding_for_vif() check the PB type. It would be nice to > decide on one of the two ways. > >> + >> 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) >> @@ -660,11 +746,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: >> @@ -712,22 +801,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; >> + } We could just "return release_lport(pb, !b_ctx_in->ovnsb_idl_txn);" here instead. >> } >> } >> + >> + 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; > > We don't really need the "success" variable, we can just return > claim_lport()/release_lport() below or false at the end of the function. > >> if (!strcmp(pb->type, "l2gateway")) { >> if (our_chassis) { >> sset_add(b_ctx_out->local_lports, pb->logical_port); >> @@ -775,12 +868,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 >> @@ -813,6 +908,8 @@ build_local_bindings_from_local_ifaces(struct binding_ctx_in *b_ctx_in, >> BT_VIF); >> local_binding_add(b_ctx_out->local_bindings, lbinding); >> 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. */ >> @@ -921,60 +1018,6 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) >> hmap_destroy(&qos_map); > > Unrelated to your change but I think hmap_destroy(&qos_map) above is not > enough at it will not free the memory allocated for the nodes. > >> } >> >> -/* 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 >> @@ -1009,3 +1052,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; >> + } >> + It's probably a bit more efficient and a bit more readable to search for the port binding (i.e., compute "present") here instead of the beginning of the function. >> + 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; Nit: s/i =0/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--; >> + Should we consider reallocating the peer_ports array if n_peer_ports < n_allocated_peer_ports / 2 ? Regards, Dumitru >> + 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: > > I think hmap_destroy(&qos_map) is not enough because it will not free > the nodes in the qos_map, if any. Also, the "out" label should be before > cleaning up the qos_map otherwise we'll leak memory when > consider_port_binding_for_vif() returns false. > >> + 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 6bee1d12e..fda680723 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 36a1cadb9..37ffc399c 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 >> @@ -1111,11 +1121,13 @@ 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); >> shash_init(&rt_data->local_bindings); >> + smap_init(&rt_data->local_iface_ids); >> } >> >> struct binding_ctx_in b_ctx_in; >> @@ -1140,6 +1152,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) >> { >> @@ -1147,11 +1187,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. */ >> @@ -1893,7 +1943,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 a8a15f8fe..5dfc6f7ca 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], >> >
On Tue, May 12, 2020 at 8:58 PM Dumitru Ceara <dceara@redhat.com> wrote: > On 5/11/20 5:46 PM, Dumitru Ceara wrote: > > On 5/11/20 11:45 AM, numans@ovn.org wrote: > >> From: Numan Siddique <numans@ovn.org> > >> > >> 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. > > > Thanks for the reviews. > > Hi Numan, > > > > This is not a full review as I only went through half of the changes in > > this patch but please find some initial comments inline. > > > > Thanks, > > Dumitru > > > >> > >> Signed-off-by: Numan Siddique <numans@ovn.org> > >> --- > >> controller/binding.c | 602 ++++++++++++++++++++++++++++++------ > >> controller/binding.h | 9 +- > >> controller/ovn-controller.c | 61 +++- > >> tests/ovn-performance.at | 13 + > >> 4 files changed, 593 insertions(+), 92 deletions(-) > >> > >> diff --git a/controller/binding.c b/controller/binding.c > >> index e35440c78..662424518 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=<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, > >> @@ -530,18 +541,34 @@ 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); > >> +} > >> + > >> +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); > >> @@ -594,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) > > > > I think "sb_readonly" would be more explicit than "cant_update_sb". > > > Ack. Done in v6. > >> { > >> if (pb->chassis != chassis_rec) { > >> if (pb->chassis) { > >> @@ -611,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); > >> } > >> > >> @@ -618,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) > > > > Same here. > Ack. Done in v6. > > > >> { > >> VLOG_INFO("Releasing lport %s from this chassis.", > pb->logical_port); > >> if (pb->encap) { > >> - sbrec_port_binding_set_encap(pb, NULL); > >> + if (pb->encap) { > > > > I guess this is a typo, i.e., "if (pb->encap) { 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, > >> @@ -644,6 +724,12 @@ consider_port_binding_for_vif(const struct > sbrec_port_binding *pb, > >> struct binding_ctx_out *b_ctx_out, > >> struct hmap *qos_map) > >> { > >> + if (pb->type[0] && strcmp(pb->type, "virtual")) { > >> + /* The port binding type should be empty or 'virtual' > >> + * to consider it for port binding here. */ > >> + return true; > >> + } > > > > In binding_run() we do the check outside consider_port_binding_for_vif() > > but in binding_handle_ovs_interface_changes() we > > consider_port_binding_for_vif() check the PB type. It would be nice to > > decide on one of the two ways. > This function is not there anymore in v6. > > > >> + > >> 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) > >> @@ -660,11 +746,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: > >> @@ -712,22 +801,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; > >> + } > > We could just "return release_lport(pb, !b_ctx_in->ovnsb_idl_txn);" here > instead. > Ack. Done in v6. > > >> } > >> } > >> + > >> + 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; > > > > We don't really need the "success" variable, we can just return > > claim_lport()/release_lport() below or false at the end of the function. > > > >> if (!strcmp(pb->type, "l2gateway")) { > >> if (our_chassis) { > >> sset_add(b_ctx_out->local_lports, pb->logical_port); > >> @@ -775,12 +868,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 > >> @@ -813,6 +908,8 @@ build_local_bindings_from_local_ifaces(struct > binding_ctx_in *b_ctx_in, > >> BT_VIF); > >> local_binding_add(b_ctx_out->local_bindings, lbinding); > >> 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. */ > >> @@ -921,60 +1018,6 @@ binding_run(struct binding_ctx_in *b_ctx_in, > struct binding_ctx_out *b_ctx_out) > >> hmap_destroy(&qos_map); > > > > Unrelated to your change but I think hmap_destroy(&qos_map) above is not > > enough at it will not free the memory allocated for the nodes. > Thanks for pointing this out. I submitted the patch to fix this memory leak. > > > >> } > >> > >> -/* 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 > >> @@ -1009,3 +1052,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; > >> + } > >> + > > It's probably a bit more efficient and a bit more readable to search for > the port binding (i.e., compute "present") here instead of the beginning > of the function. > Ack. Done in v6. > > >> + 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; > > Nit: s/i =0/i = 0/ > Done in v6. > > >> + 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--; > >> + > > Should we consider reallocating the peer_ports array if n_peer_ports < > n_allocated_peer_ports / 2 ? > > I was not really sure if it was worth this. Probably it is. In v6, I added a comment about possible improvement. We can probably revisit it later. > Regards, > Dumitru > > >> + 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: > > > > I think hmap_destroy(&qos_map) is not enough because it will not free > > the nodes in the qos_map, if any. Also, the "out" label should be before > > cleaning up the qos_map otherwise we'll leak memory when > > consider_port_binding_for_vif() returns false. > > > Ack. Addressed ib v6. > >> + 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 6bee1d12e..fda680723 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 36a1cadb9..37ffc399c 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 > >> @@ -1111,11 +1121,13 @@ 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); > >> shash_init(&rt_data->local_bindings); > >> + smap_init(&rt_data->local_iface_ids); > >> } > >> > >> struct binding_ctx_in b_ctx_in; > >> @@ -1140,6 +1152,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) > >> { > >> @@ -1147,11 +1187,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. */ > >> @@ -1893,7 +1943,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 a8a15f8fe..5dfc6f7ca 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], > >> > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/controller/binding.c b/controller/binding.c index e35440c78..662424518 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=<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, @@ -530,18 +541,34 @@ 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); +} + +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); @@ -594,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) { @@ -611,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); } @@ -618,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, @@ -644,6 +724,12 @@ consider_port_binding_for_vif(const struct sbrec_port_binding *pb, struct binding_ctx_out *b_ctx_out, struct hmap *qos_map) { + if (pb->type[0] && strcmp(pb->type, "virtual")) { + /* The port binding type should be empty or 'virtual' + * to consider it for port binding here. */ + return true; + } + 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) @@ -660,11 +746,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: @@ -712,22 +801,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); @@ -775,12 +868,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 @@ -813,6 +908,8 @@ build_local_bindings_from_local_ifaces(struct binding_ctx_in *b_ctx_in, BT_VIF); local_binding_add(b_ctx_out->local_bindings, lbinding); 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. */ @@ -921,60 +1018,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 @@ -1009,3 +1052,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 6bee1d12e..fda680723 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 36a1cadb9..37ffc399c 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 @@ -1111,11 +1121,13 @@ 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); shash_init(&rt_data->local_bindings); + smap_init(&rt_data->local_iface_ids); } struct binding_ctx_in b_ctx_in; @@ -1140,6 +1152,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) { @@ -1147,11 +1187,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. */ @@ -1893,7 +1943,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 a8a15f8fe..5dfc6f7ca 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],