On 10/6/23 3:02 AM, Ales Musil wrote:
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 <amu...@redhat.com>
---
  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 9a81f1a80..a8c48630f 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;
@@ -4901,11 +4900,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
@@ -4980,9 +4974,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 = {};

AFAIU = {} is allowed in C23+ only. Should we assume that the compiler is C23 compliant? (If not, I think = { 0 } would initialize all members to zero for all C versions.)

See: https://en.cppreference.com/w/c/language/struct_initialization (in Notes section) for details.

      int retval;
/* Read from system-id-override file once on startup. */
@@ -5002,7 +4994,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();
@@ -5514,10 +5506,8 @@ main(int argc, char *argv[])
      VLOG_INFO("OVN internal version is : [%s]", ovn_version);
/* Main loop. */
-    exiting = false;
-    restart = false;
(you can see that previously, we were explicit about the starting values for the variables - which in theory also allowed a "exit" command issued right before these two lines above to have no effect, but that's a separate problem.)
      bool sb_monitor_all = false;
-    while (!exiting) {
+    while (!exit_args.exiting) {
          memory_run();
          if (memory_should_report()) {
              struct simap usage = SIMAP_INITIALIZER(&usage);
@@ -5954,7 +5944,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();
          }
@@ -6005,7 +5995,7 @@ loop_done:
          memory_wait();
          poll_block();
          if (should_service_stop()) {
-            exiting = true;
+            exit_args.exiting = true;
          }
      }
@@ -6013,7 +6003,7 @@ loop_done:
      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,
@@ -6065,7 +6055,6 @@ loop_done:
      }
free(ovn_version);
-    unixctl_server_destroy(unixctl);
      lflow_destroy();
      ofctrl_destroy();
      pinctrl_destroy();
@@ -6090,6 +6079,8 @@ loop_done:
      if (cli_system_id) {
          free(cli_system_id);
      }
+    ovn_exit_args_finish(&exit_args);
+    unixctl_server_destroy(unixctl);
      service_stop();
      ovsrcu_exit();
@@ -6213,16 +6204,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 ffe295696..33105202f 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 16f18353d..bff50dbde 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 68fc8836e..b2ec67c06 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_)

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to