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

bb_runner: Allow it to chroot into the input root #23

Closed
Qinusty opened this issue Sep 5, 2019 · 4 comments
Closed

bb_runner: Allow it to chroot into the input root #23

Qinusty opened this issue Sep 5, 2019 · 4 comments

Comments

@Qinusty
Copy link
Contributor

Qinusty commented Sep 5, 2019

This comes as a proposal to implement behavior which is currently outside of the REAPI spec but is seen as an additional non standard platform property within RBE.

The proposal is that the worker/runner be provided with an optional platform property allowing the client to request that the action be executed within a sandbox within the input root. Currently this is the case but this is simply coincidental that the build directory is also the input root currently (See #20).

@Qinusty
Copy link
Contributor Author

Qinusty commented Sep 5, 2019

This was briefly disuccsed on the #buildstream IRC on gimpnet on 04/09/2019 by @juergbi and @kinnison.

...
10:13 <juergbi>  to support buildstream clients, the server/worker either has to directly support this extra chroot, e.g., triggered via platform property
10:13 <juergbi>  or buildstream has to use chroot as action command
10:14 <Kinnison>  juergbi: the latter requiring that the action run as root presumably
10:14 <juergbi>  yes, although it can be root in a user namespace, or we could use bwrap instead of chroot
10:15 <juergbi>  however, it's not so easy to do it right as this is a platform-specific thing, of course
10:15 <juergbi>  my preferred solution would be a platform property in standard REAPI that will trigger this on the server/worker side
10:16 <juergbi>  right now REAPI is underspecified in this regard. if you just use what's in the spec, the client has no idea what kind of environment the action will run in
10:16 <juergbi>  what path, what tools will be available etc.
10:16 <juergbi>  this generally can't work, also not for bazel
10:16 <juergbi>  Google RBE has a (non-standard) platform property to allow specifying a docker image for the action to run in
10:17 <juergbi>  that is theoretically good enough (but it's still a bit inconvenient for buildstream as it's platform specific, and doesn't work at all on non-Linux)
10:18 <juergbi>  we should probably make a proposal for such a platform property
10:18 <juergbi>  and ideally provide patches for buildbarn and buildfarm
10:19 <juergbi>  or at least bring it up on mailing list or in the monthly meeting
10:20 <qinusty>  And this would be a modification to the REAPI proto?
10:22 <juergbi>  addition
10:22 <juergbi>  could be part of the platform lexicon, i.e., considered optional to implement

@Qinusty Qinusty changed the title Consider addition of property to enable chroot within input root Consider addition of property to enable sandboxing within input root Sep 5, 2019
@EdSchouten EdSchouten changed the title Consider addition of property to enable sandboxing within input root bb_runner: Allow it to chroot into the input root Mar 26, 2020
@EdSchouten
Copy link
Member

Renamed this issue to explicitly mention the chroot use case. Stuff like respecting the container-image label is out of scope for this issue, as I think that should be done as part of #40.

EdSchouten added a commit that referenced this issue Apr 2, 2020
When support for multiple runners for a single worker as added, we ran
into a pretty serious design flaw: the cardinality between the number of
build directories and runners had changed: 1:1 -> 1:*. Back then we sort
of ignored this issue, which led interesting bugs, such as cleaning of
the build directory not working properly in multi-runner setups. We
eventually worked around this by disabling this feature. Some TODOs in
cmd/bb_worker/main.go remained.

The goal of this change is to finally tackle that problem by basically
kicking out the Environment interface, which caused the 1:1 coupling. It
introduces separate BuildDirectory and Runner types. LocalBuildExecutor
now takes individual copies of each.

Because of this decoupling, we can also fix #20. Builds are no longer
performed in the build directory itself. Instead, they are run inside a
subdirectory called "root". This makes it possible to use the top level
directory for other purposes (e.g., storing stdout and stderr logs).

It also partially addresses #23. The runner protocol has been extended
to explicitly pass on the input root directory, so that a runner could
chroot into it.
@EdSchouten
Copy link
Member

This should be easy to implement now. The runner protocol now has a dedicated field that provides you the input root directory:

// Path of the input root, relative to the build directory.
string input_root_directory = 6;

That is the directory in which you should chroot.

@EdSchouten
Copy link
Member

We now have chroot support. 🎉

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

No branches or pull requests

2 participants