Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

daemon: Make actually initiating reboot asynchronous #2848

Merged
merged 1 commit into from
May 25, 2021
Merged

daemon: Make actually initiating reboot asynchronous #2848

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions src/daemon/rpmostreed-daemon.cxx
View file
Open in desktop
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),
Copy link
Member

@kelvinfan001 kelvinfan001 May 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if there is a systemd inhibitor lock that is blocking shutdown? Would the systemctl reboot command spawned this function fail later when the reboot request is attempted, or error immediately and the daemon exits?
If the former, when used in finalize_deployment(), would we have an unlocked staged deployment since we'd unlink() _OSTREE_SYSROOT_RUNSTATE_STAGED_LOCKED but not actually reboot and return a success message to the client even if we later fail to actually reboot?
So it seems like both of these situations are not ideal. (I may also just have misunderstood systemd-inhibit and these bits of code entirely)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! So...Per this point I think really the only correct thing here is for e.g. zincati to not try to finalize if there are inhibitors. I can't think of a way to solve this without adding some sort of new inhibitor API to do something more read/write lock like so that one could do "grab an exclusive lock only when the last (read) locker has dropped".

That said honestly I was just playing with this locally and I assume I'm doing something wrong, but I can't get systemd-inhibit to block reboots. I'm doing systemd-inhibit sleep 1h in one shell, and systemctl reboot in another instantly reboots.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(OK some investigation shows I need to use systemctl reboot --check-inhibitors=yes which...seems like a bug, the man page claims auto will do that on a tty, but that's not what I'm seeing)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so the --check-inhibitors option was made available in a recent-ish commit, and is not in FCOS yet; but even so, reboots should be inhibited successfully if called on a tty. I tried locally with systemd-inhibit sleep 1h &, then if I do systemctl reboot, it is indeed inhibited.

systemctl poweroff
Operation inhibited by "sleep 1h" (PID 2653 "systemd-inhibit", user root), reason is "Unknown reason".
Please retry operation after closing inhibitors and logging out other users.
Alternatively, ignore inhibitors and users with 'systemctl poweroff -i'.

However, another issue I see right now regarding systemd-inhibit is that I haven't been able to get my inhibitor locks to inhibit rpm-ostree-driven reboots, even if I set the lock as root. I assume this is because the rpm-ostree daemon is privileged and the systemctl reboot called by the daemon is non-interactive, and so in e.g. systemd 246 (before systemd/systemd#18210), any inhibitors are ignored by rpm-ostree. So IIUC, systemd-inhibitors won't really work with rpm-ostree today, at least not until systemd 248, after which we can add the --check-inhibitors=yes option?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK yeah if you follow issue links there's a huge amount of debate about this.

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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,7 +542,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 @@ -1511,7 +1511,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 @@ -1913,7 +1913,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 @@ -2051,7 +2051,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 @@ -2610,7 +2610,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 @@ -2787,7 +2787,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
View file
Open in desktop
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
View file
Open in desktop
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