Thanks for the patch. On Wed, Apr 25, 2018 at 8:43 AM, Ben Pfaff <[email protected]> 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 <[email protected]> > Reported-at: https://github.com/openvswitch/ovs-issues/issues/144 > Signed-off-by: Ben Pfaff <[email protected]> > --- > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
