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

Add sysroot.bootprefix option #2705

Merged
merged 1 commit into from
Mar 17, 2023
Merged

Conversation

cgwalters
Copy link
Member

This is a follow up to
0ced9fd
"sysroot: Support /boot on root or as seperate filesystem for syslinux and u-boot"

What we should have done at the time is changed our bootloader entries
to be prefixed with /boot. This means that the GRUB2 BLS support
will Just Work.

For now, I'm making this option default to off out of a lot of
conservatism. I think in the future we should flip this on by default.

cgwalters added a commit to cgwalters/coreos-assembler that referenced this pull request Aug 31, 2022
These repository options should be set from the start to ensure
they're honored for the initial deployment too.

Prep for using `sysroot.bootprefix = true` from
ostreedev/ostree#2705
This is a follow up to
ostreedev@0ced9fd
"sysroot: Support /boot on root or as seperate filesystem for syslinux and u-boot"

What we should have done at the time is changed our bootloader entries
to be prefixed with `/boot`.  This means that the GRUB2 BLS support
will Just Work.

For now, I'm making this option default to off out of a lot of
conservatism.  I think in the future we should flip this on by default.
Copy link
Contributor

@martinezjavier martinezjavier left a comment

Choose a reason for hiding this comment

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

LGTM.

@lucab
Copy link
Member

lucab commented Sep 1, 2022

LGTM, but I don't have a good view of the bootloader context.
From the discussion in #2149 it sounds like you really wanted this behavior enabled as the default, right? It feels a bit weird having this config option here, while the rest of that PR is already unconditionally adding boot-prefixes everywhere else.

@cgwalters
Copy link
Member Author

Yeah, perhaps I'm being too chicken 🐔 here...but I'm just so uncertain of the potential blast radius.

@dbnicholson
Copy link
Member

We've definitely had systems with /boot in both scenarios. I... don't remember how we handled it, but I think this makes sense so long as the symlink is appropriately created. What happens when /boot is on a filesystem without symlinks like FAT? Sorry, should probably look closer.

@martinezjavier
Copy link
Contributor

What happens when /boot is on a filesystem without symlinks like FAT? Sorry, should probably look closer.

If /boot is in a vfat partition then ostree won't work anyways, since it relies on symlinks to swap the /boot/loader/entries/.

@cgwalters
Copy link
Member Author

The reason I'm writing this patch now is I was experimenting with the "install inside existing system" on a CentOS Stream 9 cloud guest image, and that has one flat partition - so it needs this.

cgwalters added a commit to coreos/coreos-assembler that referenced this pull request Sep 1, 2022
These repository options should be set from the start to ensure
they're honored for the initial deployment too.

Prep for using `sysroot.bootprefix = true` from
ostreedev/ostree#2705
@dbnicholson
Copy link
Member

What happens when /boot is on a filesystem without symlinks like FAT? Sorry, should probably look closer.

If /boot is in a vfat partition then ostree won't work anyways, since it relies on symlinks to swap the /boot/loader/entries/.

Ah, right. That's another crappy downstream patch we have. I suppose if that's ever handled, it should work anyways since the FAT /boot is almost certainly a separate filesystem and the loader entries with /boot prefixed will work correctly.

@dbnicholson
Copy link
Member

It would be really nice to have a kola test that exercises no separate boot filesystem. I think it would be fairly easy to hack an FCOS system to do it:

cp -aZ /boot /boot.tmp
umount /boot
systemctl mask boot.mount
rm -rf /boot
mv /boot.tmp /boot

But I don't know if there are other parts of FCOS that will fall over if /boot isn't a separate filesystem.

@martinezjavier
Copy link
Contributor

martinezjavier commented Sep 1, 2022

Ah, right. That's another crappy downstream patch we have. I suppose if that's ever handled, it should work anyways since the FAT /boot is almost certainly a separate filesystem and the loader entries with /boot prefixed will work correctly.

Yeah. Out of interest, what your downstream patch does? FYI the renameat2(..., RENAME_EXCHANGE) support for vfat landed in Linux v6.0-rc1, so hopefully we could use that in ostree instead to not depend on symlinks and allow /boot to be in vfat partition (e.g: in an EFI system ESP).

The Linux kernel patches are the following:

019a0c9e377c ("fat: add a vfat_rename2() and make existing .rename callback a helper")
da87e1725ae2 ("fat: add renameat2 RENAME_EXCHANGE flag support")

@dbnicholson
Copy link
Member

Ah, right. That's another crappy downstream patch we have. I suppose if that's ever handled, it should work anyways since the FAT /boot is almost certainly a separate filesystem and the loader entries with /boot prefixed will work correctly.

Yeah. Out of interest, what your downstream patch does?

It's horrible and I hope to be able to get this resolved upstream sooner or later.

endlessm/ostree@e178b1c
endlessm/ostree@a8433f1

@martinezjavier
Copy link
Contributor

It's horrible and I hope to be able to get this resolved upstream sooner or later.

endlessm/ostree@e178b1c endlessm/ostree@a8433f1

Thanks for the links.

I see the motivation there is using sd-boot, that's actually why I also was looking at having atomic replacements in vfat for the ESP. There's an open PR for this #1967 already that we may revive now that we have RENAME_EXCHANGE in vfat.

Another nice thing for sd-boot support could be if ostree has an option to generate BLS snippets that follow the https://systemd.io/AUTOMATIC_BOOT_ASSESSMENT/ filename convention.

@cgwalters
Copy link
Member Author

But I don't know if there are other parts of FCOS that will fall over if /boot isn't a separate filesystem.

It's definitely possible but pokes a bit into implementation details. Today for FCOS we end up "binding" /boot and / via a boot= karg that we'll need to remove too.

I did test this manually locally, and we have a unit test too.

@cgwalters
Copy link
Member Author

This one should also be good to merge I think.

@cgwalters
Copy link
Member Author

@jmarrero this one needs a review stamp

Copy link
Member

@jmarrero jmarrero left a comment

Choose a reason for hiding this comment

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

lgtm

@jmarrero jmarrero merged commit 4f0c13f into ostreedev:main Mar 17, 2023
dustymabe added a commit to dustymabe/osbuild that referenced this pull request Feb 6, 2024
mvo5 pushed a commit to osbuild/osbuild that referenced this pull request Feb 7, 2024
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.

None yet

5 participants