sysroot: Support /boot on root or as seperate filesystem for syslinux and u-boot #2149

wmanley · 2020-07-15T16:56:48Z

We use a similar trick to having a sysroot -> . symlink on the real root
here to support both /boot on root as well as on a separate filesystem. No
matter how it's mounted /boot/xyz will always refer to the file you'd
expect.

This is nicer than my previous attempts at this because there's no
configuration nor auto-detection required.

Fixes #1452

Replaces #1404, #1914 and #2145

Reduce duplication.

… and u-boot We use a similar trick to having a `sysroot -> .` symlink on the real root here to support both /boot on root as well as on a separate filesystem. No matter how it's mounted `/boot/xyz` will always refer to the file you'd expect. This is nicer than my previous attempts at this because there's no configuration nor auto-detection required.

wmanley · 2020-07-16T10:45:44Z

jenkins ci is failing with the same issue that is currently plaguing master:

fork: retry: Resource temporarily unavailable

but I consider this ready for review. If this goes in I'll be glad to close the PRs with the other attempts :).

I've only tested this on real systems with /boot on root and in syslinux mode. Our systems have u-boot, but they read the syslinux config.

vtolstov · 2020-07-16T12:06:33Z

"\yeah, i'm really need this too as i have /boot on root

wmanley · 2020-07-20T11:57:21Z

jenkins ci is failing with the same issue that is currently plaguing master:

CI is passing now, presumably thanks to #2151. Thanks @jlebon and @cgwalters.

cgwalters · 2020-07-23T20:31:49Z

src/libostree/ostree-sysroot-deploy.c

