daemon: Respect systemd inhibitor locks #2862

kelvinfan001 · 2021-05-27T22:27:20Z

Today, non-interactive processes, including rpm-ostree, are not
inhibited by systemd inhibitor locks. systemd 248 added the
--check-inhibitors option to systemctl commands, which can be
used to make non-interactive proccesses check for shutdown/sleep
inhibitors. However, it also checks for delay mode inhibitors,
which isn't what we want. Replicate this logic prior to reboots,
but ignore delay mode inhibitors.

src/daemon/rpmostreed-daemon.cxx

src/daemon/rpmostreed-utils.cxx

jlebon

Can you link to coreos/zincati#245 somewhere in the commit message?

It's not obvious to me that we want to default to failing on block inhibitors for all reboot operations. I think the behaviour makes sense for finalize-deployment since it's tailor-made for Zincati and we do want that there.

But otherwise, ISTM like we should do something like matching the systemd behaviour of respecting inhibitors if interactive, and not respecting it otherwise. (This implies having a similar --check-inhibitors flag and proxying that via a new check-inhibitors option in the D-Bus methods.)

This also technically breaks any script/service which rely on the current behaviour of ignoring inhibitors.

I think also even in Zincati we should enforce some time limit after which it should just ignore inhibitors. That will prevent some buggy service breaking updates entirely.

So my strawman is:

make FinalizeDeployment fail on block inhibitors by default
have rpm-ostree finalize-deployment exit with e.g. code 66 if inhibitors blocked us
add a finalize-deployment --ignore-inhibitors to force finalization and reboot even if inhibitors are present
teach Zincati to use --ignore-inhibitors if we've been getting error code 66 after X amount of time (probably would have to make this configurable; and then users can opt into blocking forever if they really want that using e.g. <newknob>=-1)
leave all other rpm-ostree APIs alone for now

WDYT?

tests/vmcheck/test-misc-2.sh

src/daemon/rpmostreed-utils.cxx

src/daemon/rpmostreed-transaction-types.cxx

src/daemon/rpmostreed-utils.cxx

kelvinfan001 · 2021-05-31T17:04:53Z

@jlebon Thanks for the thorough feedback and review!

Your strawman makes sense to me. Just a comment on the last note though, there currently isn't any way to block rpm-ostree using systemd inhibitors. For example, if I hold a lock with the root user and then do an rpm-ostree upgrade --reboot interactively, the root-owned lock will be ignored. Is this the desired behaviour?

This also technically breaks any script/service which rely on the current behaviour of ignoring inhibitors.

This is indeed a concern, but should we at least provide a way to get interactive rpm-ostree CLI calls to respect inhibitors as well?

jlebon · 2021-05-31T18:58:59Z

@jlebon Thanks for the thorough feedback and review!

Your strawman makes sense to me. Just a comment on the last note though, there currently isn't any way to block rpm-ostree using systemd inhibitors. For example, if I hold a lock with the root user and then do an rpm-ostree upgrade --reboot interactively, the root-owned lock will be ignored. Is this the desired behaviour?

I think we can debate this more, though offhand, I think it would make sense to match systemd like I mentioned above (not ignore it if interactive, ignore otherwise). This has the least chance of breaking workflows.

This also technically breaks any script/service which rely on the current behaviour of ignoring inhibitors.

This is indeed a concern, but should we at least provide a way to get interactive rpm-ostree CLI calls to respect inhibitors as well?

Right yeah, if we go with matching systemd, we could have a similar --check-inhibitors switch too.

But honestly, unless there's a request for this outside of the Zincati context, I would just punt on this for now for the other APIs and focus on the Zincati path.

cgwalters · 2021-06-01T19:58:21Z

I think we can debate this more, though offhand, I think it would make sense to match systemd like I mentioned above (not ignore it if interactive, ignore otherwise). This has the least chance of breaking workflows.

