packaging: add a systemd unit to run every boot #716

jbtrystram · 2024-09-03T07:39:40Z

This enables bootloader updates automatically on boot. Note that the service is intentionally not enabled by default, it should be up to the distribution to add a systemd preset if auto-update for the bootloader is desired.

Right now RAID setups are not supported but see [1] for an example in coreos.

[1] coreos/fedora-coreos-config#3042

travier

LGTM. Thanks!

travier · 2024-09-03T08:19:12Z

I'll do some manual testing.

This enables bootloader updates automatically on boot. Note that the service is intentionally not enabled by default, it should be up to the distribution to add a systemd preset if auto-update for the bootloader is desired. Right now RAID setups are not supported but see [1] for an example in coreos. [1] coreos/fedora-coreos-config#3042

jbtrystram · 2024-09-03T15:16:50Z

I built a RPM and tested in a fcos build.
After enabling the service and rebooting :

[core@cosa-devsh ~]$ systemctl status bootupctl-update.service 
● bootupctl-update.service - Update Bootloader on boot
     Loaded: loaded (/usr/lib/systemd/system/bootupctl-update.service; enabled; preset: disabled)
    Drop-In: /usr/lib/systemd/system/service.d
             └─10-timeout-abort.conf
     Active: active (exited) since Tue 2024-09-03 15:15:56 UTC; 23s ago
       Docs: https://github.com/coreos/bootupd
    Process: 1099 ExecStart=/usr/bin/bootupctl update (code=exited, status=0/SUCCESS)
   Main PID: 1099 (code=exited, status=0/SUCCESS)
        CPU: 9ms

Sep 03 15:15:56 localhost systemd[1]: Starting bootupctl-update.service - Update Bootloader on boot...
Sep 03 15:15:56 localhost bootupctl[1099]: No update available for any component.
Sep 03 15:15:56 localhost systemd[1]: Finished bootupctl-update.service - Update Bootloader on boot.

travier · 2024-09-03T18:49:28Z

contrib/packaging/bootupctl-update.service

+[Unit]
+Description=Update Bootloader on boot
+Documentation=https://github.com/coreos/bootupd
+ConditionPathExists=/dev/disk/by-label/EFI-SYSTEM


We should add as well:

ConditionPathExists=|/dev/disk/by-partlabel/EFI\x20System\x20Partition

There's already also ConditionFirmware=uefi

That said we need to support BIOS too...so probably the best would be e.g.

RequiresMountsFor=/boot ConditionPathExists=/boot/bootupd-state.json

ConditionPathExists=/boot/bootupd-state.json
wouldn't that prevent it to run on atomic desktops as bootupd was not enabled until now ?

Yes, but that's by design in bootupd today; there is bootupctl adopt-and-update but that's a distinct thing intended only for explicit invocation in tested scenarios, distinct from a "bootupd -> bootupd" update.

I am OK with ConditionFirmware=uefi, can remove it until we add adopt-and-update for BIOS later.
One minor suggestion, should we add ConditionKernelCommandLine=ostree to skip this for live boots like coreos/rpm-ostree#4944 ?

i think the overall agreement was to close this PR and carry this in the distribution

travier · 2024-09-03T18:50:41Z

contrib/packaging/bootupctl-update.service

+[Unit]
+Description=Update Bootloader on boot
+Documentation=https://github.com/coreos/bootupd
+ConditionPathExists=/dev/disk/by-label/EFI-SYSTEM


Suggested change

ConditionPathExists=/dev/disk/by-label/EFI-SYSTEM

ConditionPathExists=|/dev/disk/by-partlabel/EFI-SYSTEM

As that's what's used in https://github.com/coreos/bootupd/blob/main/src/efi.rs#L39

travier · 2024-09-03T18:54:34Z

needs a | in the condition: https://www.freedesktop.org/software/systemd/man/latest/systemd.unit.html#Conditions%20and%20Asserts

travier · 2024-09-03T18:56:25Z

With those changes, that works for me on Silverblue

cgwalters · 2024-09-03T19:46:19Z

I'm not opposed to this but the reason I didn't do this until now is basically I think what we really want is more like #108 which means the thing doing the OS updates drives bootupd itself by default.

cgwalters · 2024-09-03T20:23:49Z

Makefile

@@ -41,6 +41,9 @@ install-grub-static:
 	install -m 644 -D -t ${DESTDIR}$(PREFIX)/lib/bootupd/grub2-static src/grub2/*.cfg
 	install -m 755 -d ${DESTDIR}$(PREFIX)/lib/bootupd/grub2-static/configs.d

+install-systemd-unit:
+	install -m 644 -D -t "${DESTDIR}$(PREFIX)/lib/systemd/system/" contrib/packaging/bootupctl-update.service


An overall challenge here is that we'll have come full circle since #663 ...and then e.g. people maintaining SELinux policy are still going to come by and say "oh hey there's a systemd service, we need to write a policy for it...".

With this combined I would probably say that instead of this we should implement bootupd support into rpm-ostree and bootc.

(This also relates to #432 )

cgwalters

Marking as requested-changes per #716 (comment) but not a hard blocker for me, if you or someone wants to do this in the interim I'm OK, but it does basically clash and I think longer term it'll be more understandable for users; we'd have to maintain this newly introduced service unit here ~forever.

travier · 2024-09-03T21:29:27Z

I would really like to get this into Atomic Desktops for F41 as we desperately need it there. So I can carry it there in the mean time until we get a cleaner solution here.

Agree that #108 feels like an improvement but I don't think it should hold us from doing this one in the mean time?

cgwalters · 2024-09-03T21:55:53Z

I would really like to get this into Atomic Desktops for F41 as we desperately need it there.

Isn't what is desperately needed more something like automatic blocking of OS updates when the bootloader state is too old? I think we could turn around an implementation of that in a few days.

cgwalters · 2024-09-03T21:59:49Z

One thing this is also basically sweeping under the rug of an on-by-default systemd unit is the rationale stated in https://github.com/coreos/bootupd?tab=readme-ov-file#questions-and-answers from the very start of this project. Yes the problem stinks, and the rename_exchage work probably makes it better, but still...the semantics here are going to be that during the middle of OS bootup we do this (potentially) unsafe operation with zero warning about that unsafety.

travier · 2024-09-04T12:55:05Z

An overall challenge here is that we'll have come full circle since #663 ...and then e.g. people maintaining SELinux policy are still going to come by and say "oh hey there's a systemd service, we need to write a policy for it...".

The initial issue was that bootupd looked-like a daemon program with a service unit. At this point, I don't know if we'll be able to go back to no policy module.

Isn't what is desperately needed more something like automatic blocking of OS updates when the bootloader state is too old? I think we could turn around an implementation of that in a few days.

If we did that, we would have to surface that somehow to the user in the interface. I find it very unlikely that we'll have something ready for Fedora 41 for all Atomic Desktops, even just GNOME & KDE.

fwupd already has a mechanism to hold updates if the system is not on AC or doesn't have enough battery so that could be a good place to look at as well as that is already integrated in Plasma Discover & GNOME Software.

We also don't have barrier releases either like Fedora CoreOS so we can't do one shot updates before major releases.

One thing this is also basically sweeping under the rug of an on-by-default systemd unit is the rationale stated in coreos/bootupd#questions-and-answers from the very start of this project. Yes the problem stinks, and the rename_exchage work probably makes it better, but still...the semantics here are going to be that during the middle of OS bootup we do this (potentially) unsafe operation with zero warning about that unsafety.

I think that the residual unsafety [1] [2] remaining after the work on #669 is more theoretical than practical.

So far, we have had a lot of issues due to the fact that we are not updating the bootloaders and those impacted a very significant chunk of our users. I don't expect this kind of widespread trouble from this safety gap. We are already far safer than what classic package mode systems do during updates.

With this combined I would probably say that instead of this we should implement bootupd support into rpm-ostree and bootc.

we'd have to maintain this newly introduced service unit here ~forever.

Why do you thing we would have to support it forever? I think it should be fine to migrate systems to a better way once we have it ready.

cgwalters · 2024-09-04T15:26:37Z

fwupd already has a mechanism to hold updates if the system is not on AC or doesn't have enough battery so that could be a good place to look at as well as that is already integrated in Plasma Discover & GNOME Software.

I like this idea, it makes sense to me to encourage grouping of these two things, even if technically they are distinct.

cgwalters · 2024-09-04T15:47:53Z

Anyways to your larger point...fair. I am OK to merge but let's at least get https://github.com/coreos/bootupd/pull/716/files#r1742591590 changed?

travier · 2024-09-04T16:04:01Z

contrib/packaging/bootupctl-update.service

@@ -0,0 +1,13 @@
+[Unit]


Let's name this unit bootloader-update.service as its what will be user visible and users ideally should not have to know about bootupd.

travier · 2024-09-04T16:04:15Z

contrib/packaging/bootupctl-update.service

@@ -0,0 +1,13 @@
+[Unit]
+Description=Update Bootloader on boot


Suggested change

Description=Update Bootloader on boot

Description=Update bootloader on boot

travier · 2024-09-04T16:05:51Z

Anyways to your larger point...fair. I am OK to merge but let's at least get #716 (files) changed?

I think we should keep this check as broad as possible. We can not require ConditionPathExists=/boot/bootupd-state.json as we explicitly want to also adopt and update systems that are not yet using bootupd.
I'm installing a BIOS system again to check the partitions.

jbtrystram · 2024-09-04T16:26:42Z

I think we should keep this check as broad as possible. We can not require ConditionPathExists=/boot/bootupd-state.json as we explicitly want to also adopt and update systems that are not yet using bootupd. I'm installing a BIOS system again to check the partitions.

So i guess we should add a script and do adopt-and-update when applicable ? (no bootupd-state.json)

travier · 2024-09-04T16:39:21Z

I can't find a way to select BIOS systems in https://www.freedesktop.org/software/systemd/man/latest/systemd.unit.html#ConditionFirmware=.

And an installed Silverblue system does not have a distinct partition to select on, so we can't match that.

travier · 2024-09-04T17:13:27Z

OK, it looks like this requires different trade-offs and conditions check depending on which system is installed right now so it might be better downstream. I've made: https://pagure.io/workstation-ostree-config/pull-request/571

cgwalters · 2024-09-04T19:00:47Z

I'm OK with that too, though the tradeoff is it's just going to get copy-pasted again by IoT...

jlebon · 2024-09-04T19:29:17Z

Isn't what is desperately needed more something like automatic blocking of OS updates when the bootloader state is too old? I think we could turn around an implementation of that in a few days.

If we did that, we would have to surface that somehow to the user in the interface.

Hmm, I think if we did that in rpm-ostree in a way that literally prevented Deploy() from working, that should surface, no? (As an rpm-ostree error?) Obviously not pretty, but wouldn't require any changes to g-s/KDE.

jlebon · 2024-09-04T19:31:42Z

Basically, something like:

# rpm-ostreed.conf
[Daemon]
RequireUpdatedBootloader=true

When true (default is false), rpm-ostree does bootupctl status --json, checks for updatable if it's at-latest-version, and if not, error out with something like:

error: RequireUpdatedBootloader is enabled and bootloader is out of date. Please run bootupctl update

?

I guess the question is whether that error message would percolate up into the UIs in full so the user understands what they need to do.

jlebon · 2024-09-05T14:04:47Z

Hmm, I think if we did that in rpm-ostree in a way that literally prevented Deploy() from working, that should surface, no? (As an rpm-ostree error?)

Chatting with @travier, it doesn't seem to be the case. See e.g. https://gitlab.gnome.org/GNOME/gnome-software/-/issues/2000.

Obviously, there's nothing that prevents us from adding it now, and then improving it during the f41 cycle.

travier · 2024-09-05T18:03:08Z

Looks like we never implemented adoption for BIOS systems so we will have to make this EFI only 😞

jbtrystram · 2024-09-11T12:22:22Z

I feel the general agreement here is that this would be better carried in the distribution side. I'll close this

HuijingHei · 2024-09-11T12:23:12Z

One thing I am concern is if booting with BIOS, is it necessary to also update UEFI (on x86_64)? Can we update the current boot firmware, like install, eg 5ee0483

But first thing, will add adoption for BIOS.

travier approved these changes Sep 3, 2024

View reviewed changes

jbtrystram force-pushed the systemd-unit branch from e86b791 to 3903dde Compare September 3, 2024 14:25

jbtrystram force-pushed the systemd-unit branch from 3903dde to 43bcccb Compare September 3, 2024 15:17

travier reviewed Sep 3, 2024

View reviewed changes

cgwalters reviewed Sep 3, 2024

View reviewed changes

cgwalters requested changes Sep 3, 2024

View reviewed changes

travier reviewed Sep 4, 2024

View reviewed changes

travier self-requested a review September 4, 2024 17:06

jbtrystram closed this Sep 11, 2024

	ConditionPathExists=/dev/disk/by-label/EFI-SYSTEM
	ConditionPathExists=\|/dev/disk/by-partlabel/EFI-SYSTEM

	Description=Update Bootloader on boot
	Description=Update bootloader on boot

packaging: add a systemd unit to run every boot #716

packaging: add a systemd unit to run every boot #716

Conversation

jbtrystram commented Sep 3, 2024

travier left a comment

Choose a reason for hiding this comment

travier commented Sep 3, 2024

jbtrystram commented Sep 3, 2024

travier Sep 3, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travier Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

travier commented Sep 3, 2024

travier commented Sep 3, 2024

cgwalters commented Sep 3, 2024

Choose a reason for hiding this comment

cgwalters left a comment

Choose a reason for hiding this comment

travier commented Sep 3, 2024

cgwalters commented Sep 3, 2024

cgwalters commented Sep 3, 2024

travier commented Sep 4, 2024

cgwalters commented Sep 4, 2024

cgwalters commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travier commented Sep 4, 2024

jbtrystram commented Sep 4, 2024

travier commented Sep 4, 2024 • edited Loading

travier commented Sep 4, 2024

cgwalters commented Sep 4, 2024

jlebon commented Sep 4, 2024

jlebon commented Sep 4, 2024 • edited Loading

jlebon commented Sep 5, 2024

travier commented Sep 5, 2024

jbtrystram commented Sep 11, 2024

HuijingHei commented Sep 11, 2024

travier Sep 3, 2024 •

edited

Loading

travier Sep 3, 2024 •

edited

Loading

travier commented Sep 4, 2024 •

edited

Loading

jlebon commented Sep 4, 2024 •

edited

Loading