From 726f0c5749f6ba3e147397c768854db515f5899a Mon Sep 17 00:00:00 2001 From: Dumitru Ceara Date: Fri, 22 Nov 2019 15:22:25 +0100 Subject: [PATCH ovn] ovn-controller: Consider non-virtual ports first when updating bindings. There's no guarantee SBREC_PORT_BINDING_TABLE_FOR_EACH will first return the non-virtual ports and then virtual ports. In the case when a virtual port is processed before its virtual_parent, consider_local_datapath might not release it in the current ovn-controller iteration even though the virtual_parent gets released. Right now this doesn't trigger any functionality issue because releasing the virtual_parent triggers a change in the SB DB which will wake up ovn-controller and trigger a new computation which will also update the virtual port. However, this is suboptimal, and we can notice that often ovn-controller gets the SB update notification before the "transaction successful" notification. In such cases the incremental engine doesn't run (ovnsb_idl_txn == NULL) and a full recompute is scheduled for the next run. By batching the two SB updates in a single transaction we lower the risk of this situation happening. CC: Numan Siddique Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'") Signed-off-by: Dumitru Ceara Acked-by: Mark Michelson Signed-off-by: Numan Siddique (cherry picked from upstream commit 5309099ec38cf41f4e41f1929c408741a3146dac) --- ovn/controller/binding.c | 97 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 69 insertions(+), 28 deletions(-) diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c index 3adbfbe..1ae9151 100644 --- a/ovn/controller/binding.c +++ b/ovn/controller/binding.c @@ -461,7 +461,12 @@ is_our_chassis(const struct sbrec_chassis *chassis_rec, return our_chassis; } -static void +/* Updates 'binding_rec' and if the port binding is local also updates the + * local datapaths and ports. + * Updates and returns the array of local virtual ports that will require + * additional processing. + */ +static const struct sbrec_port_binding ** consider_local_datapath(struct ovsdb_idl_txn *ovnsb_idl_txn, struct ovsdb_idl_txn *ovs_idl_txn, struct ovsdb_idl_index *sbrec_datapath_binding_by_key, @@ -474,7 +479,9 @@ consider_local_datapath(struct ovsdb_idl_txn *ovnsb_idl_txn, struct hmap *local_datapaths, struct shash *lport_to_iface, struct sset *local_lports, - struct sset *local_lport_ids) + struct sset *local_lport_ids, + const struct sbrec_port_binding **vpbs, + size_t *n_vpbs, size_t *n_allocated_vpbs) { const struct ovsrec_interface *iface_rec = shash_find_data(lport_to_iface, binding_rec->logical_port); @@ -576,22 +583,11 @@ consider_local_datapath(struct ovsdb_idl_txn *ovnsb_idl_txn, } } else if (binding_rec->chassis == chassis_rec) { if (!strcmp(binding_rec->type, "virtual")) { - /* pinctrl module takes care of binding the ports - * of type 'virtual'. - * Release such ports if their virtual parents are no - * longer claimed by this chassis. */ - const struct sbrec_port_binding *parent - = lport_lookup_by_name(sbrec_port_binding_by_name, - binding_rec->virtual_parent); - if (!parent || parent->chassis != chassis_rec) { - VLOG_INFO("Releasing lport %s from this chassis.", - binding_rec->logical_port); - if (binding_rec->encap) { - sbrec_port_binding_set_encap(binding_rec, NULL); - } - sbrec_port_binding_set_chassis(binding_rec, NULL); - sbrec_port_binding_set_virtual_parent(binding_rec, NULL); + if (*n_vpbs == *n_allocated_vpbs) { + vpbs = x2nrealloc(vpbs, n_allocated_vpbs, sizeof *vpbs); } + vpbs[(*n_vpbs)] = binding_rec; + (*n_vpbs)++; } else { VLOG_INFO("Releasing lport %s from this chassis.", binding_rec->logical_port); @@ -610,6 +606,30 @@ consider_local_datapath(struct ovsdb_idl_txn *ovnsb_idl_txn, vif_chassis); } } + return vpbs; +} + +static void +consider_local_virtual_port(struct ovsdb_idl_index *sbrec_port_binding_by_name, + const struct sbrec_chassis *chassis_rec, + const struct sbrec_port_binding *binding_rec) +{ + /* pinctrl module takes care of binding the ports of type 'virtual'. + * Release such ports if their virtual parents are no longer claimed by + * this chassis. + */ + const struct sbrec_port_binding *parent = + lport_lookup_by_name(sbrec_port_binding_by_name, + binding_rec->virtual_parent); + if (!parent || parent->chassis != chassis_rec) { + VLOG_INFO("Releasing lport %s from this chassis.", + binding_rec->logical_port); + if (binding_rec->encap) { + sbrec_port_binding_set_encap(binding_rec, NULL); + } + sbrec_port_binding_set_chassis(binding_rec, NULL); + sbrec_port_binding_set_virtual_parent(binding_rec, NULL); + } } static void @@ -707,20 +727,41 @@ binding_run(struct ovsdb_idl_txn *ovnsb_idl_txn, &egress_ifaces); } + /* Array to store pointers to local virtual ports. It is populated by + * consider_local_datapath. + */ + const struct sbrec_port_binding **vpbs = NULL; + size_t n_vpbs = 0; + size_t n_allocated_vpbs = 0; + /* Run through each binding record to see if it is resident on this * chassis and update the binding accordingly. This includes both - * directly connected logical ports and children of those ports. */ + * directly connected logical ports and children of those ports. + * Virtual ports are just added to vpbs array and will be processed + * later. This is special case for virtual ports is needed in order to + * make sure we update the virtual_parent port bindings first. + */ SBREC_PORT_BINDING_TABLE_FOR_EACH (binding_rec, port_binding_table) { - consider_local_datapath(ovnsb_idl_txn, ovs_idl_txn, - sbrec_datapath_binding_by_key, - sbrec_port_binding_by_datapath, - sbrec_port_binding_by_name, - active_tunnels, chassis_rec, binding_rec, - sset_is_empty(&egress_ifaces) ? NULL : - &qos_map, local_datapaths, &lport_to_iface, - local_lports, local_lport_ids); - - } + vpbs = + consider_local_datapath(ovnsb_idl_txn, ovs_idl_txn, + sbrec_datapath_binding_by_key, + sbrec_port_binding_by_datapath, + sbrec_port_binding_by_name, + active_tunnels, chassis_rec, binding_rec, + sset_is_empty(&egress_ifaces) ? NULL : + &qos_map, local_datapaths, &lport_to_iface, + local_lports, local_lport_ids, + vpbs, &n_vpbs, &n_allocated_vpbs); + } + + /* Now also update the virtual ports in case their parent ports were + * updated above. + */ + for (size_t i = 0; i < n_vpbs; i++) { + consider_local_virtual_port(sbrec_port_binding_by_name, chassis_rec, + vpbs[i]); + } + free(vpbs); add_ovs_bridge_mappings(ovs_table, bridge_table, &bridge_mappings); -- 1.8.3.1