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

drivers: enhance check for mount fs type #1261

Merged
merged 1 commit into from Jun 15, 2022

Conversation

giuseppe
Copy link
Member

when checking that a mount has a specific file system type, also
validate that it is different than its parent directory.

This helps when using virtiofs as the underlying file system since the
check used for fuse-overlayfs would fail since they both use FUSE.

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

if err := unix.Statfs(filepath.Dir(mountPath), &buf); err != nil {
return true, err
}
return FsMagic(buf.Type) != fsType, nil
Copy link
Member

Choose a reason for hiding this comment

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

Since this check was done above shouldn't this be return false, nil?

Copy link
Member Author

Choose a reason for hiding this comment

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

buf is overwritten with the data from the parent directory, the check is on a different directory now

Copy link
Member

Choose a reason for hiding this comment

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

Yup, now I see it it. I did not read the statfs line it was modifying buf as a side effect.
LGTM

@rhatdan
Copy link
Member

rhatdan commented Jun 14, 2022

@nalind
Copy link
Member

nalind commented Jun 14, 2022

when checking that a mount has a specific file system type, also validate that it is different than its parent directory.

If one FUSE filesystem is mounted somewhere below a different FUSE filesystem, why would we want this function to start to return false where it returned true before?

This helps when using virtiofs as the underlying file system since the check used for fuse-overlayfs would fail since they both use FUSE.

Can you elaborate on what this helps with?

@giuseppe
Copy link
Member Author

This helps when using virtiofs as the underlying file system since the check used for fuse-overlayfs would fail since they both use FUSE.

Can you elaborate on what this helps with?

It helps because when using kata containers rootfs itself is running on FUSE:

# podman --runtime kata run --rm fedora stat -f /
  File: "/"
    ID: 0        Namelen: 255     Type: fuseblk
Block size: 4096       Fundamental block size: 4096
Blocks: Total: 10223360   Free: 8628189    Available: 8604117
Inodes: Total: 0          Free: 0

# podman --runtime kata run --rm fedora findmnt /
TARGET SOURCE                                                     FSTYPE OPTIONS
/      kataShared[/105ae764f4312510d6ca2ede27406eab760e06d74056940ac91c7f2879a425bf/rootfs]
                                                                  virtio rw,rela

with such configuration we are not able to use fuse-overlayfs, even if it works fine on top of virtiofs.

I've not found a way to inspect the kind of FUSE file system without parsing /proc/self/mountinfo which is way more expensive than the proposed fix.

@rhvgoyal
Copy link
Contributor

So we are trying to run "buildah bud" inside a kata container and that fails with error.
kernel does not support overlay fs: unable to create kernel-style whiteout: operation not permitted WARN[0000] failed to shutdown storage: "kernel does not support overlay fs: unable to create kernel-style whiteout:operation not permitted

Kata container rootfs is virtiofs which is backed by overlayfs on host.

We were expecting that fuse-overlayfs will be used on top of virtiofs and that should work out of the box. But that's not happening for some reason.

I don't understand this code fully but giuseppe pointed me to this code.

So question is, what's wrong with running a fuse filesystem on top another and why filesystem type matters. And why we are not switching to using fuse-overlay.

Kernel overlayfs will not work as it is because whiteout creation fails on top of host overlayfs. fuse-overlayfs can work as it will switch to regular file starting with .wh as whiteout.

@nalind
Copy link
Member

nalind commented Jun 14, 2022

What's the error that this change helps us avoid? Is the overlay driver getting confused and "detecting" that a fuse-overlayfs mount is already mounted when it isn't, or is it something else?

@rhvgoyal
Copy link
Contributor

No idea. It will be good to have more explanation of current behavior and why that current behavior is not working on top of virtiofs.

@giuseppe
Copy link
Member Author

What's the error that this change helps us avoid? Is the overlay driver getting confused and "detecting" that a fuse-overlayfs mount is already mounted when it isn't, or is it something else?

the error is that the entire / is mounted on top of a FUSE file system. The current check will always return true, so we don't do any fuse-overlayfs mount and we don't do any container mount (thus the rootfs will be empty when the container starts).

The proposed fix instead also checks that the target directory is a new mount point, different than its parent.

@gkurz
Copy link

gkurz commented Jun 15, 2022

I've rebuilt buildah incorporating this change and tested it inside a kata container running on openshift. This fixes the issue we're currently hitting in https://issues.redhat.com/browse/KATA-1278 .

Thanks @giuseppe !

drivers/driver_linux.go Outdated Show resolved Hide resolved
when checking that a mount has a specific file system type, also
validate that it is on a different than its parent directory.

This helps when using virtiofs as the underlying file system since the
check used for fuse-overlayfs would fail since they both use FUSE.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@nalind
Copy link
Member

nalind commented Jun 15, 2022

/approve
LGTM

@rhatdan
Copy link
Member

rhatdan commented Jun 15, 2022

LGTM

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.

None yet

5 participants