@@ -1998,6 +1998,12 @@ prepare_new_bootloader_link (OstreeSysroot  *sysroot,
  g_assert ((current_bootversion == 0 && new_bootversion == 1) ||
            (current_bootversion == 1 && new_bootversion == 0));

+  /* This allows us to support both /boot on a seperate filesystem to / as well
+   * as on the same filesystem. */
+  if (TEMP_FAILURE_RETRY (symlinkat (".", sysroot->sysroot_fd, "boot/boot")) < 0)


Right, the simplicity of this is clearly tempting. I think what I'm pausing on here though is that so far libostree doesn't really create files except in a few places.

But...I just want to revisit again, were we overthinking this previously around detecting /boot being a mount point? Can't we just statvfs(/) and statvfs(/boot) and check the device IDs?

Passing down a boolean for that to the bootloader generation shouldn't be too hard.

RE: Btrfs

statfs() f_fsid will be different per subvolume on the same file system. The first half (8 nibbles?) of the ID reported by stat -f seems to be consistent regardless of subvolume, but I'm not certain if the 2nd half of the ID can "roll over" once subvol ID's get high enough.

inode 256 is reserved for Btrfs subvolumes, i.e. if the file system is btrfs and the inode is 256, then it is a subvolume (or snapshot).

mountpoint -d returns the same value for each mountpoint that's the same Btrfs file system, even when backed by different subvolumes. This value looks like MAG:MIN but it doesn't match the MAG:MIN if the actual physical device partition.

Maybe more challenging is that a subvolume could be named anything and exist anywhere on a Btrfs file system, and yet mounted at /boot. This is relevant because this path to subvolume is what's needed for the bootloader to find it. I see BTRFS_IOC_INO_LOOKUP and BTRFS_IOC_TREE_SEARCH ioctl's used by btrfs subvolume list command.

Updated: There are other combinations. /boot could be a directory on a subvolume 'fedora33root'. It could also be a nested subvolume inside 'fedora33root'. Neither of these /boot are mounted. mount does show the "path to subvolume" for any mountpoint using Btrfs. Simplistically, if statfs() says vmlinuz/initramfs are on Btrfs, and somehow have f_fsid turned into a path-to-subvolume for the bootloader config. The mount command shows this path to subvolume for any mountpoint using btrfs - so path to kernel+initrd could be inferred.

cgwalters · 2020-07-23T20:38:08Z

Thanks a lot for working on this BTW!

cgwalters · 2020-07-23T20:38:41Z

To be clear I'm leaning towards #2145 but also happy to continue discussion.

wmanley · 2020-07-24T11:21:42Z

what I'm pausing on here though is that so far libostree doesn't really create files except in a few places.

AIUI /boot is one of those places - ostree creates and manages the /boot/loader symlink. Ideally I think it would be nice to do this in ostree admin init-fs instead, but for backwards compatibility creating it during deploy is necessary.

Could you expand on the reasoning for this policy? I'm having difficulty thinking of downsides.

An alternative would be to check for the /boot/boot -> . symlink and only prepend /boot if it's detected. This would have the perverse effect that for users where boot is on root the symlink must exist but won't be used - but it would preserve backwards compatibility and mean that ostree wouldn't have to create that symlink at deploy time.

But...I just want to revisit again, were we overthinking this previously around detecting /boot being a mount point? Can't we just statvfs(/) and statvfs(/boot) and check the device IDs?

I think there's a general problem that the system that is running ostree admin deploy isn't necessarily the same as the system that will be booting the deployed image, i.e when using the --sysroot option. This has come up recently for me as I've been deploying to SD and then booting on another system, but I could imagine other scenarios where ostree admin deploy --sysroot is used as part of a larger build process. The point being that you can't necessarily rely on probing the current filesystem setup to determine how it will be used/perceived by the bootloader.

I ran into this when working on #1404, which prompted the creation of #2145 - which, as you say, makes things explicit. There's more discussion here: #1404 (comment)

To be clear I'm leaning towards #2145 but also happy to continue discussion.

I have a preference for this one rather than #2145 because:

There's no configuration needed
It defers to the bootloader at boot time, rather than relying on correctly specifying how the bootloader was configured ahead of time. This is the central point that makes auto-detection so treacherous. We'd be using mount information of the currently booted system as a proxy for information on how the bootloader will behave when the system (or image) is booted.
The implementation is super simple - sysroot: Support customising /boot on root for syslinux and u-boot #2145 was a pain to write to get concatenation of paths correct without double // or missing ones.
Booting with ostree already depends on the bootloaders knowing how to resolve symlinks, so there's no additional requirements here.
It's consistent with how we recommend that /ostree -> sysroot/ostree, etc., albeit /boot is a shared location, while these other symlinks
It doesn't require further work in terms of improving auto-detection of how /boot is set up. My experience of trying to get the auto-detection "right" in sysroot: Support /boot on / for syslinux and uboot #1404 was that it was really quite difficult to write tests for.

I think there are some commits in #2145 that would be worth salvaging, like the sysroot.bootloader config option improvements. But I can't see what benefits the sysroot.boot_path config option really provides over this PR, but I'd like to hear more about the file creation issue.

cgwalters · 2020-08-07T15:37:07Z

@martinezjavier do you have an opinion on this one?

martinezjavier · 2020-08-13T12:31:35Z

@martinezjavier do you have an opinion on this one?

I think that I'm leaning towards this PR instead of #2125, not only because is simpler as you mentioned which is appealing but also because this issue is also present for GRUB.

GRUB expects the kernel and initrd images to be loaded from a path relative to the root of the partition where these are located. For example the menuentry commands (when not using the BLS module) would have something like the following:

linux /ostree/fedora-13eb3067f1bbc3ed7128faf435a87a5cbad451f99c2696380b7c7b65c57d8f03/vmlinuz-5.6.0-0.rc5.git1.1.fc33.x86_64
initrd /ostree/fedora-13eb3067f1bbc3ed7128faf435a87a5cbad451f99c2696380b7c7b65c57d8f03/initramfs-5.6.0-0.rc5.git1.1.fc33.x86_64.img

because the kernel and initrd images are in the /ostree directory that is in the root of the partition mounted as /boot. But if /boot is not a mount point, these should be something like:

linux /boot/ostree/fedora-13eb3067f1bbc3ed7128faf435a87a5cbad451f99c2696380b7c7b65c57d8f03/vmlinuz-5.6.0-0.rc5.git1.1.fc33.x86_64
initrd /boot/ostree/fedora-13eb3067f1bbc3ed7128faf435a87a5cbad451f99c2696380b7c7b65c57d8f03/initramfs-5.6.0-0.rc5.git1.1.fc33.x86_64.img

The GRUB blscfg module just translates the linux and initrd fields from the BLS snippets to GRUB linux and initrd commands, so the current BLS snippets generated by OSTree only work if /boot is a mount point.

By having the /boot/boot -> . symlink, the BLS snippets could always have a /boot prefix in the linux and initrd fields. If there is a boot partition, the symlink will just be followed by the bootloaders to the root of the filesystem in the boot partition.

So my suggestion would be to not make this something specific to syslinux and u-boot but instead add the prefix to the BLS snippets. That way src/libostree/ostree-bootloader-{syslinux,uboot}.c won't need any changes and could just use the value of the fields in the BLS snippets which already will have the path with the prefix.

I remember there was a similar issue with the zipl tool, that always expected the paths in the BLS snippets to have a /boot prefix. And the tool had to be changed to support the BLS generated by OSTree (see commit ibm-s390-linux/s390-tools@d71628326d8).

As @wmanley mentioned, OSTree already relies heavily on symlinks. Adding another one to cover this case doesn't seem that bad to me.

cgwalters · 2020-08-18T22:22:45Z

I think I'd be a lot happier with this if this symlink wasn't something we invented in ostree, but e.g. something upstream bootloader maintainers recommended to create as well. I have no idea how to drive that consensus though.

cgwalters · 2020-08-18T22:23:25Z

OK restarted the CI
/lgtm

openshift-ci-robot · 2020-08-18T22:23:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, wmanley

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cgwalters,wmanley]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bam80 · 2020-09-11T20:32:16Z

