overlay.d: Update ignition.firstboot services for soft-reboots #3536
Conversation
jmarrero
force-pushed
the
prepare-soft-reboot
branch
from
b92652c to
f13b100
Compare
overlay.d/05core/usr/lib/systemd/system/coreos-ignition-firstboot-complete.service
Outdated
Show resolved
Hide resolved
jmarrero
force-pushed
the
prepare-soft-reboot
branch
2 times, most recently
from
31079c6 to
787b3dc
Compare
/etc/machine-id is there: However, I don't see anything in the code or service that mentions /etc/machine-id... Not sure if you mean that it should not run because /etc/machine-id means that is not longer the first boot. |
There was a problem hiding this comment.
One comment, but looks fine overall. That said, one could imagine other systemd units that use ignition.firstboot out there instead of ConditionFirstBoot that will also hit this.
For this specific unit, I think the current conditionals here are more appropriate than ConditionFirstBoot=, but everything else should probably use that.
OK and we have one actually in this very repo:
. Can you adapt that one?
overlay.d/05core/usr/lib/systemd/system/coreos-ignition-firstboot-complete.service
Show resolved
Hide resolved
jmarrero
force-pushed
the
prepare-soft-reboot
branch
2 times, most recently
from
89f8cfb to
caa29cb
Compare
jmarrero
changed the title
| # This condition is required for cases where a soft-reboot is issued on the | ||
| # firstboot. Soft reboot do not change the kernel or its command-line arguments, | ||
| # which would cause this service to fail after the soft-reboot completes. | ||
| ConditionPathExists=/boot/ignition.firstboot |
There was a problem hiding this comment.
Not quite. What I mean is that this unit should use ConditionFirstBoot=true instead of ConditionKernelCommandLine= + ConditionPathExists=.
There was a problem hiding this comment.
Hum. Looking there are some other uses of ConditionFirstBoot=true but not many
$ podman run --rm -ti quay.io/fedora/fedora-coreos:stable grep -r ConditionFirstBoot /usr/lib/systemd/system
/usr/lib/systemd/system/systemd-homed-firstboot.service:ConditionFirstBoot=yes
/usr/lib/systemd/system/coreos-update-ca-trust.service:ConditionFirstBoot=true
/usr/lib/systemd/system/coreos-update-ca-trust.service:# All services which use ConditionFirstBoot=yes should use
/usr/lib/systemd/system/ignition-delete-config.service:ConditionFirstBoot=true
/usr/lib/systemd/system/first-boot-complete.target:ConditionFirstBoot=yes
/usr/lib/systemd/system/zincati.service:# created so that we don't cause ConditionFirstBoot=true units to run twice
/usr/lib/systemd/system/systemd-firstboot.service:ConditionFirstBoot=yes
/usr/lib/systemd/system/afterburn-firstboot-checkin.service:ConditionFirstBoot=yes
$
I think I agree with you it'd be better to do so, however a general issue with ConditionFirstBoot is it doesn't handle the case where the system is interrupted (poweroff/crash) during first boot. Those services will then later fail to reconcile.
Dunno that it matters though in practice.
There was a problem hiding this comment.
A lot of background on that topic. See discussions in e.g. systemd/systemd#4511. This is why we do this here for example:
There was a problem hiding this comment.
Which, presumably we should also do for coreos-check-ssh-keys.service, but yeah I'm not too worried about it.
coreos/coreos-assembler#4119 surfaced that this services using ConditionKernelCommandLine=ignition.firstboot would fail on soft-reboots, it's non fatal but would make the Kola tests fail. Co-authored-by: Jonathan Lebon <[email protected]> Co-authored-by: Colin Walters <[email protected]>
jmarrero
force-pushed
the
prepare-soft-reboot
branch
from
caa29cb to
bc3c368
Compare
coreos/coreos-assembler#4119 surfaced that this service would fail on soft-reboots, it's non fatal but would make the Kola tests fail. It looks like originally this was set to fail instead of risking not running it:
fedora-coreos-config/overlay.d/05core/usr/libexec/coreos-ignition-firstboot-complete
Line 14 in 20feb17