On Fri, Oct 20, 2023 at 10:46 PM Ihar Hrachyshka <ihrac...@redhat.com>
wrote:
> 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.
>
Hi Ihar,
you are right we shouldn't assume C23 compatibility, interesting thing is
that both GCC and Clang generate the same instruction for both
interpretations AFIACT. I will send a patch to fix any occurence of "= {}"
within the codebase.
> > 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
>
>
Thanks,
Ales
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
amu...@redhat.com
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
