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

Preserve paths under hermetic /tmp in the sandbox #22001

Closed
wants to merge 32 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Apr 15, 2024

The bind mounting scheme used with the Linux sandbox' hermetic /tmp feature is modified to preserve all paths as they are outside the sandbox, which removes the need to rewrite paths when staging inputs into and, crucially, moving outputs out of the sandbox.

Source roots and output base paths under /tmp are now treated just like any user-specified bind mount under /tmp: They are mounted under the hermetic tmp directory with their path relativized against /tmp before the hermetic tmp directory is mounted as /tmp as the final step.

There is one caveat compared to user-specified mounts: Source roots, which may themselves not lie under /tmp, can be symlinks to directories under /tmp (e.g., when they arise from a local_repository). To handle this situation in the common case, all parent directories of package path entries (up to direct children of /tmp) are mounted into the sandbox. If users use local_repositorys with fixed target paths under /tmp, they will need to specify --sandbox_add_mount_pair.

Overlayfs has been considered as an alternative to this approach, but ultimately doesn't seem to work for this use case since its lowerpath, which would be /tmp, is not allowed to have child mounts from a different user namespace (see https://unix.stackexchange.com/questions/776030/mounting-overlayfs-in-a-user-namespace-with-child-mounts). However, this is exactly the situation created by a Bazel-in-Bazel test and can also arise if the user has existing mounts under /tmp when using Bazel (e.g. the JetBrains toolbox on Linux uses such mounts).

This replaces and mostly reverts the following commits, but keeps their tests:

Fixes #20533
Work towards #20753
Fixes #21215
Fixes #22117
Fixes #22226
Fixes #22290

RELNOTES: Paths in the Linux sandbox are now again identical to those outside the sandbox, even with --incompatible_sandbox_hermetic_tmp.

@fmeum fmeum changed the title Replace hermetic /tmp/ implementation with overlayfs Replace hermetic /tmp implementation with overlayfs Apr 15, 2024
@fmeum fmeum force-pushed the 21215-revert-hermetic-tmp-impl branch 2 times, most recently from e5dbc28 to aec09a2 Compare April 18, 2024 20:13
@fmeum fmeum force-pushed the 21215-revert-hermetic-tmp-impl branch 8 times, most recently from 4945777 to 21fcad5 Compare May 8, 2024 16:12
@fmeum fmeum changed the title Replace hermetic /tmp implementation with overlayfs Preserve paths under hermetic /tmp in the sandbox May 8, 2024
@fmeum fmeum force-pushed the 21215-revert-hermetic-tmp-impl branch from 3a641ff to 9055f00 Compare May 8, 2024 17:02
@fmeum fmeum requested review from lberki and oquenchil May 8, 2024 17:02
@fmeum fmeum marked this pull request as ready for review May 8, 2024 17:03
@fmeum
Copy link
Collaborator Author

fmeum commented May 8, 2024

@bazel-io fork 7.2.0

@github-actions github-actions bot added team-Performance Issues for Performance teams team-Local-Exec Issues and PRs for the Execution (Local) team awaiting-review PR is awaiting review from an assigned reviewer labels May 8, 2024
@@ -309,6 +317,68 @@ EOF
bazel build //pkg:a &>$TEST_log || fail "expected build to succeed"
}

# Sets up targets under //test that, when building //test:all, verify that the
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I introduced this check instead of checking for whether /tmp is shared with the host directly. We don't care so much about the latter, so I felt that encoding it in tests would make them more brittle as we explore alternative options.

@oquenchil
Copy link
Contributor

Thanks a lot for the PR Fabian.

#22226 was broken because of hermetic tmp.

The error used to be:

external/protobuf~/src/google/protobuf/port.cc:12:10: fatal error: google/protobuf/port_def.inc: No such file or directory
   12 | #include "google/protobuf/port_def.inc"

Now the error is:

Could not make proto path relative: bazel-out/k8-fastbuild/bin/external/protobuf~/src/google/protobuf/_virtual_imports/descriptor_proto/google/protobuf/descriptor.proto: No such file or directory

