-
Notifications
You must be signed in to change notification settings - Fork 27
dracut/30ignition: Reboot the system after ignition if kargs.d is present #86
Conversation
5d6eb52
to
75274e5
Compare
dracut/30ignition/ignition-reboot.sh
Outdated
reboot_if_kargs_dir_exists() { | ||
if [ -d /etc/ostree/kargs.d ] | ||
then | ||
ostree admin deploy fedora/x86_64/coreos/testing-devel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't have code that is specific to a hardcoded ref. Though this of course gets into a bit of a mess as all the nice high level logic around kargs is most recently in rpm-ostree, specifically rpm-ostree kargs
which handles all of this.
Which isn't designed to be run from the initramfs today, although for that matter neither is ostree.
Are you sure this script would work? It looks like you're using /etc/ostree/kargs.d
but that's the copy in the initramfs, it'd need to be /sysroot/etc/ostree/kargs.d
here right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't have code that is specific to a hardcoded ref.
although for that matter neither is ostree.
Can I use ostree refs --repo /sysroot/ostree/repo
to retrieve the REFSPEC
or ostree
should not be used at all?
Are you sure this script would work?
I'm not sure, since I haven't tested on this.. will be testing tomorrow morning
It looks like you're using /etc/ostree/kargs.d but that's the copy in the initramfs, it'd need to be /sysroot/etc/ostree/kargs.d
I have a blurred understanding of which part is in initramfs and which part is in sysroot before. Now I think I have a sense that before mounting sysroot, all of the files resides in initramfs. Thank you for pointing that out !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note also you'll want to test this on top of ostreedev/ostree#1836. See some tips from @cgwalters and @rfairley on doing that in ostreedev/ostree#479.
Can I use
ostree refs --repo /sysroot/ostree/repo
to retrieve theREFSPEC
orostree
should not be used at all?
Sure. There shouldn't be other refs or deployments in there on first boot normally. Though another approach that would survive that would be to pick up the ostree=
karg (see e.g. cmdline_arg
in the generator), then pickup refspec
from $(realpath $ostree_arg).origin
. Not necessary as a first pass though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 - I left some instructions with those tips collected here: ostreedev/ostree#1836 (comment)
To test with Ignition, instead of editing fedora-coreos-base.yaml
and adding kargs snippets using the add-files
field, you can add those files under /etc/ostree/kargs.d
through an Ignition config (haven't tried this, but should work!).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using ostree refs --repo /sysroot/ostree/repo
for now, might change to $(realpath $ostree_arg).origin
in the future
@@ -0,0 +1,16 @@ | |||
[Unit] | |||
Description=Ignition (reboot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd avoid using the Ignition (X)
for things that aren't Ignition stages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to Description=Reboot after Ignition to apply kargs
# Make sure ExecStart= runs before we switch root | ||
Before=initrd-switch-root.target | ||
|
||
# Make sure root filesystem is mounted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: ignition-mount
does not actually mount the rootfs; it mounts all other filesystems defined by the Ignition config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining!
One followup question is that, is it initrd-root-fs.target
that mounts the rootfs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check out bootup(7)
and systemd.special(7)
. Those are super helpful for understanding the boot process!
[Service] | ||
Type=oneshot | ||
RemainAfterExit=yes | ||
EnvironmentFile=/run/ignition.env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this, so I left it untouched, comparing to other service files..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The env file is useful if you have code dependent on a specific PLATFORM_ID
. Otherwise, we don't strictly need it (but it doesn't hurt either of course!).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to drop it if we don't need it. Less to confuse people.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, dropping this line
Before=ignition-complete.target | ||
|
||
# Make sure ExecStart= runs before we switch root | ||
Before=initrd-switch-root.target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we shouldn't need this. ignition-complete.target
is ordered before the main initrd.target
, which is reached well before initrd-switch-root.target
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropping this line..
# Make sure ExecStart= runs before we switch root | ||
Before=initrd-switch-root.target | ||
|
||
# Make sure root filesystem is mounted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check out bootup(7)
and systemd.special(7)
. Those are super helpful for understanding the boot process!
dracut/30ignition/module-setup.sh
Outdated
@@ -53,6 +56,7 @@ install() { | |||
install_ignition_unit ignition-mount.service | |||
install_ignition_unit ignition-files.service | |||
install_ignition_unit ignition-remount-sysroot.service | |||
install_ignition_unit ignition-reboot.service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe let's call it ignition-apply-kargs.service
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to drop ignition
from the name entirely. It's not running Ignition at all. It would be good to keep reboot
in the unit name as well to make it clear what it's doing. Just spitballing apply-kargs-and-reboot.service
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of unit names that don't contain any hint of where it came from. The ignition
in the unit name is more to namespace it in that sense (maybe ignition-dracut-...
would work too). Of course, we could (and probably should) also add e.g. Documentation=https://github.com/coreos/ignition-dracut
or something, which I don't think we do today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the name to ignition-apply-kargs.service
and ignition-apply-kargs.sh
, and added Reboot after Ignition to apply kargs
to description.
@@ -0,0 +1,16 @@ | |||
[Unit] | |||
Description=Ignition (reboot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could probably do ConditionDirectoryNotEmpty=/sysroot/etc/ostree/kargs.d
here. systemd checks the condition at the time the unit would be started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about that and I'm not sure. We may want more complicated checks in the future that the systemd conditionals can't support. That being said, I suppose it's fine for now; we can always change it to something more complex if it comes to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking kargs.d
inside ignition-apply-kargs.sh
for now, might change to ConditionDirectoryNotEmpty=/sysroot/etc/ostree/kargs.d
after testing.
Before=initrd-switch-root.target | ||
|
||
# Make sure root filesystem is mounted | ||
After=ignition-mount.service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think we need After=ignition-files.service
, so that user-configs can also drop kargs files and have them immediately take effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that makes sense!
[Service] | ||
Type=oneshot | ||
RemainAfterExit=yes | ||
EnvironmentFile=/run/ignition.env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The env file is useful if you have code dependent on a specific PLATFORM_ID
. Otherwise, we don't strictly need it (but it doesn't hurt either of course!).
dracut/30ignition/ignition-reboot.sh
Outdated
reboot_if_kargs_dir_exists() { | ||
if [ -d /etc/ostree/kargs.d ] | ||
then | ||
ostree admin deploy fedora/x86_64/coreos/testing-devel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note also you'll want to test this on top of ostreedev/ostree#1836. See some tips from @cgwalters and @rfairley on doing that in ostreedev/ostree#479.
Can I use
ostree refs --repo /sysroot/ostree/repo
to retrieve theREFSPEC
orostree
should not be used at all?
Sure. There shouldn't be other refs or deployments in there on first boot normally. Though another approach that would survive that would be to pick up the ostree=
karg (see e.g. cmdline_arg
in the generator), then pickup refspec
from $(realpath $ostree_arg).origin
. Not necessary as a first pass though!
@jlebon Thank you for the review and for the pointers to the |
56034b8
to
2fac5c8
Compare
Update:
Related issue: coreos/fedora-coreos-tracker#34 |
@zonggen that output makes it look like |
Right, that also leads to the point that at the present time, there's a tiny bit of ostree in the initramfs but the rest of the functionality (upgrades, changing kargs, or in general making new deployments) hasn't been tested in the initramfs. It might work, it might require libostree changes. |
I assume you mean when running implicitly referring to the running system, right? There's no reason it should behave differently when running using |
Update:
local REFSPEC=( $(ostree refs --repo /sysroot/ostree/repo) )
# retrieve 'ostree=' karg from '/proc/cmdline'
cmdline=( $(</proc/cmdline) )
cmdline_arg() {
local name="ostree" value=""
for arg in "${cmdline[@]}"; do
if [[ "${arg%%=*}" == "${name}" ]]; then
value="${arg#*=}"
break
fi
done
echo "${value}"
}
local REFSPEC="$(realpath $(cmdline_arg)).origin" which still failed with similar It seems that both directories only exist after |
Wont this run Ignition on second boot as well since we don't remove the ignition-firstboot file? |
Added checking on |
is the |
Note that the Might be easier to play around with these paths in a fully booted FCOS machine first so you see how these paths all fit together. It's definitely not straightforward on first approach. :)
Hmm, that's really odd. Note that from the emergency shell, systemd will unmount everything (including |
Trying both remounting and printing debug message 👍 |
Added the |
Wrote this in a haste, and it's not quite right. So for posterity, let me expand on this: So something like:
Right, Did you try just e.g. |
Yes, just tried it. Seems like everything is mounted correctly, also
which looks correct.. |
cf4482b
to
3f5ab24
Compare
Running |
Update:
|
local REFSPEC=( $(ostree refs --repo /sysroot/ostree/repo) ) | ||
if [ -d /sysroot/etc/ostree/kargs.d ] | ||
then | ||
ostree admin deploy REFSPEC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You definitely need a --sysroot=/sysroot
argument here.
And also it should be $REFSPEC
and not REFSPEC
literally.
Also there is ostree admin instutil set-kargs
which mutates the deployment in-place rather than creating a new one which...is probably better here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes --sysroot=/sysroot
solved the issue here : )
And also it should be $REFSPEC and not REFSPEC literally.
I only corrected this in local testing branch, should've updated pr branch too. Sorry about this.
Currently I'm testing on ostree admin instutil set-kargs
👍
As mentioned in https://github.com/coreos/ignition-dracut/issues/81#issuecomment-494888494, this change adds a service that checks for the presence of `/etc/ostree/kargs.d` and redeploys then reboots the system if it exists. Closes: #81
3f5ab24
to
857961e
Compare
Update:
REFSPEC=( $(ostree refs --repo /sysroot/ostree/repo) )
ostree admin --sysroot=/sysroot --os=fcos deploy ${REFSPEC} Failed with
kargs=$(cat /sysroot/etc/ostree/kargs.d/karg_file)
ostree admin instutil set-kargs -v --sysroot=/sysroot --replace ${kargs} Failed with
|
The problem here boils down to a lot of the libostree code just not being designed to run from the initramfs unfortunately... I think the problem in the first case is that ostree isn't expecting things to be mounted at the target path in the The second one, not sure what's going on there. |
I agree, even running |
is this something we still want to do? Does it need a new round of review? |
It is safe to close this pr since it would not work until libostree can be run inside initramfs.. |
As mentioned in https://github.com/coreos/ignition-dracut/issues/81#issuecomment-494888494,
this change adds a service that checks for the presence of
/etc/ostree/kargs.d
and redeploysthen reboots the system if it exists.
Closes: #81