Skip to content

Commit

Permalink
daemon: Make actually initiating reboot asynchronous
Browse files Browse the repository at this point in the history
Alternative to #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: #1348
  • Loading branch information
cgwalters committed May 25, 2021
1 parent d799cfe commit 9244d7a
Show file tree
Hide file tree
Showing 6 changed files with 52 additions and 16 deletions.
42 changes: 42 additions & 0 deletions src/daemon/rpmostreed-daemon.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ struct _RpmostreedDaemon {
GHashTable *bus_clients; /* <utf8 busname, struct RpmOstreeClient> */

gboolean running;
gboolean rebooting;
GDBusProxy *bus_proxy;
GSource *idle_exit_source;
guint rerender_status_id;
Expand Down Expand Up @@ -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)
{
Expand Down
2 changes: 2 additions & 0 deletions src/daemon/rpmostreed-daemon.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions src/daemon/rpmostreed-sysroot.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
12 changes: 6 additions & 6 deletions src/daemon/rpmostreed-transaction-types.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,7 @@ rollback_transaction_execute (RpmostreedTransaction *transaction,
}

if (self->reboot)
rpmostreed_reboot (cancellable, error);
rpmostreed_daemon_reboot (rpmostreed_daemon_get ());

return TRUE;
}
Expand Down Expand Up @@ -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
{
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}
Expand Down
8 changes: 0 additions & 8 deletions src/daemon/rpmostreed-utils.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 0 additions & 2 deletions src/daemon/rpmostreed-utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 9244d7a

Please sign in to comment.