I think the systemd default behavior is basically just broken. It makes block inhibitors only useful in very specific circumstances around interactive desktop systems. Upstream mostly acknowledged this.

The problem here really revolves around what happens when one just writes reboot in a shell or script. No one is ever going to train their fingers to type reboot --check-inhibitors or remember to do it in a script. Not to mention the fact that it's new and will take a long time to propagate.

If we make the default in rpm-ostree to honor them we bypass all of that.

I just can't think of a real world scenario where:

A block inhibitor is present
The system owner doesn't want it to be respected

cgwalters · 2021-06-01T20:17:08Z

I think also even in Zincati we should enforce some time limit after which it should just ignore inhibitors. That will prevent some buggy service breaking updates entirely.

But that's creating a whole new kind of block-for-a-while that an administrator has to understand. I don't think we should be second guessing other privileged code.

And things get worse if e.g. multiple components start implementing this block-for-a-while in their own code with their own arbitrary timeouts - one could be shorter than another.

I feel pretty strongly if an admin (or privileged code) adds a block then it should simply block, we shouldn't second guess them.

Our default is already to reboot autonomously. This mechanism comes into play when an admin wants more control over their computer. And we should respect that.

kelvinfan001 · 2021-06-01T20:27:58Z

Yeah, it does seem weird to me how the default upstream is that there are cases (e.g. scripts, non-interactive code) that are immune to inhibitors in block mode. I do think that it is more intuitive to me if block inhibitors held by privileged code were authoritative, but it also feels weird to have different behaviour from the defaults upstream. (Having to decide for Zincati whether it should respect block locks and to what extent feels like a result of us disagreeing with the upstream defaults, forcing us to make our own decisions on this)

cgwalters · 2021-06-01T20:28:15Z

My core argument here for not being consistent with systemd's default reboot semantics is pretty simple: In Fedora CoreOS, we reboot systems by default.

The semantics and control over it are hence greatly magnified in importance. The block inhibitor is really useful for this because it's something that already has a standard API implemented in systemd for years, with useful tools like systemd-inhibit --list for debugging it, etc. The only flaw is that (for some reason, probably worries about compat) they originally chose to ignore it when not on a tty by default. And here we are.

I think we just fix this at our level and then related to the concern around "what if there is a block inhibitor": I'm skeptical of the need for --ignore-inhibitors at all - anyone in a position to type that interactively can easily kill the offending process/service holding the lock too. (And I covered the argument against the automatic timeout above)

jlebon · 2021-06-01T20:37:27Z

I think also even in Zincati we should enforce some time limit after which it should just ignore inhibitors. That will prevent some buggy service breaking updates entirely.

But that's creating a whole new kind of block-for-a-while that an administrator has to understand. I don't think we should be second guessing other privileged code.

And things get worse if e.g. multiple components start implementing this block-for-a-while in their own code with their own arbitrary timeouts - one could be shorter than another.

I feel pretty strongly if an admin (or privileged code) adds a block then it should simply block, we shouldn't second guess them.

Our default is already to reboot autonomously. This mechanism comes into play when an admin wants more control over their computer. And we should respect that.

My concern is more with systemd services which e.g. take a lock and then hang. That would potentially create deadends in the update graph and require manual intervention to unbreak. ISTM like at some point, we should be able to make the call and say "no reasonable service should be inhibiting updates for X days; ignore it".

For the interactive admin case, we already have coreos/zincati#485 which helps there and I think we can expect admins to simply do systemctl stop zincati if they actually want to fully disable updates upon getting the notice rather than manually take a lock with systemd-inhibit.

jlebon · 2021-06-01T20:57:57Z

I just can't think of a real world scenario where:

A block inhibitor is present

The system owner doesn't want it to be respected

I think for the desktop case that's true. For the server case/non-interactive cases, it's not unreasonable IMO to have whatever is driving updates provide a grace period before going ahead and rebooting anyway. Kinda like systemd will just kill processes after some time if they're blocking reboot.

