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

Darrell Ball dlu998 at gmail.com
Wed Apr 25 15:50:35 UTC 2018


Thanks for the patch.

On Wed, Apr 25, 2018 at 8:43 AM, Ben Pfaff <blp at ovn.org> wrote:

> 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
>

It looks like the above last sentence was truncated.




>
> 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
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list