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

Add support for bind mounts under /tmp with hermetic tmp #20583

Closed
wants to merge 2 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Dec 18, 2023

This is achieved by rewriting the user-specified mounts to mounts onto subdirectories of the hermetic sandbox tmp directory.

Fixes #20527

@fmeum fmeum requested a review from lberki December 18, 2023 08:50
@github-actions github-actions bot added team-Local-Exec Issues and PRs for the Execution (Local) team awaiting-review PR is awaiting review from an assigned reviewer labels Dec 18, 2023
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 18, 2023

@lberki Happy to implement any other kind of fix if you can think of one that doesn't require disabling hermetic tmp in this case.

@fmeum fmeum force-pushed the 20527-add-mount-pair branch from 6f24a98 to 551606e Compare December 19, 2023 17:23
@fmeum fmeum changed the title Disable hermetic /tmp with conflicting mount pairs Add more tests for /tmp sandbox mount pairs Dec 19, 2023
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 19, 2023

@lberki Both tests pass with --noincompatible_sandbox_hermetic_tmp.

@lberki
Copy link
Contributor

lberki commented Dec 19, 2023

Disabling hermetic /tmp isn't a very good idea because it wouldn't let us delete the code path supporting --noincompatible_sandbox_hermetic_tmp. I have three ideas:

  1. Error out with a mount point under /tmp
  2. Change the order of mounts: if a mount point is under /tmp, mount it after we mount the hermetic /tmp here
  3. Rewrite the mount target so that it's under the directory that we eventually mount under /tmp

(2) and (3) contain some amount of DWIM, which could come back to bite us: for example, I could imagine someone creative mounting /foo under /tmp/foo, then /tmp/foo under /bar, which would not work with the reshuffling proposed.

I think the only theoretically perfect solution is either (1) or to apply some cleverness to rewrite or reorder the mount specifications such that they do they right thing. But I don't like cleverness so my preferred option is (1).

@fmeum
Copy link
Collaborator Author

fmeum commented Dec 19, 2023

I also favor 1, but what about the other code paths that disable the feature, e.g. a tmpfs mount under /tmp/foo? We also have to continue to support --sandbox_add_mount_pair=/tmp - can we even get rid of the alternate code path?

@lberki
Copy link
Contributor

lberki commented Dec 20, 2023

Why do we have to continue supporting --sandbox_add_mount_pair=/tmp ?

@fmeum
Copy link
Collaborator Author

fmeum commented Dec 20, 2023

Why do we have to continue supporting --sandbox_add_mount_pair=/tmp ?

There are cases where you would want your test to be able to communicate with some external process (such as Docker) via a socket in /tmp. It would be mildly surprising if --sandbox_add_mount_pair would work for essentially any path except for /tmp (or anything underneath).

@lberki
Copy link
Contributor

lberki commented Dec 20, 2023

Fair enough. Mind taking a stab at implementing the clever reordering / rewriting I mentioned in one of the above comments then? It doesn't look impossible or fundamentally difficult, it was just cleverness I had hoped we could live without.

@fmeum
Copy link
Collaborator Author

fmeum commented Dec 20, 2023

Sure, I will give it a try.

@lberki
Copy link
Contributor

lberki commented Dec 20, 2023