So will it work for grub also?

wmanley · 2020-09-14T16:19:54Z

So will it work for grub also?

I'm sure it could be made to work for grub, but I haven't changed anything WRT grub here. I'd expect the equivalent change for grub to be straightforward.

cgwalters · 2022-08-31T13:54:06Z

So...hmm, I think what we didn't end up doing here is this part:

but instead add the prefix to the BLS snippets

?

martinezjavier · 2022-08-31T14:02:51Z

So...hmm, I think what we didn't end up doing here is this part:

but instead add the prefix to the BLS snippets

?

Yeah, it seems that was not done.

cgwalters · 2022-08-31T19:01:02Z

PR in #2705

Refactor tests/bootloader-entries-crosscheck.py

4deb426

Reduce duplication.

openshift-ci-robot requested review from mwleeds and rfairley July 15, 2020 16:58

openshift-ci-robot added the approved label Jul 15, 2020

wmanley force-pushed the boot-self-symlink branch from 7db8cab to 35e77a0 Compare July 15, 2020 17:22

wmanley force-pushed the boot-self-symlink branch from 35e77a0 to 0ced9fd Compare July 15, 2020 19:39

cgwalters reviewed Jul 23, 2020

View reviewed changes

openshift-ci-robot assigned cgwalters Aug 18, 2020

openshift-ci-robot added the lgtm label Aug 18, 2020

openshift-merge-robot merged commit 199562f into ostreedev:master Aug 18, 2020

This was referenced Aug 18, 2020

sysroot: Support customising /boot on root for syslinux and u-boot #2145

Closed

Bootloader no boot partition #1914

Closed

sysroot: Support /boot on / for syslinux and uboot #1404

Closed

wmanley deleted the boot-self-symlink branch September 1, 2020 13:32

VladimirSlavik mentioned this pull request Sep 25, 2020

Enforce /boot as separate partition on Silverblue rhinstaller/anaconda#2848

Closed

lucab mentioned this pull request Sep 1, 2022

Add sysroot.bootprefix option #2705

Merged

vnd mentioned this pull request Jan 3, 2023

/boot on a separate partition #2801

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sysroot: Support /boot on root or as seperate filesystem for syslinux and u-boot #2149

sysroot: Support /boot on root or as seperate filesystem for syslinux and u-boot #2149

wmanley commented Jul 15, 2020

wmanley commented Jul 16, 2020

vtolstov commented Jul 16, 2020

wmanley commented Jul 20, 2020

cgwalters Jul 23, 2020

cmurf Sep 25, 2020 •

edited

Loading

cgwalters commented Jul 23, 2020

cgwalters commented Jul 23, 2020

wmanley commented Jul 24, 2020

cgwalters commented Aug 7, 2020

martinezjavier commented Aug 13, 2020

cgwalters commented Aug 18, 2020

cgwalters commented Aug 18, 2020

openshift-ci-robot commented Aug 18, 2020

bam80 commented Sep 11, 2020

wmanley commented Sep 14, 2020

cgwalters commented Aug 31, 2022

martinezjavier commented Aug 31, 2022

cgwalters commented Aug 31, 2022

sysroot: Support /boot on root or as seperate filesystem for syslinux and u-boot #2149

sysroot: Support /boot on root or as seperate filesystem for syslinux and u-boot #2149

Conversation

wmanley commented Jul 15, 2020

wmanley commented Jul 16, 2020

vtolstov commented Jul 16, 2020

wmanley commented Jul 20, 2020

cgwalters Jul 23, 2020

Choose a reason for hiding this comment

cmurf Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

cgwalters commented Jul 23, 2020

cgwalters commented Jul 23, 2020

wmanley commented Jul 24, 2020

cgwalters commented Aug 7, 2020

martinezjavier commented Aug 13, 2020

cgwalters commented Aug 18, 2020

cgwalters commented Aug 18, 2020

openshift-ci-robot commented Aug 18, 2020

bam80 commented Sep 11, 2020

wmanley commented Sep 14, 2020

cgwalters commented Aug 31, 2022

martinezjavier commented Aug 31, 2022

cgwalters commented Aug 31, 2022

cmurf Sep 25, 2020 •

edited

Loading