ovn-controller: Clean up bindings handling.

Remove the global set of logical port IDs called 'all_lports'.  This is
no longer used for anything after conntrack ID assignment was moved out
of binding.c.

Remove the global smap of logical port IDs to ovsrec_interface records.
We can't persist references to these records, as we may be holding
references to freed memory.  Instead, replace it with a new global sset
of logical port IDs called 'local_ids'.  This is used to track when
interfaces have been added or removed.  We also build a temporary
shash of logical port IDs to ovs interfaces used for fast lookup
of the right interface as needed.

Found by inspection.

Fixes: a478c4efef4d ("ovn-controller: Refactor conntrack zone allocation.")
Fixes: 263064aeaa31 ("Convert binding_run to incremental processing.")
Signed-off-by: Russell Bryant <russell@ovn.org>
Acked-by: Ryan Moats <rmoats@us.ibm.com>

diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index 4704226..d1efca4 100644 (file)
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -27,8 +27,10 @@
 
 VLOG_DEFINE_THIS_MODULE(binding);
 
-static struct sset all_lports = SSET_INITIALIZER(&all_lports);
+/* A set of the iface-id values of local interfaces on this chassis. */
+static struct sset local_ids = SSET_INITIALIZER(&local_ids);
 
+/* When this gets set to true, the next run will re-check all binding records. */
 static bool process_full_binding = false;
 
 void
@@ -60,14 +62,14 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl)
 }
 
 static bool
