ostree icon indicating copy to clipboard operation
ostree copied to clipboard

Add `sysroot.bootprefix` option

Open cgwalters opened this issue 2 years ago • 11 comments

This is a follow up to https://github.com/ostreedev/ostree/pull/2149/commits/0ced9fde7649271d9458ca424aa8c41908634b02 "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 avatar Aug 31 '22 19:08 cgwalters

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.

lucab avatar Sep 01 '22 10:09 lucab

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

cgwalters avatar Sep 01 '22 12:09 cgwalters

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.

dbnicholson avatar Sep 01 '22 13:09 dbnicholson

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/.

martinezjavier avatar Sep 01 '22 13:09 martinezjavier

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 avatar Sep 01 '22 13:09 cgwalters

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 avatar Sep 01 '22 15:09 dbnicholson

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.

dbnicholson avatar Sep 01 '22 15:09 dbnicholson

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")

martinezjavier avatar Sep 01 '22 16:09 martinezjavier

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.

https://github.com/endlessm/ostree/commit/e178b1c228c0a9dd1669f1b9f61ecc3afe233b62 https://github.com/endlessm/ostree/commit/a8433f1204381f9b4a1b1f09ea17b35e4265f20c

dbnicholson avatar Sep 01 '22 17:09 dbnicholson

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.

martinezjavier avatar Sep 01 '22 17:09 martinezjavier

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 avatar Sep 07 '22 14:09 cgwalters

This one should also be good to merge I think.

cgwalters avatar Feb 17 '23 21:02 cgwalters

@jmarrero this one needs a review stamp

cgwalters avatar Mar 06 '23 15:03 cgwalters