diff mbox series

[ovs-dev,branch-2.12,1/4] ovn-controller: Consider non-virtual ports first when updating bindings.

Message ID 20200407110320.7801.36179.stgit@dceara.remote.csb
State New
Headers show
Series OVN: Backport virtual port related fixes. | expand

Commit Message

Dumitru Ceara April 7, 2020, 11:03 a.m. UTC
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@ovn.org>
Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'")
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Acked-by: Mark Michelson <mmichels@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>

(cherry picked from OVN commit 5309099ec38cf41f4e41f1929c408741a3146dac)
---
 ovn/controller/binding.c |   97 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 69 insertions(+), 28 deletions(-)

Comments

0-day Robot April 7, 2020, 1 p.m. UTC | #1
Bleep bloop.  Greetings Dumitru Ceara, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Numan Siddique <numans@ovn.org>
Lines checked: 178, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
diff mbox series

Patch

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