[ovs-dev] [PATCH branch-2.12 1/4] ovn-controller: Consider non-virtual ports first when updating bindings.
Dumitru Ceara
dceara at redhat.com
Tue Apr 7 11:03:29 UTC 2020
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 <numans at ovn.org>
Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'")
Signed-off-by: Dumitru Ceara <dceara at redhat.com>
Acked-by: Mark Michelson <mmichels at redhat.com>
Signed-off-by: Numan Siddique <numans at ovn.org>
(cherry picked from OVN 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 dfe002b..ef8445f 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -460,7 +460,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,
@@ -473,7 +478,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);
@@ -572,22 +579,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);
@@ -606,6 +602,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
@@ -662,20 +682,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);
/* Run through each binding record to see if it is a localnet port
* on local datapaths discovered from above loop, and update the
More information about the dev
mailing list