Thanks, let me know when you need my eyes (I'm OOO from Friday until the end of this year, though)

@fmeum fmeum force-pushed the 20527-add-mount-pair branch from 551606e to 816ebca Compare December 21, 2023 17:02
@fmeum fmeum changed the title Add more tests for /tmp sandbox mount pairs Add support for bind mounts under /tmp with hermetic tmp Dec 21, 2023
@fmeum fmeum force-pushed the 20527-add-mount-pair branch from 816ebca to 91d0d37 Compare December 21, 2023 17:03
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 21, 2023

@lberki I implemented the rewriting and my test cases pass now. Are there any more scenarios you would like me to add tests for?

@sgowroji sgowroji added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Dec 22, 2023
@fmeum fmeum added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Dec 22, 2023
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 22, 2023

@bazel-io fork 7.0.1

This is achieved by rewriting the user-specified mounts to mounts onto
subdirectories of the hermetic sandbox tmp directory.
@fmeum fmeum force-pushed the 20527-add-mount-pair branch from 91d0d37 to 9aa90a0 Compare December 22, 2023 08:23
Copy link
Contributor

@lberki lberki left a comment

Choose a reason for hiding this comment

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

Looks very reasonable. I have one nit, but I could even be convinced that that one is not important.

@fmeum fmeum force-pushed the 20527-add-mount-pair branch from ee5a5dd to 64c8693 Compare January 2, 2024 09:19
@lberki lberki 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 Jan 2, 2024
@iancha1992
Copy link
Member

@fmeum @lberki the tests keep failing. Can you please take a look?
cc: @bazelbuild/triage

@lberki
Copy link
Contributor

lberki commented Jan 3, 2024

For the Open JDK 11 / Mac OS tests (//src/test/py/bazel:py_test), this change needs to be cherrry-picked to 7.0.1: d9dc4fd

for the rest, I don't know -- maybe @meteorcloudy or @fweikert or @Wyverald do?

@Wyverald
Copy link
Member

Wyverald commented Jan 3, 2024

The test failures are due to our CI machine upgrades and will be gone when we rebase this PR on top of master. (No need to do that; we're already doing the import internally.)

@copybara-service copybara-service bot closed this in 5e68afd Jan 4, 2024
@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 Jan 4, 2024
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Jan 4, 2024
This is achieved by rewriting the user-specified mounts to mounts onto subdirectories of the hermetic sandbox tmp directory.

Fixes bazelbuild#20527

Closes bazelbuild#20583.

PiperOrigin-RevId: 595815029
Change-Id: Ibfe5f67fb8fb59131b6c82a826ed5200f2b10a94
iancha1992 pushed a commit that referenced this pull request Jan 5, 2024
…20749)

This is achieved by rewriting the user-specified mounts to mounts onto
subdirectories of the hermetic sandbox tmp directory.

Fixes #20527

Closes #20583.

Commit
5e68afd

PiperOrigin-RevId: 595815029
Change-Id: Ibfe5f67fb8fb59131b6c82a826ed5200f2b10a94

Co-authored-by: Fabian Meumertzheim <[email protected]>
@fmeum fmeum deleted the 20527-add-mount-pair branch January 5, 2024 08:42
@iancha1992
Copy link
Member

@bazel-io fork 7.1.0

bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Jan 6, 2024
This is achieved by rewriting the user-specified mounts to mounts onto subdirectories of the hermetic sandbox tmp directory.

Fixes bazelbuild#20527

Closes bazelbuild#20583.

PiperOrigin-RevId: 595815029
Change-Id: Ibfe5f67fb8fb59131b6c82a826ed5200f2b10a94
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Jan 6, 2024
This is achieved by rewriting the user-specified mounts to mounts onto subdirectories of the hermetic sandbox tmp directory.

Fixes bazelbuild#20527

Closes bazelbuild#20583.

PiperOrigin-RevId: 595815029
Change-Id: Ibfe5f67fb8fb59131b6c82a826ed5200f2b10a94
github-merge-queue bot pushed a commit that referenced this pull request Jan 12, 2024
…20772)

This is achieved by rewriting the user-specified mounts to mounts onto
subdirectories of the hermetic sandbox tmp directory.

Fixes #20527

Closes #20583.

Commit
5e68afd

PiperOrigin-RevId: 595815029
Change-Id: Ibfe5f67fb8fb59131b6c82a826ed5200f2b10a94

Co-authored-by: Fabian Meumertzheim <[email protected]>
Co-authored-by: lberki <[email protected]>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bazel 7: --sandbox_add_mount_pair under /tmp fails
5 participants