[ovs-dev] [PATCH 3/3] ovn-controller: Honor updates to SSL configuration while waiting for SB DB.

Ben Pfaff blp at ovn.org
Wed Apr 25 15:43:00 UTC 2018


At startup time, ovn-controller connects to the OVS database and retrieves
a pointer to the southbound database, then connects to the southbound
database and retrieves a snapshot.  Until now, however, it didn't pay
attention to changes in the OVS database while trying to retrieve the
southbound database, which meant that if the SSL

Also honor changes to the remote for the southbound database while waiting
to connect to it.

Most of the changes in this commit are whitespace only indentation changes,
so passing -w to "git show" (etc.) make it easier to understand.

Reported-by: Dan Williams <dcbw at redhat.com>
Reported-at: https://github.com/openvswitch/ovs-issues/issues/144
Signed-off-by: Ben Pfaff <blp at ovn.org>
---
 ovn/controller/ovn-controller.c | 325 ++++++++++++++++++----------------------
 1 file changed, 146 insertions(+), 179 deletions(-)

diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 29b3f1cade0a..b79ed185d373 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -72,8 +72,6 @@ static unixctl_cb_func inject_pkt;
 
 #define CONTROLLER_LOOP_STOPWATCH_NAME "ovn-controller-flow-generation"
 
-static void update_probe_interval(struct controller_ctx *,
-                                  const char *ovnsb_remote);
 static char *parse_options(int argc, char *argv[]);
 OVS_NO_RETURN static void usage(void);
 
@@ -321,28 +319,26 @@ update_ssl_config(const struct ovsdb_idl *ovs_idl)
     }
 }
 
-/* Retrieves the OVN Southbound remote location from the
- * "external-ids:ovn-remote" key in 'ovs_idl' and returns a copy of it. */
-static char *
-get_ovnsb_remote(struct ovsdb_idl *ovs_idl)
+/* Retrieves the pointer to the OVN Southbound database from 'ovs_idl' and
+ * updates 'sbdb_idl' with that pointer. */
+static void
+update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl)
 {
-    while (1) {
-        ovsdb_idl_run(ovs_idl);
-
-        const struct ovsrec_open_vswitch *cfg
-            = ovsrec_open_vswitch_first(ovs_idl);
-        if (cfg) {
-            const char *remote = smap_get(&cfg->external_ids, "ovn-remote");
-            if (remote) {
-                update_ssl_config(ovs_idl);
-                return xstrdup(remote);
-            }
-        }
+    const struct ovsrec_open_vswitch *cfg = ovsrec_open_vswitch_first(ovs_idl);
 
-        VLOG_INFO("OVN OVSDB remote not specified.  Waiting...");
-        ovsdb_idl_wait(ovs_idl);
-        poll_block();
+    /* Set remote based on user configuration. */
+    const char *remote = NULL;
+    if (cfg) {
+        remote = smap_get(&cfg->external_ids, "ovn-remote");
     }
+    ovsdb_idl_set_remote(ovnsb_idl, remote, true);
+
+    /* Set probe interval, based on user configuration and the remote. */
+    int default_interval = (remote && !stream_or_pstream_needs_probes(remote)
+                            ? 0 : DEFAULT_PROBE_INTERVAL_MSEC);
+    int interval = smap_get_int(&cfg->external_ids,
+                                "ovn-remote-probe-interval", default_interval);
+    ovsdb_idl_set_probe_interval(ovnsb_idl, interval);
 }
 
 static void
@@ -629,10 +625,9 @@ main(int argc, char *argv[])
     ctrl_register_ovs_idl(ovs_idl_loop.idl);
     ovsdb_idl_get_initial_snapshot(ovs_idl_loop.idl);
 
-    /* Connect to OVN SB database and get a snapshot. */
-    char *ovnsb_remote = get_ovnsb_remote(ovs_idl_loop.idl);
+    /* Configure OVN SB database. */
     struct ovsdb_idl_loop ovnsb_idl_loop = OVSDB_IDL_LOOP_INITIALIZER(
-        ovsdb_idl_create(ovnsb_remote, &sbrec_idl_class, true, true));
+        ovsdb_idl_create_unconnected(&sbrec_idl_class, true));
     ovsdb_idl_set_leader_only(ovnsb_idl_loop.idl, false);
 
     create_ovnsb_indexes(ovnsb_idl_loop.idl);
