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

Mount special filesystem in chroot runners #116

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

Conversation

stagnation
Copy link

This mounts '/proc' and '/sys' in the input root of runners if 'chroot' is enabled. It is done with "at"-syscall semantics in the 'Directory', to avoid side effects. A program-scope mutex is required for unmounting because there is no easy-to-use "unmountat".
#115

@stagnation stagnation force-pushed the feature/mount-special-filesystems-in-chroot-runners branch from 8a86c36 to ea9d269 Compare September 6, 2023 09:25
Copy link
Author

@stagnation stagnation left a comment

Choose a reason for hiding this comment

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

👎

To my reading the mount calls require 'CAP_SYS_ADMIN', there is no 'CAP_MOUNT', so this may have consequences for deployments. But then again, to use chroot where the tools crash is also ill-advised.

if requiresSpecialFilesystems {
var dir filesystem.Directory
dir = r.buildDirectory
// TODO(nils): How to best open the input root `Directory`
Copy link
Author

Choose a reason for hiding this comment

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

This seems to be a misunderstanding on my part.
I want (to hold on to) a Directory object for the input root.
It can be reopened, but do not want to call this opening code twice, I don't think.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, you likely also don't want it, because the directory can be moved around to other places, right?

var dir filesystem.Directory
dir = r.buildDirectory
// TODO(nils): How to best open the input root `Directory`
// It must be a `localDirectory`, but outer directories can be `lazyDirectory`.
Copy link
Author

Choose a reason for hiding this comment

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

This strings-split is a cop out for my lack of understanding of the builder and scope walker.
I want to replace this block.

👎

Copy link
Member

Choose a reason for hiding this comment

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

No worries. I can also help out with this part, after we get the bb-storage part sorted out.

pkg/runner/local_runner.go Outdated Show resolved Hide resolved
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.

Thanks for working on this, Nils!

defer dircloser.Close()
}

dir = dircloser.(filesystem.Directory)
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't DirectoryCloser implement Directory?

pkg/runner/local_runner.go Outdated Show resolved Hide resolved
if requiresSpecialFilesystems {
var dir filesystem.Directory
dir = r.buildDirectory
// TODO(nils): How to best open the input root `Directory`
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, you likely also don't want it, because the directory can be moved around to other places, right?

var dir filesystem.Directory
dir = r.buildDirectory
// TODO(nils): How to best open the input root `Directory`
// It must be a `localDirectory`, but outer directories can be `lazyDirectory`.
Copy link
Member

Choose a reason for hiding this comment

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

No worries. I can also help out with this part, after we get the bb-storage part sorted out.

@stagnation stagnation force-pushed the feature/mount-special-filesystems-in-chroot-runners branch from ea9d269 to 0172979 Compare September 12, 2023 07:33
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.

General remark: Could this be implemented as a decorator for Runner?

type mountingRunner struct {
     base Runner
}

pkg/runner/local_runner.go Outdated Show resolved Hide resolved
pkg/runner/local_runner.go Outdated Show resolved Hide resolved
@stagnation stagnation force-pushed the feature/mount-special-filesystems-in-chroot-runners branch from 0172979 to d813b2f Compare September 13, 2023 08:44
cmd/bb_runner/main.go Show resolved Hide resolved
cmd/bb_runner/main.go Outdated Show resolved Hide resolved
// some tools require access to special filesystems
// that are created when the operating system boots.
// An input root with a full userland implementation may need these.
// Typical choices are:
Copy link
Author

Choose a reason for hiding this comment

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

Is this a good place for this? The chroot connection is smeared all over this work,
but the work can also stand on its own feet, maybe a central "how-to-chroot" document is better?

  //    {"proc", "/proc", "proc"}
  //    {"sys", "/sys", "sysfs"}

// that mounts `mount` before running a build action.
//
// This decorator can be used for chroot runners
// that must mount special filesystems into the input root.
Copy link
Author

Choose a reason for hiding this comment

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

We could also compare it to the clean runner that has generalized pre- and post-command execution.

pkg/runner/mounting_runner.go Outdated Show resolved Hide resolved
@stagnation stagnation force-pushed the feature/mount-special-filesystems-in-chroot-runners branch 2 times, most recently from 13e7f92 to 5dd227a Compare September 15, 2023 08:26
pkg/proto/configuration/bb_runner/bb_runner.proto Outdated Show resolved Hide resolved
pkg/proto/configuration/bb_runner/bb_runner.proto Outdated Show resolved Hide resolved
pkg/proto/configuration/bb_runner/bb_runner.proto Outdated Show resolved Hide resolved
pkg/runner/mounting_runner.go Show resolved Hide resolved
pkg/runner/mounting_runner.go Outdated Show resolved Hide resolved
pkg/runner/mounting_runner.go Outdated Show resolved Hide resolved
pkg/runner/mounting_runner.go Outdated Show resolved Hide resolved
defer root.Close()

mountpoint := path.MustNewComponent(r.mount.Mountpoint)
err = root.Mkdir(mountpoint, 0o555)
Copy link
Member

Choose a reason for hiding this comment

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

Should we be creating the directories ourselves? Maybe it makes more sense to only establish these mounts if they actually exist in the input root?

Copy link
Author

Choose a reason for hiding this comment

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

Good point, with this as an advanced feature, we can probably require very strict compliance from clients that wants to use it. I can require that in the proto specification.

Copy link
Author

Choose a reason for hiding this comment

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

Just a heads-up: When implementing a reproduction case with the directories I realized that the inputRootCharacterDevices inside /dev have the runner create /dev:

func (be *localBuildExecutor) createCharacterDevices(inputRootDirectory BuildDirectory) error {
Should these be symmetric?

pkg/runner/mounting_runner.go Outdated Show resolved Hide resolved
pkg/runner/mounting_runner.go Outdated Show resolved Hide resolved
@stagnation stagnation marked this pull request as draft November 10, 2023 13:40
@stagnation stagnation force-pushed the feature/mount-special-filesystems-in-chroot-runners branch from 5dd227a to 0945ccb Compare November 30, 2023 08:06
@stagnation stagnation force-pushed the feature/mount-special-filesystems-in-chroot-runners branch 9 times, most recently from b1c0f7e to 20533db Compare February 1, 2024 10:45
@stagnation stagnation marked this pull request as ready for review February 1, 2024 12:04
@stagnation stagnation force-pushed the feature/mount-special-filesystems-in-chroot-runners branch from 20533db to 97cecc7 Compare February 2, 2024 09:17
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.

Thanks for working on this!

pkg/proto/configuration/bb_runner/bb_runner.proto Outdated Show resolved Hide resolved
pkg/proto/configuration/bb_runner/bb_runner.proto Outdated Show resolved Hide resolved
scopeWalker := path.NewRelativeScopeWalker(&pathResolver)
defer pathResolver.closeAll()

fullpath := filepath.Join(request.InputRootDirectory, r.mount.Mountpoint)
Copy link
Member

Choose a reason for hiding this comment

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

My suggestion would be to not use filepath.Join() here. It assumes that the native path separator is used, even though request.InputRootDirectory always uses forward slashes, even on Windows. There is also no need to do that if you just call path.Resolve() multiple times:

if err := path.Resolve(request.InputRootDirectory, path.NewRelativeScopeWalker(&pathResolver)); err != nil {
    ...
}
if err := path.Resolve(r.mount.Mountpoint, path.NewRelativeScopeWalker(&pathResolver)); err != nil {
    ...
}

I would also strongly advise against using the same PathResolver for both processing the input root directory and the mount point. Notice how the path resolver contains a stack. That stack is there to process ".." entries. Because the stack isn't cleared/truncated, a mount point containing ".." components might leave the input root directory.

This is not a big issue right now, because the mount point is fully specified in config. But if someone ever comes along and wants the mount point path resolution to respect symlinks, it can become dangerous fairly quickly.

stack: util.NewNonEmptyStack(filesystem.NopDirectoryCloser(r.buildDirectory)),
}
scopeWalker := path.NewRelativeScopeWalker(&pathResolver)
defer pathResolver.closeAll()
Copy link
Member

Choose a reason for hiding this comment

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

This might keep more file descriptors open than strictly necessary. Would there be a way for us to close intermediate directories while Run() is called?

@stagnation stagnation force-pushed the feature/mount-special-filesystems-in-chroot-runners branch 2 times, most recently from b60b67b to 9b52000 Compare February 7, 2024 14:36
@stagnation
Copy link
Author

stagnation commented Feb 16, 2024

I think this is a flaky storage error in the check, but other storage error that occurred around the same time had more explicit error messages.

ERROR: /home/runner/work/bb-remote-execution/bb-remote-execution/cmd/bb_scheduler/BUILD.bazel:93:9: ImageLayer cmd/bb_scheduler/bb_scheduler_container-layer.tar failed: (Exit 1): build_tar failed: error executing command (from target //cmd/bb_scheduler:bb_scheduler_container) bazel-out/k8-opt-exec-2B5CBBC6-ST-625e526ca8a8/bin/external/io_bazel_rules_docker/container/build_tar ... (remaining 5 arguments skipped)

I cannot retrigger it.


Update 10:45

I pushed a rebased PS, this requires Benjamin's updated LLVM toolchain PR

@stagnation stagnation force-pushed the feature/mount-special-filesystems-in-chroot-runners branch from 9b52000 to 07ffd9d Compare February 16, 2024 09:40
This can be used to mount special filesystems like '/proc' and '/sys' in
the input root of actions if 'chroot' is enabled. The filesystems are
required for many tools to work.

Solves: buildbarn#115
@stagnation stagnation force-pushed the feature/mount-special-filesystems-in-chroot-runners branch from 07ffd9d to 2dfbdb8 Compare March 27, 2024 10:18
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