[ovs-dev] [PATCH 3/4] Revert "Convert binding_run to incremental processing."

Russell Bryant russell at ovn.org
Fri Jun 24 19:52:10 UTC 2016


This reverts commit 263064aeaa31e758538773fac571dff0cb246cde.

Signed-off-by: Russell Bryant <russell at ovn.org>
---
 ovn/controller/binding.c        | 205 ++++++++++------------------------------
 ovn/controller/binding.h        |   4 +-
 ovn/controller/encaps.c         |   3 -
 ovn/controller/ovn-controller.c |  29 ++++--
 ovn/controller/ovn-controller.h |   3 -
 ovn/controller/patch.c          |  10 +-
 6 files changed, 76 insertions(+), 178 deletions(-)

diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index e36c8f4..a07c327 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -27,16 +27,6 @@
 
 VLOG_DEFINE_THIS_MODULE(binding);
 
-static struct sset all_lports = SSET_INITIALIZER(&all_lports);
-
-static bool process_full_binding = false;
-
-void
-binding_reset_processing(void)
-{
-    process_full_binding = true;
-}
-
 void
 binding_register_ovs_idl(struct ovsdb_idl *ovs_idl)
 {
@@ -82,67 +72,13 @@ get_local_iface_ids(const struct ovsrec_bridge *br_int, struct shash *lports)
                 continue;
             }
             shash_add(lports, iface_id, iface_rec);
-            if (!sset_find(&all_lports, iface_id)) {
-                sset_add(&all_lports, iface_id);
-                binding_reset_processing();
-            }
-        }
-    }
-}
-
-/* Contains "struct local_datpath" nodes whose hash values are the
- * row uuids of datapaths with at least one local port binding. */
-static struct hmap local_datapaths_by_uuid =
-    HMAP_INITIALIZER(&local_datapaths_by_uuid);
-
-static struct local_datapath *
-local_datapath_lookup_by_uuid(struct hmap *hmap_p, const struct uuid *uuid)
-{
-    struct local_datapath *ld;
-    HMAP_FOR_EACH_WITH_HASH(ld, uuid_hmap_node, uuid_hash(uuid), hmap_p) {
-        if (uuid_equals(ld->uuid, uuid)) {
-            return ld;
-        }
-    }
-    return NULL;
-}
-
-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;
-    }
-    hmap_remove(local_datapaths, &ld->hmap_node);
-    hmap_remove(&local_datapaths_by_uuid, &ld->uuid_hmap_node);
-    free(ld);
-}
-
-static void
-remove_local_datapath_by_binding(struct hmap *local_datapaths,
-                                 const struct sbrec_port_binding *binding_rec)
-{
-    const struct uuid *uuid = &binding_rec->header_.uuid;
-    struct local_datapath *ld = local_datapath_lookup_by_uuid(local_datapaths,
-                                                              uuid);
-    if (ld) {
-        remove_local_datapath(local_datapaths, ld);
-    } else {
-        struct local_datapath *ld;
-        HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
-            if (ld->localnet_port == binding_rec) {
-                ld->localnet_port = NULL;
-            }
         }
     }
 }
 
 static void
 add_local_datapath(struct hmap *local_datapaths,
-        const struct sbrec_port_binding *binding_rec,
-        const struct uuid *uuid)
+        const struct sbrec_port_binding *binding_rec)
 {
     if (get_local_datapath(local_datapaths,
                            binding_rec->datapath->tunnel_key)) {
@@ -150,12 +86,8 @@ add_local_datapath(struct hmap *local_datapaths,
     }
 
     struct local_datapath *ld = xzalloc(sizeof *ld);
-    ld->logical_port = xstrdup(binding_rec->logical_port);
-    ld->uuid = &binding_rec->header_.uuid;
     hmap_insert(local_datapaths, &ld->hmap_node,
                 binding_rec->datapath->tunnel_key);
-    hmap_insert(&local_datapaths_by_uuid, &ld->uuid_hmap_node,
-                uuid_hash(uuid));
 }
 
 static void
@@ -169,69 +101,15 @@ update_qos(const struct ovsrec_interface *iface_rec,
     ovsrec_interface_set_ingress_policing_burst(iface_rec, MAX(0, burst));
 }
 
-static void
-consider_local_datapath(struct controller_ctx *ctx, struct shash *lports,
-                        const struct sbrec_chassis *chassis_rec,
-                        const struct sbrec_port_binding *binding_rec,
-                        struct hmap *local_datapaths)
-{
-    const struct ovsrec_interface *iface_rec
-        = shash_find_and_delete(lports, 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);
-        }
-        add_local_datapath(local_datapaths, binding_rec,
-                           &binding_rec->header_.uuid);
-        if (iface_rec && ctx->ovs_idl_txn) {
-            update_qos(iface_rec, binding_rec);
-        }
-        if (binding_rec->chassis == chassis_rec) {
-            return;
-        }
-        if (ctx->ovnsb_idl_txn) {
-            if (binding_rec->chassis) {
-                VLOG_INFO("Changing chassis for lport %s from %s to %s.",
-                          binding_rec->logical_port,
-                          binding_rec->chassis->name,
-                          chassis_rec->name);
-            } else {
-                VLOG_INFO("Claiming lport %s for this chassis.",
-                          binding_rec->logical_port);
-            }
-            sbrec_port_binding_set_chassis(binding_rec, chassis_rec);
-        }
-    } else if (chassis_rec && binding_rec->chassis == chassis_rec
-               && strcmp(binding_rec->type, "gateway")) {
-        if (ctx->ovnsb_idl_txn) {
-            VLOG_INFO("Releasing lport %s from this chassis.",
-                      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);
-    }
-}
-
 void
 binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
