Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial support for live PXE and ISO images #155

Merged
merged 7 commits into from
Sep 16, 2019
Merged

Initial support for live PXE and ISO images #155

merged 7 commits into from
Sep 16, 2019

Conversation

bgilbert
Copy link
Contributor

@bgilbert bgilbert commented Sep 5, 2019

Deferred to follow-up PRs:

  • Functioning with SELinux enabled
  • Allowing users to provide an Ignition config via a custom initrd image
  • Automatic login in the ISO image

Other known issues:

  • dracut passes the ostree path to ostree-prepare-root by bind-mounting over /proc/cmdline
  • rpm-ostreed refuses to start
  • ISO kargs are not automatically synced from the BLS configs

Fixes #115.

- Change labels
- Use short bootloader timeout
- Disable installer
- Enable Ignition, defaulting to metal platform
- Drop nomodeset karg
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good! I'd like to think a bit harder about the "API" around which live mounts are detected. I feel like e.g. /run/ostree-booted has worked well; why not have /run/ostree-live or so and use ConditionPathExists=/run/ostree-live rather than ConditionPathIsReadWrite=/?

It's funny because just last night I was working on ostreedev/ostree#1265
and while that happens to leave / as technically rw...I am not sure I really want to encode that in many systemd units permanently.

BTW...there is also https://www.freedesktop.org/software/systemd/man/systemd-volatile-root.service.html which was clearly intended for something like this, but it doesn't deal with /etc for example.

