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 <amusil@redhat.com>
Acked-by: Mark Michelson <mmichels@redhat.com>
Signed-off-by: Mark Michelson <mmichels@redhat.com>

controller/ovn-controller.c

@@ -86,7 +86,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;
@@ -4566,11 +4565,6 @@ controller_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
@@ -4645,9 +4639,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. */
@@ -4667,7 +4659,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();
@@ -5162,10 +5154,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);
@@ -5586,7 +5576,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();
         }
 
@@ -5637,15 +5627,15 @@ main(int argc, char *argv[])
         memory_wait();
         poll_block();
         if (should_service_stop()) {
-            exiting = true;
+            exit_args.exiting = true;
         }
     }
 
     engine_set_context(NULL);
     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,
@@ -5697,7 +5687,6 @@ main(int argc, char *argv[])
     }
 
     free(ovn_version);
-    unixctl_server_destroy(unixctl);
     lflow_destroy();
     ofctrl_destroy();
     pinctrl_destroy();
@@ -5721,6 +5710,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();
 
@@ -5844,16 +5835,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_)

lib/ovn-util.c

@@ -1126,3 +1126,34 @@ void flow_collector_ids_clear(struct flow_collector_ids *ids)
     flow_collector_ids_destroy(ids);
     flow_collector_ids_init(ids);
 }
+
+/* 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);
+}

lib/ovn-util.h

@@ -403,4 +403,16 @@ bool flow_collector_ids_lookup(const struct flow_collector_ids *, uint32_t);
 void flow_collector_ids_destroy(struct flow_collector_ids *);
 void flow_collector_ids_clear(struct flow_collector_ids *);
 
+/* 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 */

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;
@@ -748,7 +747,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,
@@ -770,7 +769,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,
@@ -855,10 +855,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()) {
@@ -993,7 +992,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();
         }
 
@@ -1023,32 +1022,23 @@ 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();
 
     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_)