-            const char *chassis_id, struct hmap *local_datapaths)
+            const char *chassis_id, struct sset *all_lports,
+            struct hmap *local_datapaths)
 {
     const struct sbrec_chassis *chassis_rec;
     const struct sbrec_port_binding *binding_rec;
 
     chassis_rec = get_chassis(ctx->ovnsb_idl, chassis_id);
-    if (!chassis_rec) {
-        return;
-    }
 
     struct shash lports = SHASH_INITIALIZER(&lports);
     if (br_int) {
@@ -241,43 +119,62 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
          * We'll remove our chassis from all port binding records below. */
     }
 
+    struct shash_node *node;
+    SHASH_FOR_EACH (node, &lports) {
+        sset_add(all_lports, node->name);
+    }
+
     /* 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. */
-    if (process_full_binding) {
-        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);
-            struct local_datapath *ld = xzalloc(sizeof *ld);
-            ld->uuid = &binding_rec->header_.uuid;
-            hmap_insert(&keep_local_datapath_by_uuid, &ld->uuid_hmap_node,
-                        uuid_hash(ld->uuid));
-        }
-        struct local_datapath *old_ld, *next;
-        HMAP_FOR_EACH_SAFE (old_ld, next, hmap_node, local_datapaths) {
-            if (!local_datapath_lookup_by_uuid(&keep_local_datapath_by_uuid,
-                                               old_ld->uuid)) {
-                remove_local_datapath(local_datapaths, old_ld);
+    SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) {
+        const struct ovsrec_interface *iface_rec
+            = shash_find_and_delete(&lports, 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);
             }
-        }
-        hmap_destroy(&keep_local_datapath_by_uuid);
-        process_full_binding = false;
-    } else {
-        SBREC_PORT_BINDING_FOR_EACH_TRACKED(binding_rec, ctx->ovnsb_idl) {
-            bool is_deleted = sbrec_port_binding_row_get_seqno(binding_rec,
-                OVSDB_IDL_CHANGE_DELETE) > 0;
-
-            if (is_deleted) {
-                remove_local_datapath_by_binding(local_datapaths, binding_rec);
-                continue;
+            add_local_datapath(local_datapaths, binding_rec);
+            if (iface_rec && ctx->ovs_idl_txn) {
+                update_qos(iface_rec, binding_rec);
             }
-            consider_local_datapath(ctx, &lports, chassis_rec, binding_rec,
-                                    local_datapaths);
+            if (ctx->ovnsb_idl_txn && chassis_rec
+                && binding_rec->chassis != chassis_rec) {
+                if (binding_rec->chassis) {
+                    VLOG_INFO("Changing chassis for lport %s from %s to %s.",
+                              binding_rec->logical_port,
+                              binding_rec->chassis->name,
+                              chassis_rec->name);
+                } else {
+                    VLOG_INFO("Claiming lport %s for this chassis.",
+                              binding_rec->logical_port);
+                }
+                sbrec_port_binding_set_chassis(binding_rec, chassis_rec);
+            }
+        } else if (chassis_rec && binding_rec->chassis == chassis_rec
+                   && strcmp(binding_rec->type, "gateway")) {
+            if (ctx->ovnsb_idl_txn) {
+                VLOG_INFO("Releasing lport %s from this chassis.",
+                          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);
         }
     }
 
+    SHASH_FOR_EACH (node, &lports) {
+        VLOG_DBG("No port binding record for lport %s", node->name);
+    }
+
     shash_destroy(&lports);
 }
 
diff --git a/ovn/controller/binding.h b/ovn/controller/binding.h
index 8753d44..25f8989 100644
--- a/ovn/controller/binding.h
+++ b/ovn/controller/binding.h
@@ -27,9 +27,9 @@ struct simap;
 struct sset;
 
 void binding_register_ovs_idl(struct ovsdb_idl *);
-void binding_reset_processing(void);
 void binding_run(struct controller_ctx *, const struct ovsrec_bridge *br_int,
-                 const char *chassis_id, struct hmap *local_datapaths);
+                 const char *chassis_id, struct sset *all_lports,
+                 struct hmap *local_datapaths);
 bool binding_cleanup(struct controller_ctx *, const char *chassis_id);
 
 #endif /* ovn/binding.h */
diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
index 495f8f6..bd5650a 100644
--- a/ovn/controller/encaps.c
+++ b/ovn/controller/encaps.c
@@ -15,7 +15,6 @@
 
 #include <config.h>
 #include "encaps.h"
