Skip to content

boot: Empty /sysroot too #169

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

Merged
merged 1 commit into from
Aug 5, 2025
Merged

Conversation

cgwalters
Copy link
Collaborator

This aids our compatibility with existing ostree-containers.

Closes: #164

Copy link
Collaborator

@jeckersb jeckersb left a comment

Choose a reason for hiding this comment

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

Needs DCO but otherwise 👍

Copy link
Collaborator

@allisonkarlitskaya allisonkarlitskaya left a comment

Choose a reason for hiding this comment

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

Thanks for the change, and the comments. I more or less agree. Two (related notes):

  • how intentional is the added continue? Should we not create /boot if it doesn't exist? Or is that an error? How about /sysroot?
  • if we do decide to always create/sysroot then we can simplify the examples to skip that step

@cgwalters
Copy link
Collaborator Author

how intentional is the added continue? Should we not create /boot if it doesn't exist? Or is that an error? How about /sysroot?

I was trying to avoid introducing a new failure if somehow the image didn't have /sysroot. Now that I look more closely it seems the code is trying to make it optional here:

match mount_at(&sysroot_clone, &new_root, "sysroot") {

The only rationale I can think of for not making it mandatory is to support having it be somewhere else (say /run/physical-root or something), but I think someone would have to have some decent motivation for that.

So I can change both here and the mount code to make it mandatory?

@cgwalters
Copy link
Collaborator Author

And then it makes sense in the code to have /boot be mandatory too.

That said, there is I think a rationale for having that be configurable; the spec argues for mounting the ESP at /efi, not /boot/efi.

@allisonkarlitskaya
Copy link
Collaborator

Last I heard you're supposed to put the ESP at /boot unless there's also an xloader or whatever partition. So /boot is always mandatory, imho.

I agree that /sysroot could be optional and we do indeed go out of our way to support not mounting it if the mountpoint is missing. On the other hand, Repository (and cfsctl) assume that /sysroot/composefs/ is the location of the "system" repository. Moving that around is just causing ourselves grief for no good reason.

If we did want another (fixed) spot for this under /run I'd consider that to be interesting, but it would complicate things: the distro expects to mount its own tmpfs for /run (or move the one from the initramfs environment?) after (or before?) /sbin/init is started... the fact that I don't know the answer to either of those questions is an indication that we might not want to rely on this...

Also: everyone knows what /sysroot is. Maybe the ship has sailed on moving this one.

But: if someone wants it to be optional to avoid having that mount by default, I think that's legit...

@cgwalters
Copy link
Collaborator Author

Last I heard you're supposed to put the ESP at /boot unless there's also an xloader or whatever partition.

Yeah you're right, see https://uapi-group.org/specifications/specs/boot_loader_specification/#mount-points

But that's mostly talking about the client state which is different from the container image state. Eh I think we can just require /boot/efi in the container image, not the end of the world. (But of course this touches on our debate about masking all of /boot vs just very surgically targeting /usr/lib/modules/$UNAME/*.efi but...yes)

OK so...I updated things to make both required and empty both.

@cgwalters cgwalters changed the title boot: Skip /sysroot too boot: Empty /sysroot too Aug 5, 2025
This aids our compatibility with existing ostree-containers.

Closes: containers#164
Signed-off-by: Colin Walters <walters@verbum.org>
@allisonkarlitskaya
Copy link
Collaborator

I think we'll never have /boot/efi in the root filesystem, though, right? /boot will come from the boot partition (either xbootldr or efi) and efi may be a subdirectory of that (but even that is deprecated, from what I understand)... So we'd certainly never demand for /boot/efi to be part of the container image...

Copy link
Collaborator

@allisonkarlitskaya allisonkarlitskaya left a comment

Choose a reason for hiding this comment

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

Can't argue with that. We can adjust this later, if needed.

Thanks!

@cgwalters cgwalters merged commit 83dee22 into containers:main Aug 5, 2025
14 checks passed
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.

erofs: Incorrect selinux labels
3 participants