From 7747eeb1b4f92d340db85e7467bf5313aff4049e 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 | 12 ++++++++++++ northd/ovn-northd.c | 26 ++++++++------------------ 4 files changed, 59 insertions(+), 45 deletions(-) diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 6eb7556754..61c79ac432 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -83,7 +83,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; @@ -3974,11 +3973,6 @@ flow_output_lflow_output_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 @@ -4049,9 +4043,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; ovs_cmdl_proctitle_init(argc, argv); @@ -4068,7 +4060,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(); @@ -4530,10 +4522,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); @@ -4944,7 +4934,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(); } @@ -4995,7 +4985,7 @@ main(int argc, char *argv[]) memory_wait(); poll_block(); if (should_service_stop()) { - exiting = true; + exit_args.exiting = true; } } @@ -5003,7 +4993,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, @@ -5055,7 +5045,6 @@ main(int argc, char *argv[]) } free(ovn_version); - unixctl_server_destroy(unixctl); lflow_destroy(); ofctrl_destroy(); pinctrl_destroy(); @@ -5073,6 +5062,8 @@ main(int argc, char *argv[]) ovs_feature_support_destroy(); free(ovs_remote); + ovn_exit_args_finish(&exit_args); + unixctl_server_destroy(unixctl); service_stop(); ovsrcu_exit(); @@ -5190,16 +5181,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 69ab56423a..40f4848430 100644 --- a/lib/ovn-util.c +++ b/lib/ovn-util.c @@ -947,3 +947,34 @@ lr_lb_address_set_ref(uint32_t lr_tunnel_key, int addr_family) { return lr_lb_address_set_name_(lr_tunnel_key, "$", addr_family); } + +/* 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 b35e470a20..fe50c00fab 100644 --- a/lib/ovn-util.h +++ b/lib/ovn-util.h @@ -337,4 +337,16 @@ int64_t daemon_startup_ts(void); char *lr_lb_address_set_name(uint32_t lr_tunnel_key, int addr_family); char *lr_lb_address_set_ref(uint32_t lr_tunnel_key, int addr_family); +/* 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 3a3af2a260..093d55eeae 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -46,7 +46,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; @@ -736,7 +735,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, @@ -758,7 +757,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, @@ -843,10 +843,9 @@ main(int argc, char *argv[]) run_update_worker_pool(n_threads); /* Main loop. */ - exiting = false; bool recompute = false; - while (!exiting) { + while (!exit_args.exiting) { update_ssl_config(); memory_run(); if (memory_should_report()) { @@ -978,7 +977,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(); } @@ -1009,15 +1008,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(); @@ -1025,16 +1025,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_)