ovn-controller: decouple localnet_port update from patch_run

We figure out local datapaths in binding_run() but update the field
localnet_port for each local datapath that has localnet port in
patch_run(). This patch updates the localnet_port field in binding_run
directly and removes the logic in patch_run(), since the logic is
more about port-binding processing, and patch_run() is focusing on
patch port creation only.

In a future patch binding_run() will be used in a new thread for
pinctrl, but patch_run() will not.

Signed-off-by: Han Zhou <zhouhan@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>

4 files changed, 37 insertions, 30 deletions

diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index bb7660875..11145dd4c 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -442,6 +442,30 @@ consider_local_datapath(struct controller_ctx *ctx,
     }
 }
 
+static void
+consider_localnet_port(const struct sbrec_port_binding *binding_rec,
+                       struct hmap *local_datapaths)
+{
+    struct local_datapath *ld
+        = get_local_datapath(local_datapaths,
+                             binding_rec->datapath->tunnel_key);
+    if (!ld) {
+        return;
+    }
+
+    if (ld->localnet_port && strcmp(ld->localnet_port->logical_port,
+                                    binding_rec->logical_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'.",
+                     ld->localnet_port->logical_port,
+                     binding_rec->datapath->tunnel_key,
+                     binding_rec->logical_port);
+        return;
+    }
+    ld->localnet_port = binding_rec;
+}
+
 void
 binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
             const struct sbrec_chassis *chassis_rec,
@@ -476,6 +500,15 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
 
     }
 
+    /* Run through each binding record to see if it is a localnet port
+     * on local datapaths discovered from above loop, and update the
+     * corresponding local datapath accordingly. */
+    SBREC_PORT_BINDING_FOR_EACH (binding_rec, ctx->ovnsb_idl) {
+        if (!strcmp(binding_rec->type, "localnet")) {
+            consider_localnet_port(binding_rec, local_datapaths);
+        }
+    }
+
     if (!sset_is_empty(&egress_ifaces)
         && set_noop_qos(ctx, &egress_ifaces)) {
         const char *entry;
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 00b4cdabf..45a670b5d 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -643,7 +643,7 @@ main(int argc, char *argv[])
             struct shash addr_sets = SHASH_INITIALIZER(&addr_sets);
             addr_sets_init(&ctx, &addr_sets);
 
-            patch_run(&ctx, br_int, chassis, &local_datapaths);
+            patch_run(&ctx, br_int, chassis);
 
             enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int,
                                                          &pending_ct_zones);
diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
index 27c6ac385..47cf7947f 100644
--- a/ovn/controller/patch.c
+++ b/ovn/controller/patch.c
@@ -136,7 +136,6 @@ static void
 add_bridge_mappings(struct controller_ctx *ctx,
                     const struct ovsrec_bridge *br_int,
                     struct shash *existing_ports,
-                    struct hmap *local_datapaths,
                     const struct sbrec_chassis *chassis)
 {
     /* Get ovn-bridge-mappings. */
@@ -180,30 +179,6 @@ add_bridge_mappings(struct controller_ctx *ctx,
     SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) {
         const char *patch_port_id;
         if (!strcmp(binding->type, "localnet")) {
-            struct local_datapath *ld
-                = get_local_datapath(local_datapaths,
-                                     binding->datapath->tunnel_key);
-            if (!ld) {
-                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)) {
-                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'.",
-                             ld->localnet_port->logical_port,
-                             binding->datapath->tunnel_key,
-                             binding->logical_port);
-                continue;
-            }
-            ld->localnet_port = binding;
             patch_port_id = "ovn-localnet-port";
         } else if (!strcmp(binding->type, "l2gateway")) {
             if (!binding->chassis
@@ -249,7 +224,7 @@ add_bridge_mappings(struct controller_ctx *ctx,
 
 void
 patch_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
-          const struct sbrec_chassis *chassis, struct hmap *local_datapaths)
+          const struct sbrec_chassis *chassis)
 {
     if (!ctx->ovs_idl_txn) {
         return;
@@ -275,8 +250,7 @@ patch_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
     /* Create in the database any patch ports that should exist.  Remove from
      * 'existing_ports' any patch ports that do exist in the database and
      * should be there. */
-    add_bridge_mappings(ctx, br_int, &existing_ports, local_datapaths,
-                        chassis);
+    add_bridge_mappings(ctx, br_int, &existing_ports, chassis);
 
     /* Now 'existing_ports' only still contains patch ports that exist in the
      * database but shouldn't.  Delete them from the database. */
diff --git a/ovn/controller/patch.h b/ovn/controller/patch.h
index 2feb44bec..bcc89b60b 100644
--- a/ovn/controller/patch.h
+++ b/ovn/controller/patch.h
@@ -28,6 +28,6 @@ struct ovsrec_bridge;
 struct sbrec_chassis;
 
 void patch_run(struct controller_ctx *, const struct ovsrec_bridge *br_int,
-               const struct sbrec_chassis *, struct hmap *local_datapaths);
+               const struct sbrec_chassis *);
 
 #endif /* ovn/patch.h */