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

Stop if /dev is not a bind mount with loopback #431

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

bcrochet
Copy link
Contributor

@bcrochet bcrochet commented Mar 26, 2024

At container start, /dev is snapshotted, so any new device files
don't get added unless /dev is bindmounted in. For now, check that
/dev is the same as the host's /dev via fsid. If they differ, it
means that /dev is not bindmounted.

Fixes #352

@github-actions github-actions bot added the area/install Issues related to `bootc install` label Mar 26, 2024
@bcrochet
Copy link
Contributor Author

What's here now is the "quick and dirty" fix. If this is sufficient for the time being, I'd say it's good-to-go. I'm currently looking at cleaning up some of the mount code (there are lots of places that it's done now). Some deduplication is in order.

@bcrochet
Copy link
Contributor Author

What's here now is the "quick and dirty" fix. If this is sufficient for the time being, I'd say it's good-to-go. I'm currently looking at cleaning up some of the mount code (there are lots of places that it's done now). Some deduplication is in order.

Moved around a little bit. I've added it as an additional commit. Will collapse before merging.

Copy link
Collaborator

@cgwalters cgwalters 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 working on this, our saga of filesystem mounts will probably continue for a while 😄

lib/src/mount.rs Outdated Show resolved Hide resolved
lib/src/mount.rs Outdated Show resolved Hide resolved
lib/src/mount.rs Outdated Show resolved Hide resolved
lib/src/mount.rs Outdated Show resolved Hide resolved
lib/src/mount.rs Outdated Show resolved Hide resolved
lib/src/blockdev.rs Outdated Show resolved Hide resolved
@bcrochet
Copy link
Contributor Author

I got trigger-happy with pushing. I still need to address the idempotency part.

@cgwalters
Copy link
Collaborator

cgwalters commented Mar 28, 2024

Per discussion I think simplest may be:

  • Recommend -v /dev:/dev and for now error out if we're doing loopback and /dev is not the real system /dev (I think we could statx /dev versus /proc/1/root/dev and compare stx_mnt_id ?)

Other two paths are:

@bcrochet
Copy link
Contributor Author

Per discussion I think simplest may be:

  • Recommend -v /dev:/dev and for now error out if we're doing loopback and /dev is not the real system /dev (I think we could statx /dev versus /proc/1/root/dev and compare stx_mnt_id ?)

Other two paths are:

Agreed. Let's start with the error out and suggestion of -v /dev:/dev.

@cgwalters
Copy link
Collaborator

Comparing the mounts I think can be done via https://docs.rs/rustix/latest/rustix/fs/fn.statx.html

@bcrochet
Copy link
Contributor Author

bcrochet commented Apr 2, 2024

So, this code works as expected, however for some reason once -v /dev:/dev is added, the losetup is still failing for me locally. Investigating...

lib/src/mount.rs Outdated Show resolved Hide resolved
@bcrochet bcrochet changed the title Ensure /dev is mounted before losetup is called Stop if /dev is not a bind mount with loopback Apr 2, 2024
@bcrochet
Copy link
Contributor Author

bcrochet commented Apr 2, 2024

So, this code works as expected, however for some reason once -v /dev:/dev is added, the losetup is still failing for me locally. Investigating...

It works on a centos VM, but not on a fedora host directly. That's the only diff I have so far.

lib/src/mount.rs Outdated Show resolved Hide resolved
lib/src/install.rs Outdated Show resolved Hide resolved
lib/src/mount.rs Outdated Show resolved Hide resolved
At container start, /dev is snapshotted, so any new device files
don't get added unless /dev is bindmounted in. For now, check that
/dev is the same as the host's /dev via fsid. If they differ, it
means that /dev is not bindmounted.

Fixes containers#352

Signed-off-by: Brad P. Crochet <brad@redhat.com>
@cgwalters cgwalters enabled auto-merge April 2, 2024 14:50
@cgwalters cgwalters merged commit f0959e6 into containers:main Apr 2, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/install Issues related to `bootc install`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Requires -v /dev:/dev for reliable loopback
3 participants