-get_local_iface_ids(const struct ovsrec_bridge *br_int, struct shash *lports)
+get_local_iface_ids(const struct ovsrec_bridge *br_int,
+                    struct shash *lport_to_iface)
 {
     int i;
     bool changed = false;
 
-    /* A local copy of ports that we can use to compare with the persisted
-     * list. */
-    struct shash local_ports = SHASH_INITIALIZER(&local_ports);
+    struct sset old_local_ids = SSET_INITIALIZER(&old_local_ids);
+    sset_clone(&old_local_ids, &local_ids);
 
     for (i = 0; i < br_int->n_ports; i++) {
         const struct ovsrec_port *port_rec = br_int->ports[i];
@@ -86,25 +88,22 @@ get_local_iface_ids(const struct ovsrec_bridge *br_int, struct shash *lports)
             if (!iface_id) {
                 continue;
             }
-            shash_add(&local_ports, iface_id, iface_rec);
-            if (!shash_find(lports, iface_id)) {
-                shash_add(lports, iface_id, iface_rec);
+            shash_add(lport_to_iface, iface_id, iface_rec);
+            if (!sset_find_and_delete(&old_local_ids, iface_id)) {
+                sset_add(&local_ids, iface_id);
                 changed = true;
             }
-            if (!sset_find(&all_lports, iface_id)) {
-                sset_add(&all_lports, iface_id);
-                binding_reset_processing();
-            }
         }
     }
-    struct shash_node *iter, *next;
-    SHASH_FOR_EACH_SAFE(iter, next, lports) {
-        if (!shash_find_and_delete(&local_ports, iter->name)) {
-            shash_delete(lports, iter);
-            changed = true;
-        }
+
+    /* Any item left in old_local_ids is an ID for an interface
+     * that has been removed. */
+    if (!changed && !sset_is_empty(&old_local_ids)) {
+        changed = true;
     }
-    shash_destroy(&local_ports);
+
+    sset_destroy(&old_local_ids);
+
     return changed;
 }
 
@@ -129,7 +128,6 @@ static void
 remove_local_datapath(struct hmap *local_datapaths, struct local_datapath *ld)
 {
     if (ld->logical_port) {
-        sset_find_and_delete(&all_lports, ld->logical_port);
         free(ld->logical_port);
         ld->logical_port = NULL;
     }
@@ -188,20 +186,18 @@ update_qos(const struct ovsrec_interface *iface_rec,
 }
 
 static void
-consider_local_datapath(struct controller_ctx *ctx, struct shash *lports,
+consider_local_datapath(struct controller_ctx *ctx,
                         const struct sbrec_chassis *chassis_rec,
                         const struct sbrec_port_binding *binding_rec,
-                        struct hmap *local_datapaths)
+                        struct hmap *local_datapaths,
+                        struct shash *lport_to_iface)
 {
     const struct ovsrec_interface *iface_rec
-        = shash_find_data(lports, binding_rec->logical_port);
+        = shash_find_data(lport_to_iface, binding_rec->logical_port);
+
     if (iface_rec
         || (binding_rec->parent_port && binding_rec->parent_port[0] &&
-            sset_contains(&all_lports, binding_rec->parent_port))) {
-        if (binding_rec->parent_port && binding_rec->parent_port[0]) {
-            /* Add child logical port to the set of all local ports. */
-            sset_add(&all_lports, binding_rec->logical_port);
-        }
+            sset_contains(&local_ids, binding_rec->parent_port))) {
         add_local_datapath(local_datapaths, binding_rec,
                            &binding_rec->header_.uuid);
         if (iface_rec && ctx->ovs_idl_txn) {
@@ -242,7 +238,6 @@ consider_local_datapath(struct controller_ctx *ctx, struct shash *lports,
             VLOG_INFO("Claiming l2gateway port %s for this chassis.",
                       binding_rec->logical_port);
             sbrec_port_binding_set_chassis(binding_rec, chassis_rec);
-            sset_add(&all_lports, binding_rec->logical_port);
             add_local_datapath(local_datapaths, binding_rec,
                                &binding_rec->header_.uuid);
         }
@@ -253,26 +248,16 @@ consider_local_datapath(struct controller_ctx *ctx, struct shash *lports,
                       binding_rec->logical_port);
             sbrec_port_binding_set_chassis(binding_rec, NULL);
         }
-    } else if (!binding_rec->chassis
-               && !strcmp(binding_rec->type, "localnet")) {
-        /* Localnet ports will never be bound to a chassis, but we want
-         * to list them in all_lports because we want to allocate
-         * a conntrack zone ID for each one, as we'll be creating
-         * a patch port for each one. */
-        sset_add(&all_lports, binding_rec->logical_port);
     }
 }
 
-/* We persist lports because we need to know when it changes to
- * handle ports going away correctly in the binding record. */
-static struct shash lports = SHASH_INITIALIZER(&lports);
-
 void
 binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
             const char *chassis_id, struct hmap *local_datapaths)
 {
     const struct sbrec_chassis *chassis_rec;
     const struct sbrec_port_binding *binding_rec;
+    struct shash lport_to_iface = SHASH_INITIALIZER(&lport_to_iface);
 
     chassis_rec = get_chassis(ctx->ovnsb_idl, chassis_id);
     if (!chassis_rec) {
@@ -280,7 +265,7 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
     }
 
     if (br_int) {
-        if (ctx->ovnsb_idl_txn && get_local_iface_ids(br_int, &lports)) {
+        if (ctx->ovnsb_idl_txn && get_local_iface_ids(br_int, &lport_to_iface)) {
             process_full_binding = true;
         }
     } else {
@@ -296,8 +281,8 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
         struct hmap keep_local_datapath_by_uuid =
             HMAP_INITIALIZER(&keep_local_datapath_by_uuid);
         SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) {
-            consider_local_datapath(ctx, &lports, chassis_rec, binding_rec,
-                                    local_datapaths);
+            consider_local_datapath(ctx, chassis_rec, binding_rec,
+                                    local_datapaths, &lport_to_iface);
             struct local_datapath *ld = xzalloc(sizeof *ld);
             memcpy(&ld->uuid, &binding_rec->header_.uuid, sizeof ld->uuid);
             hmap_insert(&keep_local_datapath_by_uuid, &ld->uuid_hmap_node,
@@ -317,11 +302,13 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
             if (sbrec_port_binding_is_deleted(binding_rec)) {
                 remove_local_datapath_by_binding(local_datapaths, binding_rec);
             } else {
-                consider_local_datapath(ctx, &lports, chassis_rec, binding_rec,
-                                        local_datapaths);
+                consider_local_datapath(ctx, chassis_rec, binding_rec,
+                                        local_datapaths, &lport_to_iface);
             }
         }
     }
+
+    shash_destroy(&lport_to_iface);
 }
 
 /* Returns true if the database is all cleaned up, false if more work is