@@ -0,0 +1,4 @@
install() {
inst_script "$moddir/is-live-image.sh" \
"/usr/bin/is-live-image"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is just the initramfs but I am not a fan of "/usr/bin pollution". /usr/libexec/coreos-is-live-image ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an API with ignition-dracut, which is distro-independent, so it shouldn't be coreos-. We could use /usr/libexec but nothing else in the initrd does (except coreos-installer), and /usr/bin pollution is rampant in the initrd, so I'm inclined to leave it.

@@ -0,0 +1,9 @@
[Unit]
DefaultDependencies=false
RequiresMountsFor=/writable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we preserve /run from the initramfs? If so, let's put stuff in /run/coreos-live-writable or so?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do. I thought it'd be cleaner to have a dedicated tmpfs for user-created state. Also, this avoids exposing the overlayfs upperdir and workdir within the directory structure.

add_requires sysroot-var.mount

mkdir -p "${UNIT_DIR}/ostree-prepare-root.service.d"
cat > "${UNIT_DIR}/ostree-prepare-root.service.d/10-live.conf" <<EOF
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with this, though I'd also like to consider standardizing support for this in ostree itself at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that'd be good. This is meant as a starting point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's definitely hacky to bind mount over /proc/cmdline but is a creative way to get this working now. Can you open an RFE against ostree to support this natively and copy the URL to the issue in this dropin unit as well as a comment in the ostree-cmdline.sh script?

@@ -28,6 +28,6 @@ set timeout=1

### BEGIN /etc/grub.d/10_linux ###
menuentry 'Fedora CoreOS (Live)' --class fedora --class gnu-linux --class gnu --class os {
linux /images/vmlinuz rd.neednet=1 ip=dhcp ignition.firstboot ignition.platform.id=metal
linux /images/vmlinuz mitigations=auto,nosmt rd.neednet=1 ip=dhcp ignition.firstboot ignition.platform.id=metal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. It feels cleaner if we pulled this stuff out of an existing disk image. But OK for now.

@bgilbert
Copy link
Contributor Author

bgilbert commented Sep 5, 2019

I feel like e.g. /run/ostree-booted has worked well; why not have /run/ostree-live or so and use ConditionPathExists=/run/ostree-live rather than ConditionPathIsReadWrite=/?

I'm okay with that. ConditionPathIsReadWrite doesn't feel 💯 to me.

ignition-dracut will invoke this to determine whether to enable units
that require a boot disk.
@bgilbert
Copy link
Contributor Author

bgilbert commented Sep 7, 2019

Updated.

@lucab
Copy link
Contributor

lucab commented Sep 9, 2019

Just for clarity, which component is going to create /run/ostree-live and at what point of the build (or boot) flow?

@bgilbert
Copy link
Contributor Author

bgilbert commented Sep 9, 2019

@lucab It's currently being done at boot time, in live-generator in the initramfs.

@cgwalters
Copy link
Member

This all LGTM!

@dustymabe dustymabe self-requested a review September 9, 2019 15:18
@bgilbert
Copy link
Contributor Author

bgilbert commented Sep 9, 2019

To test:

# PXE
qemu-system-x86_64 -machine accel=kvm -m 2048 \
    -netdev user,id=eth0,hostfwd=tcp::2222-:22,hostname="live" -device virtio-net-pci,netdev=eth0 \
    -fw_cfg name=opt/com.coreos/config,file=$HOME/ssh-key.ign \
    -object rng-random,filename=/dev/urandom,id=rng0 -device virtio-rng-pci,rng=rng0 \
    -kernel fedora-coreos-30.*-live-kernel \
    -initrd fedora-coreos-30.*-live-initramfs.img \
    -append 'rd.neednet=1 ip=dhcp ignition.firstboot ignition.platform.id=qemu enforcing=0'`
# ISO
qemu-system-x86_64 -machine accel=kvm -m 2048 -cdrom fedora-coreos-30.*.iso \
    -netdev user,id=eth0,hostfwd=tcp::2222-:22,hostname="live" -device virtio-net-pci,netdev=eth0 \
    -fw_cfg name=opt/com.coreos/config,file=$HOME/ssh-key.ign \
    -object rng-random,filename=/dev/urandom,id=rng0 -device virtio-rng-pci,rng=rng0

For ISO boot, manually edit the kargs to specify ignition.platform.id=qemu enforcing=0.

@cgwalters
Copy link
Member

I'm OK merging this as is; do you think someone else needs to test before merging to master?

Where=/writable
Type=tmpfs
Options=mode=0700
EOF
Copy link

@SerialVelocity SerialVelocity Sep 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably want to remove this line before merging. Pretty sure it is leftover from when you extracted it out from a cat <<EOF

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch, thanks! Updated.

@dustymabe
Copy link
Member

I'm OK merging this as is; do you think someone else needs to test before merging to master?

i'm running through reviews/tests today

Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some initial comments. I'll probably have some more tomorrow when I finally test the artifacts. Good news is they did build locally for me!

Type=squashfs
EOF

cat >"${UNIT_DIR}/sysroot-etc.mount" <<EOF
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent

Options=lowerdir=/sysroot/etc,upperdir=/writable/etc/upper,workdir=/writable/etc/work,redirect_dir=on,index=on,xino=on
EOF

cat >"${UNIT_DIR}/sysroot-var.mount" <<EOF
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent

set -euo pipefail

case "${1:-unset}" in
start)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent these cases?

add_requires sysroot-var.mount

mkdir -p "${UNIT_DIR}/ostree-prepare-root.service.d"
cat > "${UNIT_DIR}/ostree-prepare-root.service.d/10-live.conf" <<EOF
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's definitely hacky to bind mount over /proc/cmdline but is a creative way to get this working now. Can you open an RFE against ostree to support this natively and copy the URL to the issue in this dropin unit as well as a comment in the ostree-cmdline.sh script?

Where=/boot
EOF

# Mount /boot/efi only on UEFI platforms, as well as x86_64 BIOS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a comment here on why we want this behavior on these specific cases

ln -sf "../${name}" "${wants_dir}/${name}"
}

# Don't create mount units for /boot or /boot/efi on live systems
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be nice if we add a comment here about not being able to use ConditionPathExists /run/ostree-live. You have this comment in your commit message, but would be nice to have it here.

@@ -0,0 +1 @@
mitigations=auto,nosmt rd.neednet=1 ip=dhcp ignition.firstboot ignition.platform.id=metal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regarding the mitigations=auto,nosmt (the last commit in this series) it seems like this is going to be a bit of a maintenance burden because we store that information currently in image.yaml can we pull those args from there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean just the extra-kargs? We can, but there are also args coming from other places (ignition.platform.id, console, the temporary networking args, rootflags, etc). We could read them out of the BLS config and filter out the undesired ones, but I thought that might be more trouble than it's worth. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could read them out of the BLS config and filter out the undesired ones, but I thought that might be more trouble than it's worth. Thoughts?

I like that, depending on how much trouble it really is. Basically we'd maintain a blacklist in a single place vs duplicated kernel args in various places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Can I queue that for a followup PR?

@dustymabe
Copy link
Member

it looks like the selinux issues are related to files getting labeled with the tmpfs_t type probably because they are getting fiddled with under the /writable tmpfs at some point?

If /root.squashfs exists in the initramfs, do the following:

1. Mount /root.squashfs on /sysroot
2. Locate the ostree deployment, add a corresponding ostree= argument
   to /proc/cmdline by bind-mounting over it, run
   ostree-prepare-root.service to set up the deployment, and unmount
   /proc/cmdline
3. Mount a tmpfs on /writable
4. Create needed directories in /writable
5. Create a writable overlay on /sysroot/etc, with /sysroot/etc as
   lowerdir and upper/work dirs in /writable
6. Bind-mount /writable/var to /sysroot/var
7. Run coreos-populate-var.service to populate /sysroot/var

Disable mounting of stateroot /var on live systems, since that's handled
by step 6.
There's no on-disk filesystem to resize.
Conditionalizing the units won't work here because conditions don't
affect the dependency on the underlying device unit.  Enable
boot.mount on non-PXE boots.  Enable boot-efi.mount on non-PXE boots on
platforms (x86_64 and aarch64) where we have an EFI partition, whether
the /boot/efi mountpoint exists or not.

Also make the mount units WantedBy local-fs.target, rather than
RequiredBy, to prevent boot failures if the mount fails.
We enable it in non-live builds.
@bgilbert
Copy link
Contributor Author

Updated.

@dustymabe
Copy link
Member

dustymabe commented Sep 15, 2019

OK. I ran through and tested the live PXE, live ISO, and installer ISO with these changes. We have some outstanding comments but they are mostly small fixup items and some of them will lead to followup work (rather than work to do in this PR). We should be able to address the remaining small comments on Monday and open a few issues for followup work (maybe others can pick up some of the tasks) and then get this in!

Nice work @bgilbert!

@bgilbert
Copy link
Contributor Author

@dustymabe Thanks for the reviews and testing!

You're right about SELinux. I have a followup commit that fixes that via relabel-extra.d, but it depends on both selinux-policy-3.14.3-46.fc30 and systemd/systemd#13524.

As far as I know, all of the feedback on this PR set has now been addressed, other than missing functionality that can be handled in a followup. @dustymabe, is there something I've missed? I'd like to get this set of PRs merged to clear the way for the next set.

Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - I've enumerated some followup items below. Should we open up a few issues to track anything that doesn't already have one?

@dustymabe
Copy link
Member

One other thing that I thought of this weekend: It would probably be really nice to have a high level design doc for how this all works since it can be hard to stitch all of the pieces together from the various repos. It's optional of course, but a design doc in the fcos tracker repo (or somewhere) might be helpful to us all in the future.

@bgilbert
Copy link
Contributor Author

pull kernel args for live images from common location as other disk images, possibly blacklist ones we know we don't want for lives

Should be done this week.

enable selinux for live images

Coming in followup PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"enable boot-efi.mount" make ppc64le platform fail
5 participants