cgwalters · 2021-06-01T21:09:05Z

My concern is more with systemd services which e.g. take a lock and then hang.

Do you know of a scenario where that's happened? And furthermore the admin would actually prefer us to ignore it and reboot anyways after some period of time?

I can say for sure I know of one RH customer who really, really really wanted absolute control over when individual machines rebooted. They have once-a-month maintenance windows and need some manual action taken around managing workloads. In this scenario, I don't think there's any default value for a "timeout" that they'd want.

I'd love to be able to tell them "Hey we fixed systemd inhibitors, so you can now use those and e.g. the MCO will honor them".

cgwalters · 2021-06-01T21:14:35Z

Kinda like systemd will just kill processes after some time if they're blocking reboot.

I think that analogy doesn't quite apply here because there the admin (or a privileged process) has explicitly initiated a reboot - we're trying to honor its will. We necessarily only give (mostly unprivileged) processes a grace period to delay.

Speaking of "honoring its will" - that's the simple way to say this: Who is in charge? Is it the admin or code initiating a block? Or do we think the ultimate "in charge" agent is zincati that is just ultimately going to win, and if you don't want that, then you need to stop it?

jlebon · 2021-06-01T22:05:58Z

Do you know of a scenario where that's happened? And furthermore the admin would actually prefer us to ignore it and reboot anyways after some period of time?

This is hard to answer because that's exactly the behaviour we're changing now (going from not caring to caring about systemd inhibitors). So we may have hit this 0 times or 10 times in the past. I realize this isn't a convincing argument. But I'm also very conservative when it comes to avoiding deadends.

I can say for sure I know of one RH customer who really, really really wanted absolute control over when individual machines rebooted. They have once-a-month maintenance windows and need some manual action taken around managing workloads. In this scenario, I don't think there's any default value for a "timeout" that they'd want.

I'd love to be able to tell them "Hey we fixed systemd inhibitors, so you can now use those and e.g. the MCO will honor them".

IMO, that doesn't really sound like a good use case for systemd inhibitors. In a Zincati-controlled scenario, they'd be better served just having coreos/zincati#498 and doing rpm-ostree upgrade manually themselves when they're ready. Otherwise, it's like awkwardly trying to mix manual and automated updates. (Re. the MCO, correct me if I'm wrong but it doesn't actually use rpm-ostree's --reboot flag, right?)

Speaking of "honoring its will" - that's the simple way to say this: Who is in charge? Is it the admin or code initiating a block? Or do we think the ultimate "in charge" agent is zincati that is just ultimately going to win, and if you don't want that, then you need to stop it?

Yeah, that's a good way to look at it. If I had to order them, it'd be: admins, Zincati/MCO, and everything else. Since we can't easily tell apart admins from "everything else", we're left having to make this call. I think it's a tough decision, and in the end I'm fine going with whatever the team decides! 👍

cgwalters · 2021-06-02T13:47:59Z

IMO, that doesn't really sound like a good use case for systemd inhibitors. In a Zincati-controlled scenario, they'd be better served just having coreos/zincati#498 and doing rpm-ostree upgrade manually themselves when they're ready. Otherwise, it's like awkwardly trying to mix manual and automated updates.

I think we also want to guard against other things that might invoke reboot, which could include admins accidentally logged into the wrong shell, etc.

