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

Enable directmountstrict mode for fuse #156

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sfc-gh-guwang
Copy link

@sfc-gh-guwang sfc-gh-guwang commented Dec 12, 2024

When fuse uses directmount mode, and direct mount fails, it will fall back to use 'fusermount'. BB binaries(bb-worker) doesn't have fusermount, and will always output the error message 'Fatal error: rpc error: code = Unknown desc = Failed to expose build directory mount: Failed to create FUSE server: exec: "/bin/fusermount": stat /bin/fusermount: no such file or directory'

This error message is confusing and the real error has been swallowed.

This PR enables Buildbarn to pass along DirectMountStrict options that will not do fallbacks.

// DirectMountStrict is like DirectMount but no fallback to fusermount is
// performed. If both DirectMount and DirectMountStrict are set,
// DirectMountStrict wins.
bool direct_mount_strict = 10;
Copy link
Member

Choose a reason for hiding this comment

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

I'm always a big fan of making illegal states unrepresentable. There are only three behaviors:

  • Always use fusermount
  • Attempt to mount directly, falling back to fusermount
  • Only attempt to mount directly

Yet two booleans give us four different options. Could you replace direct_mount and direct_mount_strict by an enum? Maybe something like this:

enum MountMethod {
  // Call into the fusermount utility to create the FUSE mount.
  FUSERMOUNT = 0;

  // Create the FUSE mount using the mount() system call. This can be
  // used in environments where the fusermount utility is not available,
  // such as the bb_worker container images.
  DIRECT = 1;

  // First attempt to create the FUSE mount using the mount() system
  // call. Upon failure, fall back to the fusermount utility.
  DIRECT_AND_FUSERMOUNT = 2;
}

Copy link
Author

Choose a reason for hiding this comment

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

Updated!

Copy link
Member

@EdSchouten EdSchouten left a comment

Choose a reason for hiding this comment

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

This looks good. I have one final minor comment. Thanks for working on this!

case virtualpb.FUSEMountConfiguration_FUSERMOUNT:
// No additional options needed for FUSERMOUNT
default:
// Default to use FUSERMOUNT
Copy link
Member

@EdSchouten EdSchouten Dec 14, 2024

Choose a reason for hiding this comment

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

Please make the default fail:

return status.Error(codes.InvalidArgument, “Invalid mount method”)

And add an explicit case for FUSERMOUNT.

@EdSchouten
Copy link
Member

Also make sure to remove the +x bit from that .pb.go file to keep CI happy.

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.

2 participants