From aae5b2ec8ec9f4f9f7c9738d23818c2c4967627c Mon Sep 17 00:00:00 2001 From: Ales Musil Date: Fri, 6 Oct 2023 09:02:18 +0200 Subject: [PATCH] controller, northd: Wait for cleanup before replying to exit The unixctl exit command would receive reply immediately which is confusing and can cause some issues in some tests if the cleanup takes longer than expected. To avoid that make sure we reply to the exit command only after the main cleanup was done so there shouldn't be any possible window when the services are working when they are no longer expected to. Because it is in theory possible that we will receive multiple exit commands while waiting for the cleanup, make sure that we will reply to all of them by storing them in array. At the same time unify the exit structure for both ovn-controller and northd, so it can be easily extended as needed. This is inspired by OvS commit that was solving similar issue: 24520a401e06 ("vswitchd: Wait for a bridge exit before replying to exit unixctl.") Signed-off-by: Ales Musil Acked-by: Mark Michelson Signed-off-by: Mark Michelson --- controller/ovn-controller.c | 35 ++++++++--------------------------- lib/ovn-util.c | 31 +++++++++++++++++++++++++++++++ lib/ovn-util.h | 13 +++++++++++++ northd/ovn-northd.c | 27 ++++++++------------------- 4 files changed, 60 insertions(+), 46 deletions(-) diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index fbb501cab4..da7d145ed3 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -88,7 +88,6 @@ VLOG_DEFINE_THIS_MODULE(main); -static unixctl_cb_func ovn_controller_exit; static unixctl_cb_func ct_zone_list; static unixctl_cb_func extend_table_list; static unixctl_cb_func inject_pkt; @@ -4914,11 +4913,6 @@ controller_output_mac_cache_handler(struct engine_node *node, return true; } -struct ovn_controller_exit_args { - bool *exiting; - bool *restart; -}; - /* Handles sbrec_chassis changes. * If a new chassis is added or removed return false, so that * flows are recomputed. For any updates, there is no need for @@ -4993,9 +4987,7 @@ int main(int argc, char *argv[]) { struct unixctl_server *unixctl; - bool exiting; - bool restart; - struct ovn_controller_exit_args exit_args = {&exiting, &restart}; + struct ovn_exit_args exit_args = {}; int retval; /* Read from system-id-override file once on startup. */ @@ -5015,7 +5007,7 @@ main(int argc, char *argv[]) if (retval) { exit(EXIT_FAILURE); } - unixctl_command_register("exit", "", 0, 1, ovn_controller_exit, + unixctl_command_register("exit", "", 0, 1, ovn_exit_command_callback, &exit_args); daemonize_complete(); @@ -5527,10 +5519,8 @@ main(int argc, char *argv[]) VLOG_INFO("OVN internal version is : [%s]", ovn_version); /* Main loop. */ - exiting = false; - restart = false; bool sb_monitor_all = false; - while (!exiting) { + while (!exit_args.exiting) { memory_run(); if (memory_should_report()) { struct simap usage = SIMAP_INITIALIZER(&usage); @@ -5967,7 +5957,7 @@ main(int argc, char *argv[]) unixctl_server_run(unixctl); unixctl_server_wait(unixctl); - if (exiting || pending_pkt.conn) { + if (exit_args.exiting || pending_pkt.conn) { poll_immediate_wake(); } @@ -6018,7 +6008,7 @@ main(int argc, char *argv[]) memory_wait(); poll_block(); if (should_service_stop()) { - exiting = true; + exit_args.exiting = true; } } @@ -6026,7 +6016,7 @@ main(int argc, char *argv[]) engine_cleanup(); /* It's time to exit. Clean up the databases if we are not restarting */ - if (!restart) { + if (!exit_args.restart) { bool done = !ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl); while (!done) { update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl, @@ -6078,7 +6068,6 @@ main(int argc, char *argv[]) } free(ovn_version); - unixctl_server_destroy(unixctl); lflow_destroy(); ofctrl_destroy(); pinctrl_destroy(); @@ -6103,6 +6092,8 @@ main(int argc, char *argv[]) if (cli_system_id) { free(cli_system_id); } + ovn_exit_args_finish(&exit_args); + unixctl_server_destroy(unixctl); service_stop(); ovsrcu_exit(); @@ -6226,16 +6217,6 @@ usage(void) exit(EXIT_SUCCESS); } -static void -ovn_controller_exit(struct unixctl_conn *conn, int argc, - const char *argv[], void *exit_args_) -{ - struct ovn_controller_exit_args *exit_args = exit_args_; - *exit_args->exiting = true; - *exit_args->restart = argc == 2 && !strcmp(argv[1], "--restart"); - unixctl_command_reply(conn, NULL); -} - static void ct_zone_list(struct unixctl_conn *conn, int argc OVS_UNUSED, const char *argv[] OVS_UNUSED, void *ct_zones_) diff --git a/lib/ovn-util.c b/lib/ovn-util.c index ffe2956963..33105202f2 100644 --- a/lib/ovn-util.c +++ b/lib/ovn-util.c @@ -1302,3 +1302,34 @@ sorted_array_apply_diff(const struct sorted_array *a1, apply_callback(arg, a2->arr[idx2], false); } } + +/* Call for the unixctl command that will store the connection and + * set the appropriate conditions. */ +void +ovn_exit_command_callback(struct unixctl_conn *conn, int argc, + const char *argv[], void *exit_args_) +{ + struct ovn_exit_args *exit_args = exit_args_; + + exit_args->n_conns++; + exit_args->conns = xrealloc(exit_args->conns, + exit_args->n_conns * sizeof *exit_args->conns); + + exit_args->exiting = true; + exit_args->conns[exit_args->n_conns - 1] = conn; + + if (!exit_args->restart) { + exit_args->restart = argc == 2 && !strcmp(argv[1], "--restart"); + } +} + +/* Reply to all waiting unixctl connections and free the connection array. + * This function should be called after the heaviest cleanup has finished. */ +void +ovn_exit_args_finish(struct ovn_exit_args *exit_args) +{ + for (size_t i = 0; i < exit_args->n_conns; i++) { + unixctl_command_reply(exit_args->conns[i], NULL); + } + free(exit_args->conns); +} diff --git a/lib/ovn-util.h b/lib/ovn-util.h index 16f18353da..bff50dbde9 100644 --- a/lib/ovn-util.h +++ b/lib/ovn-util.h @@ -474,4 +474,17 @@ void sorted_array_apply_diff(const struct sorted_array *a1, const char *item, bool add), const void *arg); + +/* Utilities around properly handling exit command. */ +struct ovn_exit_args { + struct unixctl_conn **conns; + size_t n_conns; + bool exiting; + bool restart; +}; + +void ovn_exit_command_callback(struct unixctl_conn *conn, int argc, + const char *argv[], void *exit_args_); +void ovn_exit_args_finish(struct ovn_exit_args *exit_args); + #endif /* OVN_UTIL_H */ diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 68fc8836e8..b2ec67c067 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -47,7 +47,6 @@ VLOG_DEFINE_THIS_MODULE(ovn_northd); -static unixctl_cb_func ovn_northd_exit; static unixctl_cb_func ovn_northd_pause; static unixctl_cb_func ovn_northd_resume; static unixctl_cb_func ovn_northd_is_paused; @@ -752,7 +751,7 @@ main(int argc, char *argv[]) int res = EXIT_SUCCESS; struct unixctl_server *unixctl; int retval; - bool exiting; + struct ovn_exit_args exit_args = {}; int n_threads = 1; struct northd_state state = { .had_lock = false, @@ -774,7 +773,8 @@ main(int argc, char *argv[]) if (retval) { exit(EXIT_FAILURE); } - unixctl_command_register("exit", "", 0, 0, ovn_northd_exit, &exiting); + unixctl_command_register("exit", "", 0, 0, ovn_exit_command_callback, + &exit_args); unixctl_command_register("pause", "", 0, 0, ovn_northd_pause, &state); unixctl_command_register("resume", "", 0, 0, ovn_northd_resume, &state); unixctl_command_register("is-paused", "", 0, 0, ovn_northd_is_paused, @@ -878,11 +878,9 @@ main(int argc, char *argv[]) run_update_worker_pool(n_threads); /* Main loop. */ - exiting = false; - struct northd_engine_context eng_ctx = {}; - while (!exiting) { + while (!exit_args.exiting) { update_ssl_config(); memory_run(); if (memory_should_report()) { @@ -1024,7 +1022,7 @@ main(int argc, char *argv[]) unixctl_server_run(unixctl); unixctl_server_wait(unixctl); memory_wait(); - if (exiting) { + if (exit_args.exiting) { poll_immediate_wake(); } @@ -1057,15 +1055,16 @@ main(int argc, char *argv[]) stopwatch_stop(NORTHD_LOOP_STOPWATCH_NAME, time_msec()); poll_block(); if (should_service_stop()) { - exiting = true; + exit_args.exiting = true; } stopwatch_start(NORTHD_LOOP_STOPWATCH_NAME, time_msec()); } inc_proc_northd_cleanup(); - unixctl_server_destroy(unixctl); ovsdb_idl_loop_destroy(&ovnnb_idl_loop); ovsdb_idl_loop_destroy(&ovnsb_idl_loop); + ovn_exit_args_finish(&exit_args); + unixctl_server_destroy(unixctl); service_stop(); run_update_worker_pool(0); ovsrcu_exit(); @@ -1073,16 +1072,6 @@ main(int argc, char *argv[]) exit(res); } -static void -ovn_northd_exit(struct unixctl_conn *conn, int argc OVS_UNUSED, - const char *argv[] OVS_UNUSED, void *exiting_) -{ - bool *exiting = exiting_; - *exiting = true; - - unixctl_command_reply(conn, NULL); -} - static void ovn_northd_pause(struct unixctl_conn *conn, int argc OVS_UNUSED, const char *argv[] OVS_UNUSED, void *state_)