Move reboot handling to client #2845

cgwalters · 2021-05-20T11:57:03Z

Having the daemon start the reboot means that it can be killed
by SIGTERM before the client processes the completion, causing
the client to exit with a spurious error.

Instead, have the client start the reboot. This is cleaner
in numerous ways. An additional benefit for example is that
if there are systemd reboot inhibitors present, it will be
the client that blocks, not the daemon.

kelvinfan001

LGTM! Just some questions/comments.

kelvinfan001 · 2021-05-21T01:50:35Z

src/daemon/rpmostreed-transaction-types.cxx

@@ -1526,7 +1526,10 @@ deploy_transaction_execute (RpmostreedTransaction *transaction,
        }

      if (deploy_has_bool_option (self, "reboot"))
-        rpmostreed_reboot (cancellable, error);
+        {
+          sd_journal_print (LOG_INFO, "daemon-initiated reboot is deprecated");


I was just wondering why we don't log a warning in the other methods too (e.g. initramfs_etc_transaction_execute(), kernel_arg_transaction_execute(), rollback_transaction_execute(), etc.)? Or if they should never be used anyways, since no one should be calling with that option, what if we just removed rpmostreed_reboot() from those functions?

For Zincati's purposes though, since it cares about the rpm-ostree finalize-deployment CLI command, which eventually calls the finalize_deployment_transaction_execute() daemon function, it looks like the reboot will still be on the daemon side. Is it possible to move the reboot to the client side for rpm-ostree finalize-deployment too?

initramfs_etc_transaction_execute()

Good catch! I missed at least that one because it used a different function for looking up the "reboot" flag so my code search missed it. Will update.

Is it possible to move the reboot to the client side for rpm-ostree finalize-deployment too?

Ah right, I should have mentioned this in the PR. I looked at finalize-deployment but the problem I see is that (this relates to your first point too) the DBus API should generally be considered a stable interface.

For example, https://github.com/cockpit-project/cockpit-ostree/blob/master/src/client.js and https://gitlab.gnome.org/GNOME/gnome-software/-/blob/master/plugins/rpm-ostree/gs-plugin-rpm-ostree.c

So what we'd likely need to do for e.g. finalize-deployment is add a reboot option, make it the default, and have our CLI pass FALSE or so.

Probably cleanest as a followup PR?

(Decided to fix FinalizeDeployment in this PR too, then we can log the deprecation in one place)

I think CI highlighted a regression here. The previous reboot logic was reachable by non-privileged processes through polkit ACL. This new code instead moves it client-side, where the same processes cannot directly trigger a reboot. That is the case for zincati:

zincati[1029]: [ERROR] failed to finalize deployment: rpm-ostree finalize-deployment failed: zincati[1029]: Failed to set wall message, ignoring: Interactive authentication required. zincati[1029]: Failed to reboot system via logind: Interactive authentication required. zincati[1029]: Failed to open initctl fifo: Permission denied zincati[1029]: Failed to talk to init daemon.

Mmm. OK what about actually pushing the reboot all the way into zincati, i.e. we extend the polkit rules there to allow it?

That'd be fine for zincati specifically, I'll look into if/how it can be done directly in polkit (I think so) or granular capabilities.
I am not sure about other consumers though, in particular the ones running containerized under their own PID namespace where "reboot" has a bit more nuances.

src/daemon/rpmostreed-transaction-types.cxx

lucab · 2021-05-21T12:37:53Z

src/app/rpmostree-clientlib.cxx

+gboolean
+rpmostree_client_reboot (GError ** error)
+{
+  execlp ("reboot", "reboot", NULL);


Is this exec really intended here? I was somehow expecting some kind of fork+exec spawning (like in the original rpmostreed_reboot). This way instead it looks a bit like a dangerous landmine, because in the non-error case any other code following this function call won't really be reached.

In a non-error case, the process will get killed by SIGTERM at some point soon later, so any code after would be inherently racy.

Self-note, I had a personal doubt related to blocking-behavior of this action, and I double-checked the systemd manual:

This command is asynchronous; it will return after the reboot operation is enqueued, without waiting for it to complete.

Compare https://doc.rust-lang.org/std/os/unix/process/trait.CommandExt.html#tymethod.exec

Brutal but fair point. Although Rust type-system represents such kind of never-returning functions, while here it's one more thing to keep in mind. At least a comment would help to highlight the intentional blackhole.

Anyway I agree this way it makes potential races obvious upfront, and we don't seem to currently have any critical logic running afterwards, so fine for me.

Well it can return, just on error. This is where the Rust type system shows its strength because unlike almost every other function it doesn't return Result<()> it returns Error.

Now you're completely right that the command itself is async...which is not usually what one wants. But that's also not a new issue here. The bug we're fixing is that the client can sometimes spuriously exit with an error because the daemon gets killed before it can send the success reply.

"never" as in the underlying diverging a case which can't be represented here. Sorry English language is confusing :)

Anyway I think we are in agreement. Mind adding a docstring highlighting that the TRUE case does not really return from this function blackhole and the whole execution flow diverges instead, and then we land it?

Having the daemon start the reboot means that it can be killed by `SIGTERM` before the client processes the completion, causing the client to exit with a spurious error. Instead, have the client start the reboot. This is cleaner in numerous ways. An additional benefit for example is that if there are systemd reboot inhibitors present, it will be the client that blocks, not the daemon.

Alternative to coreos#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: coreos#1348

cgwalters · 2021-05-21T16:33:34Z

Closing in favor of #2848

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

cgwalters force-pushed the client-reboot branch from 303718f to 8a102f6 Compare May 20, 2021 15:20

kelvinfan001 previously approved these changes May 21, 2021

View reviewed changes

cgwalters dismissed kelvinfan001’s stale review via 15e1e90 May 21, 2021 11:02

cgwalters force-pushed the client-reboot branch from 8a102f6 to 15e1e90 Compare May 21, 2021 11:02

lucab reviewed May 21, 2021

View reviewed changes

cgwalters force-pushed the client-reboot branch from 15e1e90 to 3a0e9dc Compare May 21, 2021 12:44

cgwalters mentioned this pull request May 21, 2021

daemon: Make actually initiating reboot asynchronous #2848

Merged

cgwalters closed this May 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move reboot handling to client #2845

Move reboot handling to client #2845

cgwalters commented May 20, 2021

kelvinfan001 left a comment

kelvinfan001 May 21, 2021 •

edited

Loading

cgwalters May 21, 2021

cgwalters May 21, 2021

lucab May 21, 2021

cgwalters May 21, 2021

lucab May 21, 2021 •

edited

Loading

lucab May 21, 2021

cgwalters May 21, 2021

lucab May 21, 2021

cgwalters May 21, 2021

lucab May 21, 2021 •

edited

Loading

cgwalters May 21, 2021

cgwalters May 21, 2021

lucab May 21, 2021

cgwalters commented May 21, 2021

Move reboot handling to client #2845

Move reboot handling to client #2845

Conversation

cgwalters commented May 20, 2021

kelvinfan001 left a comment

Choose a reason for hiding this comment

kelvinfan001 May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucab May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucab May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented May 21, 2021

kelvinfan001 May 21, 2021 •

edited

Loading

lucab May 21, 2021 •

edited

Loading

lucab May 21, 2021 •

edited

Loading