@@ -640,7 +635,6 @@ main(int argc, char *argv[])
 
     ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, &sbrec_chassis_col_nb_cfg);
     update_sb_monitors(ovnsb_idl_loop.idl, NULL, NULL, NULL);
-    ovsdb_idl_get_initial_snapshot(ovnsb_idl_loop.idl);
 
     /* Initialize connection tracking zones. */
     struct simap ct_zones = SIMAP_INITIALIZER(&ct_zones);
@@ -660,15 +654,8 @@ main(int argc, char *argv[])
     /* Main loop. */
     exiting = false;
     while (!exiting) {
-        /* Check OVN SB database. */
-        char *new_ovnsb_remote = get_ovnsb_remote(ovs_idl_loop.idl);
-        if (strcmp(ovnsb_remote, new_ovnsb_remote)) {
-            free(ovnsb_remote);
-            ovnsb_remote = new_ovnsb_remote;
-            ovsdb_idl_set_remote(ovnsb_idl_loop.idl, ovnsb_remote, true);
-        } else {
-            free(new_ovnsb_remote);
-        }
+        update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl);
+        update_ssl_config(ovs_idl_loop.idl);
 
         struct controller_ctx ctx = {
             .ovs_idl = ovs_idl_loop.idl,
@@ -677,137 +664,142 @@ main(int argc, char *argv[])
             .ovnsb_idl_txn = ovsdb_idl_loop_run(&ovnsb_idl_loop),
         };
 
