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

[RFC] sysroot: Support for directories instead of symbolic links in boot part #3359

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

igoropaniuk
Copy link

Allow manipulating and updating /boot/loader entries under a normal directory, as well as using symbolic links.

For directories this uses renameat2 to do atomic swap of the loader directory in the boot partition. It fallsback to non-atomic rename. This stays atomic on filesystems supporting links but also provide a non-atomic behavior when filesystem does not provide any atomic alternative.

/boot/loader as a normal directory is needed by systemd-boot support, and can be stored under the EFI ESP vfat partition.

Tests were duplicated for simplicity reasons.

Based on the original implementation done by Valentin David [1].

[1] #1967

Copy link

openshift-ci bot commented Dec 19, 2024

Hi @igoropaniuk. Thanks for your PR.

I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@github-actions github-actions bot added the area/prepare-root Issue relates to ostree-prepare-root label Dec 19, 2024
@igoropaniuk igoropaniuk force-pushed the symlinks-directories-support branch from 95b54ad to bb7a7cb Compare December 19, 2024 15:22
Allow manipulating and updating /boot/loader entries under a normal
directory, as well as using symbolic links.

For directories this uses `renameat2` to do atomic swap of the loader
directory in the boot partition. It fallsback to non-atomic rename.
This stays atomic on filesystems supporting links but also provide
a non-atomic behavior when filesystem does not provide any atomic
alternative.

/boot/loader as a normal directory is needed by systemd-boot support,
and can be stored under the EFI ESP vfat partition.

Based on the original implementation done by Valentin David [1].

[1] ostreedev#1967

Signed-off-by: Ricardo Salveti <[email protected]>
Signed-off-by: Jose Quaresma <[email protected]>
Signed-off-by: Igor Opaniuk <[email protected]>
@igoropaniuk igoropaniuk force-pushed the symlinks-directories-support branch from bb7a7cb to 5be249e Compare December 19, 2024 15:22
@igoropaniuk
Copy link
Author

This is based on the initial work done in the #1967 PR (looks like it stalled)
I'll push adjusted tests soon to cover this change.

@cgwalters
Copy link
Member

Thanks so much for picking this back up!! I am very interested in systemd-boot support. But this is just one part of the picture as far as BLS type 1 spec.

Note that that spec added the srel file which I think we should use to dispatch on.

IOW if we find that file, it is an error if /boot/loader is a symlink. We should encourage installers to create that to signal compatibility (after this patch lands).


This is a tangentially related topic but I'd like to ask you: Have you investigated https://github.com/containers/bootc/ ? That's where most of my development work today is going. I will continue to help maintain ostree (and bootc hard depends on it today) but medium term I do plan to slowly sever that dependency (as we head towards composefs in particular, xref https://github.com/containers/composefs-rs/ for some cool PoC work). And especially that would mean here that we may end up doing BLS and UKI support in Rust in bootc, not here.

@cgwalters cgwalters self-assigned this Dec 19, 2024
@cgwalters cgwalters self-requested a review December 19, 2024 19:30
@cgwalters
Copy link
Member

@igoropaniuk Based on your contributions I'd like to offer you reviewer-level access to ostree. This would mean both the right and some responsibility to review/merge PRs made by others.

Comment on lines +2236 to +2238
if (TEMP_FAILURE_RETRY (symlinkat (".", sysroot->sysroot_fd, "boot/boot")) < 0)
if (errno != EPERM && errno != EEXIST)
return glnx_throw_errno_prefix (error, "symlinkat");
Copy link
Member

Choose a reason for hiding this comment

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

This is the bit I'd like to have dispatch on the srel file in particular.

To be conformant, we shouldn't create a symlink even if we can - the spec just says that XBOOTLDR needs to be something "the firmware" can read, and maybe someone makes a firmware that handles ext4 or whatever.

To be clear there's no good reason I am aware of to make XBOOTLDR something other than VFAT today - but still, the srel file seems like the right way to dispatch.

Copy link
Member

Choose a reason for hiding this comment

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

Oh and as a big bonus, dispatching on that file means we can unit test this code without setting up a special hack to make symlink writing fail with EPERM etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/prepare-root Issue relates to ostree-prepare-root needs-ok-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants