Mount special filesystem in chroot runners #116

stagnation · 2023-09-06T09:22:38Z

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

👎

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.

stagnation · 2023-09-06T09:26:58Z

pkg/runner/local_runner.go

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


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.

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

stagnation · 2023-09-06T09:27:50Z

pkg/runner/local_runner.go

+		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`.


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

👎

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

pkg/runner/local_runner.go

EdSchouten

Thanks for working on this, Nils!

EdSchouten · 2023-09-06T10:17:10Z

pkg/runner/local_runner.go

+				defer dircloser.Close()
+			}
+
+			dir = dircloser.(filesystem.Directory)


Doesn't DirectoryCloser implement Directory?

pkg/runner/local_runner.go

EdSchouten · 2023-09-06T10:18:07Z

pkg/runner/local_runner.go

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


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

EdSchouten · 2023-09-06T10:18:42Z

pkg/runner/local_runner.go

+		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`.


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

EdSchouten

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

type mountingRunner struct {
     base Runner
}

pkg/runner/local_runner.go

cmd/bb_runner/main.go

stagnation · 2023-09-13T08:58:50Z

pkg/proto/configuration/bb_runner/bb_runner.proto

+  // 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:


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"}

stagnation · 2023-09-13T09:01:17Z

pkg/runner/mounting_runner.go

+// 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.


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

pkg/runner/mounting_runner.go

pkg/proto/configuration/bb_runner/bb_runner.proto

pkg/runner/mounting_runner.go

EdSchouten · 2023-09-16T16:52:17Z

pkg/runner/mounting_runner.go

+	defer root.Close()
+
+	mountpoint := path.MustNewComponent(r.mount.Mountpoint)
+	err = root.Mkdir(mountpoint, 0o555)


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?

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.

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

bb-remote-execution/pkg/builder/local_build_executor.go

Line 93 in fe4cf5d

func (be *localBuildExecutor) createCharacterDevices(inputRootDirectory BuildDirectory) error {

Should these be symmetric?

pkg/runner/mounting_runner.go

EdSchouten

Thanks for working on this!

pkg/proto/configuration/bb_runner/bb_runner.proto

EdSchouten · 2024-02-07T11:23:23Z

pkg/runner/mounting_runner.go

+	scopeWalker := path.NewRelativeScopeWalker(&pathResolver)
+	defer pathResolver.closeAll()
+
+	fullpath := filepath.Join(request.InputRootDirectory, r.mount.Mountpoint)


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.

EdSchouten · 2024-02-07T11:24:04Z

pkg/runner/mounting_runner.go

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


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 · 2024-02-16T09:35:31Z

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

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 force-pushed the feature/mount-special-filesystems-in-chroot-runners branch from 8a86c36 to ea9d269 Compare September 6, 2023 09:25

stagnation commented Sep 6, 2023

View reviewed changes

EdSchouten reviewed Sep 6, 2023

View reviewed changes

stagnation force-pushed the feature/mount-special-filesystems-in-chroot-runners branch from ea9d269 to 0172979 Compare September 12, 2023 07:33

stagnation mentioned this pull request Sep 12, 2023

Mount '/proc' into chrooted actions #115

Open

EdSchouten reviewed Sep 12, 2023

View reviewed changes

pkg/runner/local_runner.go Outdated Show resolved Hide resolved

pkg/runner/local_runner.go Outdated Show resolved Hide resolved

stagnation force-pushed the feature/mount-special-filesystems-in-chroot-runners branch from 0172979 to d813b2f Compare September 13, 2023 08:44

stagnation commented Sep 13, 2023

View reviewed changes

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

EdSchouten reviewed Sep 16, 2023

View reviewed changes

stagnation marked this pull request as draft November 10, 2023 13:40

stagnation force-pushed the feature/mount-special-filesystems-in-chroot-runners branch from 5dd227a to 0945ccb Compare November 30, 2023 08:06

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 marked this pull request as ready for review February 1, 2024 12:04

stagnation force-pushed the feature/mount-special-filesystems-in-chroot-runners branch from 20533db to 97cecc7 Compare February 2, 2024 09:17

EdSchouten reviewed Feb 7, 2024

View reviewed changes

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 force-pushed the feature/mount-special-filesystems-in-chroot-runners branch from 9b52000 to 07ffd9d Compare February 16, 2024 09:40

Fix typo

89ca85f

Mount special filesystems in chroot runners

2dfbdb8

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mount special filesystem in chroot runners #116

Mount special filesystem in chroot runners #116

stagnation commented Sep 6, 2023

stagnation left a comment •

edited

Loading

stagnation Sep 6, 2023

EdSchouten Sep 6, 2023

stagnation Sep 6, 2023

EdSchouten Sep 6, 2023

EdSchouten left a comment

EdSchouten Sep 6, 2023

EdSchouten Sep 6, 2023

EdSchouten Sep 6, 2023

EdSchouten left a comment

stagnation Sep 13, 2023

stagnation Sep 13, 2023

EdSchouten Sep 16, 2023

stagnation Jan 30, 2024

stagnation Jan 31, 2024

EdSchouten left a comment

EdSchouten Feb 7, 2024

EdSchouten Feb 7, 2024

stagnation commented Feb 16, 2024 •

edited

Loading

Mount special filesystem in chroot runners #116

Are you sure you want to change the base?

Mount special filesystem in chroot runners #116

Conversation

stagnation commented Sep 6, 2023

stagnation left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdSchouten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdSchouten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdSchouten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stagnation commented Feb 16, 2024 • edited Loading

stagnation left a comment •

edited

Loading

stagnation commented Feb 16, 2024 •

edited

Loading