Do you think this is fixable in this PR?

@fmeum
Copy link
Collaborator Author

fmeum commented May 12, 2024 via email

@fmeum fmeum force-pushed the 21215-revert-hermetic-tmp-impl branch from 5204156 to 9ebdcc2 Compare May 14, 2024 13:35
@fmeum
Copy link
Collaborator Author

fmeum commented May 14, 2024

Tests are passing 🎉

Copy link
Contributor

@oquenchil oquenchil left a comment

Choose a reason for hiding this comment

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

Amazing contribution Fabian. A lot of bug fixes with simpler code than before 🥳

LGTM

@oquenchil oquenchil added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels May 15, 2024
@tjgq
Copy link
Contributor

tjgq commented May 16, 2024

This is amazing work, Fabian! Thank you.

@fmeum
Copy link
Collaborator Author

fmeum commented May 16, 2024

Just noticed that the PR description was out of date, I updated it.

@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label May 16, 2024
fmeum added a commit to fmeum/bazel that referenced this pull request May 16, 2024
The bind mounting scheme used with the Linux sandbox' hermetic `/tmp` feature is modified to preserve all paths as they are outside the sandbox, which removes the need to rewrite paths when staging inputs into and, crucially, moving outputs out of the sandbox.

Source roots and output base paths under `/tmp` are now treated just like any user-specified bind mount under `/tmp`: They are mounted under the hermetic tmp directory with their path relativized against `/tmp` before the hermetic tmp directory is mounted as `/tmp` as the final step.

There is one caveat compared to user-specified mounts: Source roots, which may themselves not lie under `/tmp`, can be symlinks to directories under `/tmp` (e.g., when they arise from a `local_repository`). To handle this situation in the common case, all parent directories of package path entries (up to direct children of `/tmp`) are mounted into the sandbox. If users use `local_repository`s with fixed target paths under `/tmp`, they will need to specify `--sandbox_add_mount_pair`.

Overlayfs has been considered as an alternative to this approach, but ultimately doesn't seem to work for this use case since its `lowerpath`, which would be `/tmp`, is not allowed to have child mounts from a different user namespace (see https://unix.stackexchange.com/questions/776030/mounting-overlayfs-in-a-user-namespace-with-child-mounts). However, this is exactly the situation created by a Bazel-in-Bazel test and can also arise if the user has existing mounts under `/tmp` when using Bazel (e.g. the JetBrains toolbox on Linux uses such mounts).

This replaces and mostly reverts the following commits, but keeps their tests:
* bazelbuild@bf6ebe9
* bazelbuild@fb6658c
* bazelbuild@bc1d9d3
* bazelbuild@1829883
* bazelbuild@70691f2
* bazelbuild@a556969
* bazelbuild@8e32f44 (had its test lost in an incorrect merge conflict resolution, this PR adds it back)

Fixes bazelbuild#20533
Work towards bazelbuild#20753
Fixes bazelbuild#21215
Fixes bazelbuild#22117
Fixes bazelbuild#22226
Fixes bazelbuild#22290

RELNOTES: Paths in the Linux sandbox are now again identical to those outside the sandbox, even with `--incompatible_sandbox_hermetic_tmp`.

Closes bazelbuild#22001.

PiperOrigin-RevId: 634381503
Change-Id: I9f7f3948c705be120c55c9b0c51204e5bea45f61
@fmeum fmeum deleted the 21215-revert-hermetic-tmp-impl branch May 16, 2024 14:55
github-merge-queue bot pushed a commit that referenced this pull request May 16, 2024
The bind mounting scheme used with the Linux sandbox' hermetic `/tmp`
feature is modified to preserve all paths as they are outside the
sandbox, which removes the need to rewrite paths when staging inputs
into and, crucially, moving outputs out of the sandbox.

Source roots and output base paths under `/tmp` are now treated just
like any user-specified bind mount under `/tmp`: They are mounted under
the hermetic tmp directory with their path relativized against `/tmp`
before the hermetic tmp directory is mounted as `/tmp` as the final
step.

