From 9244d7a8aec125957c593e41434842f39c42bca8 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 21 May 2021 11:43:48 -0400 Subject: [PATCH] daemon: Make actually initiating reboot asynchronous Alternative to https://github.com/coreos/rpm-ostree/pull/2845 which moved the `reboot` invocation into the client (which I think still makes sense in some cases). Previously we were starting the reboot before we're returned a success reply to the client, so it could see the daemon killed by `SIGTERM` and hence emit a spurious error. (Really in this case any 3 of the calling process, the client binary or the daemon could be killed in any order, so it's messy but this just closes one race) For cleanliness we reject starting any new transactions after the daemon has started a reboot. Closes: https://github.com/coreos/rpm-ostree/issues/1348 --- src/daemon/rpmostreed-daemon.cxx | 42 +++++++++++++++++++++ src/daemon/rpmostreed-daemon.h | 2 + src/daemon/rpmostreed-sysroot.cxx | 2 + src/daemon/rpmostreed-transaction-types.cxx | 12 +++--- src/daemon/rpmostreed-utils.cxx | 8 ---- src/daemon/rpmostreed-utils.h | 2 - 6 files changed, 52 insertions(+), 16 deletions(-) diff --git a/src/daemon/rpmostreed-daemon.cxx b/src/daemon/rpmostreed-daemon.cxx index 60ae58e7..439ca45f 100644 --- a/src/daemon/rpmostreed-daemon.cxx +++ b/src/daemon/rpmostreed-daemon.cxx @@ -61,6 +61,7 @@ struct _RpmostreedDaemon { GHashTable *bus_clients; /* */ gboolean running; + gboolean rebooting; GDBusProxy *bus_proxy; GSource *idle_exit_source; guint rerender_status_id; @@ -774,6 +775,47 @@ rpmostreed_daemon_exit_now (RpmostreedDaemon *self) self->running = FALSE; } +static gboolean +idle_initiate_reboot (void *_unused) +{ + sd_journal_print (LOG_INFO, "Initiating reboot requested from transaction"); + + /* Note that we synchronously spawn this command, but the command just queues the request and returns. + */ + const char *child_argv[] = { "systemctl", "reboot", NULL }; + g_autoptr(GError) local_error = NULL; + if (!g_spawn_sync (NULL, (char**)child_argv, NULL, (GSpawnFlags)(G_SPAWN_CHILD_INHERITS_STDIN | G_SPAWN_SEARCH_PATH), + NULL, NULL, NULL, NULL, NULL, &local_error)) + { + sd_journal_print (LOG_WARNING, "Failed to initate reboot: %s", local_error->message); + // And now...not a lot of great choices. We could loop and retry, but exiting will surface the error + // in an obvious way. + exit (1); + } + + return FALSE; +} + +void +rpmostreed_daemon_reboot (RpmostreedDaemon *self) +{ + g_assert (!self->rebooting); + self->rebooting = TRUE; + /* Queue actually starting the reboot until we return to the client, so + * that they get a success message for the transaction. Otherwise + * if the daemon gets killed via SIGTERM they just see the bus connection + * broken and may spuriously error out. + */ + g_idle_add_full (G_PRIORITY_LOW, idle_initiate_reboot, NULL, NULL); +} + +gboolean +rpmostreed_daemon_is_rebooting (RpmostreedDaemon *self) +{ + return self->rebooting; +} + + void rpmostreed_daemon_run_until_idle_exit (RpmostreedDaemon *self) { diff --git a/src/daemon/rpmostreed-daemon.h b/src/daemon/rpmostreed-daemon.h index ceeab212..04182a8a 100644 --- a/src/daemon/rpmostreed-daemon.h +++ b/src/daemon/rpmostreed-daemon.h @@ -53,6 +53,8 @@ char * rpmostreed_daemon_client_get_agent_id (RpmostreedDaemon *self char * rpmostreed_daemon_client_get_sd_unit (RpmostreedDaemon *self, const char *client); void rpmostreed_daemon_exit_now (RpmostreedDaemon *self); +void rpmostreed_daemon_reboot (RpmostreedDaemon *self); +gboolean rpmostreed_daemon_is_rebooting (RpmostreedDaemon *self); void rpmostreed_daemon_run_until_idle_exit (RpmostreedDaemon *self); void rpmostreed_daemon_publish (RpmostreedDaemon *self, const gchar *path, diff --git a/src/daemon/rpmostreed-sysroot.cxx b/src/daemon/rpmostreed-sysroot.cxx index 7bb48f32..1218a3c0 100644 --- a/src/daemon/rpmostreed-sysroot.cxx +++ b/src/daemon/rpmostreed-sysroot.cxx @@ -818,6 +818,8 @@ rpmostreed_sysroot_prep_for_txn (RpmostreedSysroot *self, RpmostreedTransaction **out_compat_txn, GError **error) { + if (rpmostreed_daemon_is_rebooting (rpmostreed_daemon_get())) + return glnx_throw (error, "Reboot initiated, cannot start new transaction"); if (self->transaction) { if (rpmostreed_transaction_is_compatible (self->transaction, invocation)) diff --git a/src/daemon/rpmostreed-transaction-types.cxx b/src/daemon/rpmostreed-transaction-types.cxx index bc194e6a..ae67a56a 100644 --- a/src/daemon/rpmostreed-transaction-types.cxx +++ b/src/daemon/rpmostreed-transaction-types.cxx @@ -546,7 +546,7 @@ rollback_transaction_execute (RpmostreedTransaction *transaction, } if (self->reboot) - rpmostreed_reboot (cancellable, error); + rpmostreed_daemon_reboot (rpmostreed_daemon_get ()); return TRUE; } @@ -1526,7 +1526,7 @@ deploy_transaction_execute (RpmostreedTransaction *transaction, } if (deploy_has_bool_option (self, "reboot")) - rpmostreed_reboot (cancellable, error); + rpmostreed_daemon_reboot (rpmostreed_daemon_get ()); } else { @@ -1928,7 +1928,7 @@ initramfs_etc_transaction_execute (RpmostreedTransaction *transaction, return FALSE; if (vardict_lookup_bool (self->options, "reboot", FALSE)) - rpmostreed_reboot (cancellable, error); + rpmostreed_daemon_reboot (rpmostreed_daemon_get ()); return TRUE; } @@ -2066,7 +2066,7 @@ initramfs_state_transaction_execute (RpmostreedTransaction *transaction, return FALSE; if (vardict_lookup_bool (self->options, "reboot", FALSE)) - rpmostreed_reboot (cancellable, error); + rpmostreed_daemon_reboot (rpmostreed_daemon_get ()); return TRUE; } @@ -2625,7 +2625,7 @@ finalize_deployment_transaction_execute (RpmostreedTransaction *transaction, (void) rpmostree_syscore_bump_mtime (sysroot, NULL); sd_journal_print (LOG_INFO, "Finalized deployment; rebooting into %s", checksum); - rpmostreed_reboot (cancellable, error); + rpmostreed_daemon_reboot (rpmostreed_daemon_get ()); return TRUE; } @@ -2802,7 +2802,7 @@ kernel_arg_transaction_execute (RpmostreedTransaction *transaction, return FALSE; if (vardict_lookup_bool (self->options, "reboot", FALSE)) - rpmostreed_reboot (cancellable, error); + rpmostreed_daemon_reboot (rpmostreed_daemon_get ()); return TRUE; } diff --git a/src/daemon/rpmostreed-utils.cxx b/src/daemon/rpmostreed-utils.cxx index c4420e1b..ccfccd9f 100644 --- a/src/daemon/rpmostreed-utils.cxx +++ b/src/daemon/rpmostreed-utils.cxx @@ -211,14 +211,6 @@ rpmostreed_refspec_parse_partial (const gchar *new_provided_refspec, return TRUE; } -void -rpmostreed_reboot (GCancellable *cancellable, GError **error) -{ - const char *child_argv[] = { "systemctl", "reboot", NULL }; - (void) g_spawn_sync (NULL, (char**)child_argv, NULL, (GSpawnFlags)(G_SPAWN_CHILD_INHERITS_STDIN | G_SPAWN_SEARCH_PATH), - NULL, NULL, NULL, NULL, NULL, NULL); -} - /** * rpmostreed_repo_pull_ancestry: * @repo: Repo diff --git a/src/daemon/rpmostreed-utils.h b/src/daemon/rpmostreed-utils.h index aa052619..f4408cb5 100644 --- a/src/daemon/rpmostreed-utils.h +++ b/src/daemon/rpmostreed-utils.h @@ -36,8 +36,6 @@ gboolean rpmostreed_refspec_parse_partial (const gchar *new_provided_refspec, const gchar *base_refspec, gchar **out_refspec, GError **error); -void -rpmostreed_reboot (GCancellable *cancellable, GError **error); /* XXX These pull-ancestry and lookup-version functions should eventually * be integrated into libostree, but it's still a bit premature to do