(Re. the MCO, correct me if I'm wrong but it doesn't actually use rpm-ostree's --reboot flag, right?)

No, but it could learn to check inhibitors in the same way we're doing it here.

cgwalters · 2021-06-02T15:20:41Z

Random note from chat; I remembered that rpm-plugin-systemd-inhibit exists, which is both:

Not necessary for rpm-ostree because the updates are transactional
Might cause a problem if used in rpm-ostreed actually

It's not installed in FCOS by default, but seems to be in e.g. FSB.

src/daemon/rpmostreed-transaction-types.cxx

src/daemon/rpmostreed-utils.cxx

kelvinfan001 · 2021-06-02T20:00:32Z

Ah OK, finally discovered that CI errors are caused by

rpm-ostree/tests/common/libvm.sh

Line 102 in 15e945c

vm_cmd systemctl mask --now systemd-logind

, but we need systemd-logind.service to be enabled since we need to call systemd-logind's D-Bus interface's ListInhibitors method.

jlebon · 2021-06-02T20:44:30Z

Ah OK, finally discovered that CI errors are caused by

rpm-ostree/tests/common/libvm.sh

Line 102 in 15e945c

vm_cmd systemctl mask --now systemd-logind

, but we need systemd-logind.service to be enabled since we need to call systemd-logind's D-Bus interface's ListInhibitors method.

Ahh yes, this dates from a while ago now. Feel free to drop that and let's see if that bug is fixed now!

kelvinfan001 · 2021-06-02T21:15:06Z

Sample error output when trying to reboot with inhibitor locks present

Common case:

$ sudo rpm-ostree finalize-deployment xyz
error: Reboot blocked by a systemd inhibitor lock in `block` mode
Held by: MyProcess
Reason: MyProcess is currently burning a CD

Multiple block mode locks:

$ sudo rpm-ostree finalize-deployment xyz
error: Reboot blocked by a systemd inhibitor lock in `block` mode
Held by: MyProcess
Reason: MyProcess is currently burning a CD
and 1 other blocking inhibitor lock(s)
Use `systemd-inhibit --list` to see details

jlebon · 2021-06-03T21:07:26Z

src/daemon/rpmostreed-transaction-types.cxx

@@ -2582,6 +2598,10 @@ finalize_deployment_transaction_execute (RpmostreedTransaction *transaction,
  OstreeSysroot *sysroot = rpmostreed_transaction_get_sysroot (transaction);
  OstreeRepo *repo = ostree_sysroot_repo (sysroot);

+  // Check for inhibitor locks before unlocking staged deployment.
+  if (!check_sd_inhibitor_locks (cancellable, error))


We're doing this twice in this function?

ah oops. I had this for ease of manual testing and forgot to remove it :(

jlebon · 2021-06-03T21:09:32Z

src/daemon/rpmostreed-utils.cxx

+  if (!inhibitors_array_tuple)
+    return glnx_prefix_error (error, "Checking systemd inhibitor locks");
+  else if (g_variant_n_children (inhibitors_array_tuple) < 1)
+    return glnx_throw (error, "LoadInhibitors returned empty tuple");


Suggested change

return glnx_throw (error, "LoadInhibitors returned empty tuple");

return glnx_throw (error, "ListInhibitors returned empty tuple");

jlebon · 2021-06-03T21:11:11Z

src/daemon/rpmostreed-utils.cxx

+                          GError         **error)
+{
+  GDBusConnection *connection = rpmostreed_daemon_connection ();
+  g_autoptr(GVariant) inhibitors_array_tuple = 


Maybe a link around here to the systemd D-Bus API docs for this?

jlebon · 2021-06-03T21:13:11Z

src/daemon/rpmostreed-utils.cxx

+  while (g_variant_iter_loop (&viter, "(ssssuu)", &what, &who, &why, &mode, NULL, NULL))
+    {
+      // Only consider shutdown inhibitors in `block` mode.
+      if (strstr (what, "shutdown") && strcmp (mode, "block") == 0)


Minor/optional: could also do

Suggested change

if (strstr (what, "shutdown") && strcmp (mode, "block") == 0)

if (strstr (what, "shutdown") && g_str_equal (mode, "block"))

jlebon · 2021-06-03T21:13:55Z

src/daemon/rpmostreed-utils.cxx

+  char *what;
+  char *who;
+  char *why;
+  char *mode;


Probably better to initialize all those to NULL in case it triggers static analyzers.

jlebon · 2021-06-03T21:16:09Z

src/daemon/rpmostreed-utils.cxx

+  while (g_variant_iter_loop (&viter, "(ssssuu)", &what, &who, &why, &mode, NULL, NULL))
+    {
+      // Only consider shutdown inhibitors in `block` mode.
+      if (strstr (what, "shutdown") && strcmp (mode, "block") == 0)


Let's add braces here to avoid nesting single statements like this?

jlebon · 2021-06-03T21:20:22Z

src/daemon/rpmostreed-utils.cxx

+    {
+      // Only consider shutdown inhibitors in `block` mode.
+      if (strstr (what, "shutdown") && strcmp (mode, "block") == 0)
+        if (++num_block_inhibitors == 1)


Heh prefix increment, don't see those often nowadays. Combined with the previous comment to make it a block, I'd suggest just splitting that out to be easier to read/harder to mess up, e.g.:

{ if (num_block_inhibitors == 0) ... num_block_inhibitors++; }

?

Haha, yeah I tried pretty hard to not use braces there.
I thought it'd look simpler to not have those two extra lines.

jlebon · 2021-06-03T21:22:55Z

tests/common/libvm.sh

-  # XXX: hack around https://github.com/systemd/systemd/issues/14328
-  vm_cmd systemctl mask --now systemd-logind
+  # # XXX: hack around https://github.com/systemd/systemd/issues/14328
+  # vm_cmd systemctl mask --now systemd-logind


Looks like this works now then? Feel free to just delete these lines entirely.

Oh man, my bad. I think I've been debugging these bits of code for so long my eyes just got fatigued, leading to the bunch of errors 😿

jlebon · 2021-06-03T21:23:55Z

tests/vmcheck/test-misc-2.sh

+assert_file_has_content err.txt 'Reboot blocked'
+vm_cmd test -f /run/ostree/staged-deployment-locked # Make sure that staged deployment is still locked.
+vm_cmd touch /run/wakeup
+sleep 0.1 # Wait a bit for the process holding the lock to exit.


Should we just bump it to 1s to be tolerant of e.g. busy CI systems?

Today, non-interactive processes, including rpm-ostree, are not inhibited by systemd inhibitor locks. systemd 248 added the `--check-inhibitors` option to `systemctl` commands, which can be used to make non-interactive proccesses check for shutdown/sleep inhibitors. However, it also checks for `delay` mode inhibitors, which isn't what we want. Replicate this logic prior to reboots, but ignore `delay` mode inhibitors.

cgwalters · 2021-06-04T00:59:34Z

src/daemon/rpmostreed-utils.cxx

+      if (strstr (what, "shutdown") && g_str_equal (mode, "block"))
+        {
+          num_block_inhibitors++;
+          if (num_block_inhibitors == 1)


This is fine as is, though I think we could simplify the logic by doing const guint num_block_inhibitors = g_variant_n_children (inhibitors_array); and dispatch on that. But fine as is!

kelvinfan001 force-pushed the pr/check-inhibitors branch from 654f797 to ce553a0 Compare May 27, 2021 22:31

kelvinfan001 commented May 28, 2021

View reviewed changes

src/daemon/rpmostreed-daemon.cxx Show resolved Hide resolved

kelvinfan001 commented May 28, 2021

View reviewed changes

src/daemon/rpmostreed-utils.cxx Show resolved Hide resolved

jlebon reviewed May 31, 2021

View reviewed changes

cgwalters reviewed Jun 2, 2021

View reviewed changes

src/daemon/rpmostreed-transaction-types.cxx Outdated Show resolved Hide resolved

src/daemon/rpmostreed-utils.cxx Outdated Show resolved Hide resolved

src/daemon/rpmostreed-utils.cxx Outdated Show resolved Hide resolved

kelvinfan001 force-pushed the pr/check-inhibitors branch from ce553a0 to fb7ae9a Compare June 2, 2021 21:13

kelvinfan001 force-pushed the pr/check-inhibitors branch 3 times, most recently from b7c682b to 8806593 Compare June 3, 2021 18:37

jlebon reviewed Jun 3, 2021

View reviewed changes

kelvinfan001 force-pushed the pr/check-inhibitors branch from 8806593 to 5249272 Compare June 3, 2021 21:54

kelvinfan001 force-pushed the pr/check-inhibitors branch from 5249272 to a7e7095 Compare June 3, 2021 21:57

cgwalters approved these changes Jun 4, 2021

View reviewed changes

cgwalters merged commit 6a483dc into coreos:main Jun 4, 2021

kelvinfan001 mentioned this pull request Jun 4, 2021

Investigate Zincati's compatibility with systemd inhibitor locks coreos/zincati#562

Closed

cgwalters mentioned this pull request Nov 8, 2023

systemd: New bootc-fetch-apply-updates.{timer,service} containers/bootc#179

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon: Respect systemd inhibitor locks #2862

daemon: Respect systemd inhibitor locks #2862

kelvinfan001 commented May 27, 2021 •

edited

Loading

jlebon left a comment •

edited

Loading

kelvinfan001 commented May 31, 2021 •

edited

Loading

jlebon commented May 31, 2021

cgwalters commented Jun 1, 2021

cgwalters commented Jun 1, 2021

kelvinfan001 commented Jun 1, 2021

cgwalters commented Jun 1, 2021

jlebon commented Jun 1, 2021

jlebon commented Jun 1, 2021 •

edited

Loading

cgwalters commented Jun 1, 2021 •

edited

Loading

cgwalters commented Jun 1, 2021

jlebon commented Jun 1, 2021

cgwalters commented Jun 2, 2021

cgwalters commented Jun 2, 2021

kelvinfan001 commented Jun 2, 2021

jlebon commented Jun 2, 2021

kelvinfan001 commented Jun 2, 2021

jlebon Jun 3, 2021

kelvinfan001 Jun 3, 2021

jlebon Jun 3, 2021

jlebon Jun 3, 2021

jlebon Jun 3, 2021

jlebon Jun 3, 2021

jlebon Jun 3, 2021

jlebon Jun 3, 2021

kelvinfan001 Jun 3, 2021

jlebon Jun 3, 2021

kelvinfan001 Jun 3, 2021

jlebon Jun 3, 2021

cgwalters Jun 4, 2021

	return glnx_throw (error, "LoadInhibitors returned empty tuple");
	return glnx_throw (error, "ListInhibitors returned empty tuple");

	if (strstr (what, "shutdown") && strcmp (mode, "block") == 0)
	if (strstr (what, "shutdown") && g_str_equal (mode, "block"))

daemon: Respect systemd inhibitor locks #2862

daemon: Respect systemd inhibitor locks #2862

Conversation

kelvinfan001 commented May 27, 2021 • edited Loading

jlebon left a comment • edited Loading

Choose a reason for hiding this comment

kelvinfan001 commented May 31, 2021 • edited Loading

jlebon commented May 31, 2021

cgwalters commented Jun 1, 2021

cgwalters commented Jun 1, 2021

kelvinfan001 commented Jun 1, 2021

cgwalters commented Jun 1, 2021

jlebon commented Jun 1, 2021

jlebon commented Jun 1, 2021 • edited Loading

cgwalters commented Jun 1, 2021 • edited Loading

cgwalters commented Jun 1, 2021

jlebon commented Jun 1, 2021

cgwalters commented Jun 2, 2021

cgwalters commented Jun 2, 2021

kelvinfan001 commented Jun 2, 2021

jlebon commented Jun 2, 2021

kelvinfan001 commented Jun 2, 2021

Sample error output when trying to reboot with inhibitor locks present

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelvinfan001 commented May 27, 2021 •

edited

Loading

jlebon left a comment •

edited

Loading

kelvinfan001 commented May 31, 2021 •

edited

Loading

jlebon commented Jun 1, 2021 •

edited

Loading

cgwalters commented Jun 1, 2021 •

edited

Loading