There is one caveat compared to user-specified mounts: Source roots,
which may themselves not lie under `/tmp`, can be symlinks to
directories under `/tmp` (e.g., when they arise from a
`local_repository`). To handle this situation in the common case, all
parent directories of package path entries (up to direct children of
`/tmp`) are mounted into the sandbox. If users use `local_repository`s
with fixed target paths under `/tmp`, they will need to specify
`--sandbox_add_mount_pair`.

Overlayfs has been considered as an alternative to this approach, but
ultimately doesn't seem to work for this use case since its `lowerpath`,
which would be `/tmp`, is not allowed to have child mounts from a
different user namespace (see
https://unix.stackexchange.com/questions/776030/mounting-overlayfs-in-a-user-namespace-with-child-mounts).
However, this is exactly the situation created by a Bazel-in-Bazel test
and can also arise if the user has existing mounts under `/tmp` when
using Bazel (e.g. the JetBrains toolbox on Linux uses such mounts).

This replaces and mostly reverts the following commits, but keeps their
tests:
*
bf6ebe9
*
fb6658c
*
bc1d9d3
*
1829883
*
70691f2
*
a556969
*
8e32f44
(had its test lost in an incorrect merge conflict resolution, this PR
adds it back)

Fixes #20533
Work towards #20753
Fixes #21215
Fixes #22117
Fixes #22226
Fixes #22290

RELNOTES: Paths in the Linux sandbox are now again identical to those
outside the sandbox, even with `--incompatible_sandbox_hermetic_tmp`.

Closes #22001.

PiperOrigin-RevId: 634381503
Change-Id: I9f7f3948c705be120c55c9b0c51204e5bea45f61

Fixes #22291
rickystewart added a commit to rickystewart/cockroach that referenced this pull request Aug 2, 2024
This was apparently broken with the Bazel 7 upgrade and
bazelbuild/bazel#22001 specifically. If `--test_tmpdir` is set to
some directory under `/tmp`, we need to add `/tmp` as a mount pair as
well. This cannot be done in remote mode so `doctor` needs to be aware
of this.

Closes: cockroachdb#128204
Epic: None
Release note: None
Release justification: Build-only code changes
craig bot pushed a commit to cockroachdb/cockroach that referenced this pull request Aug 2, 2024
128207: dev: in `doctor`, add `--sandbox_add_mount_pair` if relevant r=rail a=rickystewart

This was apparently broken with the Bazel 7 upgrade and bazelbuild/bazel#22001 specifically. If `--test_tmpdir` is set to some directory under `/tmp`, we need to add `/tmp` as a mount pair as well. This cannot be done in remote mode so `doctor` needs to be aware of this.

Closes: #128204
Epic: None
Release note: None
Release justification: Build-only code changes

128211: dev: refactor prompts for autofix permission r=rail a=rickystewart

This code is duplicated in many places, so this refactor saves us some
LOC.

Epic: none
Release note: None
Release justification: Build-only code changes

Co-authored-by: Ricky Stewart <[email protected]>
blathers-crl bot pushed a commit to cockroachdb/cockroach that referenced this pull request Aug 2, 2024
This was apparently broken with the Bazel 7 upgrade and
bazelbuild/bazel#22001 specifically. If `--test_tmpdir` is set to
some directory under `/tmp`, we need to add `/tmp` as a mount pair as
well. This cannot be done in remote mode so `doctor` needs to be aware
of this.

Closes: #128204
Epic: None
Release note: None
Release justification: Build-only code changes
blathers-crl bot pushed a commit to cockroachdb/cockroach that referenced this pull request Aug 2, 2024
This was apparently broken with the Bazel 7 upgrade and
bazelbuild/bazel#22001 specifically. If `--test_tmpdir` is set to
some directory under `/tmp`, we need to add `/tmp` as a mount pair as
well. This cannot be done in remote mode so `doctor` needs to be aware
of this.

Closes: #128204
Epic: None
Release note: None
Release justification: Build-only code changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Local-Exec Issues and PRs for the Execution (Local) team team-Performance Issues for Performance teams
Projects
None yet
3 participants