-        update_probe_interval(&ctx, ovnsb_remote);
-
-        update_ssl_config(ctx.ovs_idl);
-
-        /* Contains "struct local_datapath" nodes. */
-        struct hmap local_datapaths = HMAP_INITIALIZER(&local_datapaths);
-
-        /* Contains the name of each logical port resident on the local
-         * hypervisor.  These logical ports include the VIFs (and their child
-         * logical ports, if any) that belong to VMs running on the hypervisor,
-         * l2gateway ports for which options:l2gateway-chassis designates the
-         * local hypervisor, and localnet ports. */
-        struct sset local_lports = SSET_INITIALIZER(&local_lports);
-        /* Contains the same ports as local_lports, but in the format:
-         * <datapath-tunnel-key>_<port-tunnel-key> */
-        struct sset local_lport_ids = SSET_INITIALIZER(&local_lport_ids);
-        struct sset active_tunnels = SSET_INITIALIZER(&active_tunnels);
-
-        const struct ovsrec_bridge *br_int = get_br_int(&ctx);
-        const char *chassis_id = get_chassis_id(ctx.ovs_idl);
-
-        struct chassis_index chassis_index;
-
-        chassis_index_init(&chassis_index, ctx.ovnsb_idl);
-
-        const struct sbrec_chassis *chassis = NULL;
-        if (chassis_id) {
-            chassis = chassis_run(&ctx, chassis_id, br_int);
-            encaps_run(&ctx, br_int, chassis_id);
-            bfd_calculate_active_tunnels(br_int, &active_tunnels);
-            binding_run(&ctx, br_int, chassis,
-                        &chassis_index, &active_tunnels, &local_datapaths,
-                        &local_lports, &local_lport_ids);
-        }
-        if (br_int && chassis) {
-            struct shash addr_sets = SHASH_INITIALIZER(&addr_sets);
-            addr_sets_init(&ctx, &addr_sets);
-            struct shash port_groups = SHASH_INITIALIZER(&port_groups);
-            port_groups_init(&ctx, &port_groups);
-
-            patch_run(&ctx, br_int, chassis);
-
-            enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int,
-                                                         &pending_ct_zones);
-
-            pinctrl_run(&ctx, br_int, chassis, &chassis_index,
-                        &local_datapaths, &active_tunnels);
-            update_ct_zones(&local_lports, &local_datapaths, &ct_zones,
-                            ct_zone_bitmap, &pending_ct_zones);
-            if (ctx.ovs_idl_txn) {
-                if (ofctrl_can_put()) {
-                    stopwatch_start(CONTROLLER_LOOP_STOPWATCH_NAME,
-                                    time_msec());
-
-                    commit_ct_zones(br_int, &pending_ct_zones);
-
-                    struct hmap flow_table = HMAP_INITIALIZER(&flow_table);
-                    lflow_run(&ctx, chassis,
-                              &chassis_index, &local_datapaths, &group_table,
-                              &meter_table, &addr_sets, &port_groups,
-                              &flow_table, &active_tunnels, &local_lport_ids);
-
-                    if (chassis_id) {
-                        bfd_run(&ctx, br_int, chassis, &local_datapaths,
-                                &chassis_index);
+        if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl)) {
+            /* Contains "struct local_datapath" nodes. */
+            struct hmap local_datapaths = HMAP_INITIALIZER(&local_datapaths);
+
+            /* Contains the name of each logical port resident on the local
+             * hypervisor.  These logical ports include the VIFs (and their
+             * child logical ports, if any) that belong to VMs running on the
+             * hypervisor, l2gateway ports for which options:l2gateway-chassis
+             * designates the local hypervisor, and localnet ports. */
+            struct sset local_lports = SSET_INITIALIZER(&local_lports);
+            /* Contains the same ports as local_lports, but in the format:
+             * <datapath-tunnel-key>_<port-tunnel-key> */
+            struct sset local_lport_ids = SSET_INITIALIZER(&local_lport_ids);
+            struct sset active_tunnels = SSET_INITIALIZER(&active_tunnels);
+
+            const struct ovsrec_bridge *br_int = get_br_int(&ctx);
+            const char *chassis_id = get_chassis_id(ctx.ovs_idl);
+
+            struct chassis_index chassis_index;
+
+            chassis_index_init(&chassis_index, ctx.ovnsb_idl);
+
+            const struct sbrec_chassis *chassis = NULL;
+            if (chassis_id) {
+                chassis = chassis_run(&ctx, chassis_id, br_int);
+                encaps_run(&ctx, br_int, chassis_id);
+                bfd_calculate_active_tunnels(br_int, &active_tunnels);
+                binding_run(&ctx, br_int, chassis,
+                            &chassis_index, &active_tunnels, &local_datapaths,
+                            &local_lports, &local_lport_ids);
+            }
+            if (br_int && chassis) {
+                struct shash addr_sets = SHASH_INITIALIZER(&addr_sets);
+                addr_sets_init(&ctx, &addr_sets);
+                struct shash port_groups = SHASH_INITIALIZER(&port_groups);
+                port_groups_init(&ctx, &port_groups);
+
+                patch_run(&ctx, br_int, chassis);
+
+                enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int,
+                                                             &pending_ct_zones);
+
+                pinctrl_run(&ctx, br_int, chassis, &chassis_index,
+                            &local_datapaths, &active_tunnels);
+                update_ct_zones(&local_lports, &local_datapaths, &ct_zones,
+                                ct_zone_bitmap, &pending_ct_zones);
+                if (ctx.ovs_idl_txn) {
+                    if (ofctrl_can_put()) {
+                        stopwatch_start(CONTROLLER_LOOP_STOPWATCH_NAME,
+                                        time_msec());
+
+                        commit_ct_zones(br_int, &pending_ct_zones);
+
+                        struct hmap flow_table = HMAP_INITIALIZER(&flow_table);
+                        lflow_run(&ctx, chassis,
+                                  &chassis_index, &local_datapaths,
+                                  &group_table, &meter_table, &addr_sets,
+                                  &port_groups, &flow_table, &active_tunnels,
+                                  &local_lport_ids);
+
+                        if (chassis_id) {
+                            bfd_run(&ctx, br_int, chassis, &local_datapaths,
+                                    &chassis_index);
+                        }
+                        physical_run(&ctx, mff_ovn_geneve, br_int, chassis,
+                                     &ct_zones, &flow_table, &local_datapaths,
+                                     &local_lports, &chassis_index,
+                                     &active_tunnels);
+
+                        stopwatch_stop(CONTROLLER_LOOP_STOPWATCH_NAME,
+                                       time_msec());
+
+                        ofctrl_put(&flow_table, &pending_ct_zones,
+                                   get_nb_cfg(ctx.ovnsb_idl));
+
+                        hmap_destroy(&flow_table);
+                    }
+                    if (ctx.ovnsb_idl_txn) {
+                        int64_t cur_cfg = ofctrl_get_cur_cfg();
+                        if (cur_cfg && cur_cfg != chassis->nb_cfg) {
+                            sbrec_chassis_set_nb_cfg(chassis, cur_cfg);
+                        }
                     }
-                    physical_run(&ctx, mff_ovn_geneve,
-                                 br_int, chassis, &ct_zones,
-                                 &flow_table, &local_datapaths, &local_lports,
-                                 &chassis_index, &active_tunnels);
-
-                    stopwatch_stop(CONTROLLER_LOOP_STOPWATCH_NAME,
-                                   time_msec());
-
-                    ofctrl_put(&flow_table, &pending_ct_zones,
-                               get_nb_cfg(ctx.ovnsb_idl));
-
-                    hmap_destroy(&flow_table);
                 }
-                if (ctx.ovnsb_idl_txn) {
-                    int64_t cur_cfg = ofctrl_get_cur_cfg();
-                    if (cur_cfg && cur_cfg != chassis->nb_cfg) {
-                        sbrec_chassis_set_nb_cfg(chassis, cur_cfg);
+
+                if (pending_pkt.conn) {
+                    char *error = ofctrl_inject_pkt(br_int, pending_pkt.flow_s,
+                                                    &port_groups, &addr_sets);
+                    if (error) {
+                        unixctl_command_reply_error(pending_pkt.conn, error);
+                        free(error);
+                    } else {
+                        unixctl_command_reply(pending_pkt.conn, NULL);
                     }
+                    pending_pkt.conn = NULL;
+                    free(pending_pkt.flow_s);
                 }
+
+                update_sb_monitors(ctx.ovnsb_idl, chassis,
+                                   &local_lports, &local_datapaths);
+
+                expr_const_sets_destroy(&addr_sets);
+                shash_destroy(&addr_sets);
+                expr_const_sets_destroy(&port_groups);
+                shash_destroy(&port_groups);
             }
 
+            /* If we haven't handled the pending packet insertion
+             * request, the system is not ready. */
             if (pending_pkt.conn) {
-                char *error = ofctrl_inject_pkt(br_int, pending_pkt.flow_s,
-                                                &port_groups, &addr_sets);
-                if (error) {
-                    unixctl_command_reply_error(pending_pkt.conn, error);
-                    free(error);
-                } else {
-                    unixctl_command_reply(pending_pkt.conn, NULL);
-                }
+                unixctl_command_reply_error(pending_pkt.conn,
+                                            "ovn-controller not ready.");
                 pending_pkt.conn = NULL;
                 free(pending_pkt.flow_s);
             }
 
-            update_sb_monitors(ctx.ovnsb_idl, chassis,
-                               &local_lports, &local_datapaths);
+            chassis_index_destroy(&chassis_index);
 
-            expr_const_sets_destroy(&addr_sets);
-            shash_destroy(&addr_sets);
-            expr_const_sets_destroy(&port_groups);
-            shash_destroy(&port_groups);
-        }
-
-        /* If we haven't handled the pending packet insertion
-         * request, the system is not ready. */
-        if (pending_pkt.conn) {
-            unixctl_command_reply_error(pending_pkt.conn,
-                                        "ovn-controller not ready.");
-            pending_pkt.conn = NULL;
-            free(pending_pkt.flow_s);
-        }
+            sset_destroy(&local_lports);
+            sset_destroy(&local_lport_ids);
+            sset_destroy(&active_tunnels);
 
-        chassis_index_destroy(&chassis_index);
-
-        sset_destroy(&local_lports);
-        sset_destroy(&local_lport_ids);
-        sset_destroy(&active_tunnels);
+            struct local_datapath *cur_node, *next_node;
+            HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node,
+                                &local_datapaths) {
+                free(cur_node->peer_dps);
+                hmap_remove(&local_datapaths, &cur_node->hmap_node);
+                free(cur_node);
+            }
+            hmap_destroy(&local_datapaths);
 
-        struct local_datapath *cur_node, *next_node;
-        HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node, &local_datapaths) {
-            free(cur_node->peer_dps);
-            hmap_remove(&local_datapaths, &cur_node->hmap_node);
-            free(cur_node);
+            if (br_int) {
+                ofctrl_wait();
+                pinctrl_wait(&ctx);
+            }
         }