-#include "binding.h"
 #include "lflow.h"
 
 #include "lib/hash.h"
@@ -235,7 +234,6 @@ tunnel_add(const struct sbrec_chassis *chassis_rec,
     sset_add(&tc.port_names, port_name);
     free(port_name);
     free(ports);
-    binding_reset_processing();
     process_full_encaps = true;
 }
 
@@ -422,7 +420,6 @@ encaps_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
                     hmap_remove(&tc.tunnel_hmap_by_uuid,
                                 &port_hash->uuid_node);
                     free(port_hash);
-                    binding_reset_processing();
                 }
                 continue;
             }
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 8620a71..5506d3b 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -322,11 +322,6 @@ update_ct_zones(struct sset *lports, struct hmap *patched_datapaths,
     sset_destroy(&all_users);
 }
 
-/* Contains "struct local_datapath" nodes whose hash values are the
- * tunnel_key of datapaths with at least one local port binding. */
-static struct hmap local_datapaths = HMAP_INITIALIZER(&local_datapaths);
-static struct hmap patched_datapaths = HMAP_INITIALIZER(&patched_datapaths);
-
 int
 main(int argc, char *argv[])
 {
@@ -416,7 +411,6 @@ main(int argc, char *argv[])
             free(ovnsb_remote);
             ovnsb_remote = new_ovnsb_remote;
             ovsdb_idl_set_remote(ovnsb_idl_loop.idl, ovnsb_remote, true);
-            binding_reset_processing();
         } else {
             free(new_ovnsb_remote);
         }
@@ -428,6 +422,11 @@ main(int argc, char *argv[])
             .ovnsb_idl_txn = ovsdb_idl_loop_run(&ovnsb_idl_loop),
         };
 
+        /* Contains "struct local_datpath" nodes whose hash values are the
+         * tunnel_key of datapaths with at least one local port binding. */
+        struct hmap local_datapaths = HMAP_INITIALIZER(&local_datapaths);
+
+        struct hmap patched_datapaths = HMAP_INITIALIZER(&patched_datapaths);
         struct sset all_lports = SSET_INITIALIZER(&all_lports);
 
         const struct ovsrec_bridge *br_int = get_br_int(&ctx);
@@ -436,7 +435,8 @@ main(int argc, char *argv[])
         if (chassis_id) {
             chassis_run(&ctx, chassis_id);
             encaps_run(&ctx, br_int, chassis_id);
-            binding_run(&ctx, br_int, chassis_id, &local_datapaths);
+            binding_run(&ctx, br_int, chassis_id, &all_lports,
+                        &local_datapaths);
         }
 
         if (br_int && chassis_id) {
@@ -470,6 +470,21 @@ main(int argc, char *argv[])
 
         sset_destroy(&all_lports);
 
+        struct local_datapath *cur_node, *next_node;
+        HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node, &local_datapaths) {
+            hmap_remove(&local_datapaths, &cur_node->hmap_node);
+            free(cur_node);
+        }
+        hmap_destroy(&local_datapaths);
+
+        struct patched_datapath *pd_cur_node, *pd_next_node;
+        HMAP_FOR_EACH_SAFE (pd_cur_node, pd_next_node, hmap_node,
+                &patched_datapaths) {
+            hmap_remove(&patched_datapaths, &pd_cur_node->hmap_node);
+            free(pd_cur_node);
+        }
+        hmap_destroy(&patched_datapaths);
+
         unixctl_server_run(unixctl);
 
         unixctl_server_wait(unixctl);
diff --git a/ovn/controller/ovn-controller.h b/ovn/controller/ovn-controller.h
index f0c4e63..ba50a98 100644
--- a/ovn/controller/ovn-controller.h
+++ b/ovn/controller/ovn-controller.h
@@ -38,9 +38,6 @@ struct controller_ctx {
  * the localnet port */
 struct local_datapath {
     struct hmap_node hmap_node;
-    struct hmap_node uuid_hmap_node;
-    const struct uuid *uuid;
-    char *logical_port;
     const struct sbrec_port_binding *localnet_port;
 };
 
diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
index fa4e624..652466b 100644
--- a/ovn/controller/patch.c
+++ b/ovn/controller/patch.c
@@ -185,15 +185,7 @@ add_bridge_mappings(struct controller_ctx *ctx,
                  * to create patch ports for it. */
                 continue;
             }
-
-            /* Under incremental processing, it is possible to re-enter the
-             * following block with a logical port that has already been
-             * recorded in binding->logical_port.  Rather than emit spurious
-             * warnings, add a check to see if the logical port name has
-             * actually changed. */
-
-            if (ld->localnet_port && strcmp(ld->localnet_port->logical_port,
-                                            binding->logical_port)) {
+            if (ld->localnet_port) {
                 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
                 VLOG_WARN_RL(&rl, "localnet port '%s' already set for datapath "
                              "'%"PRId64"', skipping the new port '%s'.",
-- 
2.7.4




More information about the dev mailing list