-        hmap_destroy(&local_datapaths);
 
         unixctl_server_run(unixctl);
 
@@ -816,11 +808,6 @@ main(int argc, char *argv[])
             poll_immediate_wake();
         }
 
-        if (br_int) {
-            ofctrl_wait();
-            pinctrl_wait(&ctx);
-        }
-
         ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop);
 
         if (ovsdb_idl_loop_commit_and_wait(&ovs_idl_loop) == 1) {
@@ -840,8 +827,11 @@ main(int argc, char *argv[])
     }
 
     /* It's time to exit.  Clean up the databases. */
-    bool done = false;
+    bool done = !ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl);
     while (!done) {
+        update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl);
+        update_ssl_config(ovs_idl_loop.idl);
+
         struct controller_ctx ctx = {
             .ovs_idl = ovs_idl_loop.idl,
             .ovs_idl_txn = ovsdb_idl_loop_run(&ovs_idl_loop),
@@ -882,7 +872,6 @@ main(int argc, char *argv[])
     ovsdb_idl_loop_destroy(&ovs_idl_loop);
     ovsdb_idl_loop_destroy(&ovnsb_idl_loop);
 
-    free(ovnsb_remote);
     free(ovs_remote);
     service_stop();
 
@@ -1019,25 +1008,3 @@ inject_pkt(struct unixctl_conn *conn, int argc OVS_UNUSED,
     pending_pkt->conn = conn;
     pending_pkt->flow_s = xstrdup(argv[1]);
 }
-
-/* Get the desired SB probe timer from the OVS database and configure it into
- * the SB database. */
-static void
-update_probe_interval(struct controller_ctx *ctx, const char *ovnsb_remote)
-{
-    const struct ovsrec_open_vswitch *cfg
-        = ovsrec_open_vswitch_first(ctx->ovs_idl);
-    int interval = -1;
-    if (cfg) {
-        interval = smap_get_int(&cfg->external_ids,
-                                "ovn-remote-probe-interval",
-                                -1);
-    }
-    if (interval == -1) {
-        interval = stream_or_pstream_needs_probes(ovnsb_remote)
-                   ? DEFAULT_PROBE_INTERVAL_MSEC
-                   : 0;
-    }
-
-    ovsdb_idl_set_probe_interval(ctx->ovnsb_idl, interval);
-}
-- 
2.16.1



More information about the dev mailing list