Avoid bind-mounts for docker environments on macOS #18225

stuhood · 2023-02-11T00:22:12Z

Bind mounts in Docker for macOS have been implemented in a variety of ways over time, and are still in flux (https://www.cncf.io/blog/2023/02/02/docker-on-macos-is-slow-and-how-to-fix-it/ is a good overview of the chaos). But the gRPC FUSE implementation which is the default in the most recently released version at the time of writing (4.16.2) can suffer from race conditions where files which are created on the host inside a bind mount may not be visible to the container. This causes issues like #18162.

To avoid race conditions for file inputs, this change introduces a "pipe" IO strategy for Docker inputs, which uses a tar-pipe (from a tar-file stream written by the Store, to an exec of tar inside the container) to write process inputs. This strategy is used by default on macOS (for now), but the choice of strategy can be overridden.

The pipe IO strategy is about 30% slower for test running than the mount strategy. As we gather feedback from macOS users, we should be able to gain a clearer picture of which Docker for macOS versions and filesystem implementations can safely use the mount strategy.

Fixes #18162.

…atter to be used as tar input.

…sed, to avoid Docker bind mount confusion.

stuhood · 2023-02-13T22:52:24Z

Commits are useful to review independently: most of them are self explanatory, but the "Split prepare_workdir from prepare_workdir_digest" change is to allow for creating a complete digest of initial contents of a process's workdir, which can then be used consistently to materialize it either using materialize_directory or a tar-file.

…n using tar file inputs.

jsirois · 2023-02-14T16:31:52Z

src/rust/engine/process_execution/src/docker.rs

+          }
+          DockerStrategy::Pipe => {
+            // NB: For now, we continue to materialize files into a bind mount, but we do so from
+            // within the container. This allows us to capture outputs without an additional


This seems logically very likely. In my reading this is a known bug we're just waiting for to happen without calling the shot in an issue, maybe do that and we get to work on it? This still feels like shipping known (slightly less) broken software.

It seems possible, yes. But since we haven't observed it yet, I'm gambling... in the interest of saving time and complexity (I hope).

With my user hat on I definitely do not appreciate you gambling and not letting me know. I hope this will make a release announcement / acknowledgement - basically macOS Docker integration is known likely broken and you may see X.

If you want a semi-reliable manual way to expose the bug, clone https://github.com/tdyas/pants-go-testing and then run ./pants package race:racy_docker repeatedly.

(although "race" there refers to the Go data race detector, not this bug)

Mm, missed your comment before merging @tdyas : but thanks, will try it.

jsirois · 2023-02-14T16:52:29Z

src/rust/engine/process_execution/src/docker.rs

+    .start_exec(&exec.id, None)
+    .await
+    .map_err(|err| format!("Failed to start Docker execution `{}`: {:?}", &exec.id, err))?;
+  let StartExecResults::Attached { mut output, mut input } = exec_result else {


This syntax is new to me. I'd expect an if let would be needed to support the else at the end. I feel like I'm reading scala and space can invoke a method and else is a method or something.

Alrighty, pretty new: https://doc.rust-lang.org/nightly/rust-by-example/flow_control/let_else.html

This is let-else, but used with something other than Option: https://rust-lang.github.io/rfcs/3137-let-else.html

…d#18225)" This reverts commit c478a13.

…18247) Symlink support in `Digest`s landed after `2.15.x` was cut, and is too significant to cherry-pick. This reverts #18225. Will re-land a smaller change which can be more easily picked. This reverts commit c478a13.

Internal changes: * go: fix go vet test flakiness ([#18296](#18296) * Add default gha 1000:1000 user to GHA images. ([#18281](#18281)) * Don't specify interpreter_constraints when building the release pex. ([#18280](#18280)) * go: update pass-through `go test` options for Go v1.20 ([#18229](#18229)) * Fix release for /tmp on a seperate filesystem. ([#18272](#18272)) * upgrade to Rust v1.67.1 ([#18269](#18269)) * Prepare the 2.15.0rc6 release. ([#18263](#18263)) * Allow lambdas for `Field` and `Target` help ([#18248](#18248)) * Revert "Avoid bind-mounts for docker environments on macOS (#18225)" ([#18247](#18247)) * Refactors `experimental_shell_command`-related files to better separate concerns ([#18223](#18223)) * Prepare the 2.15.0rc5 release. ([#18224](#18224)) * Give dedicated threads names ([#18214](#18214)) * Prepare 2.15.0rc4. ([#18196](#18196)) * Remove the `boot_script` from `experimental_shell_command`/`experimental_run_in_sandbox` ([#18168](#18168)) * Adjusts `BinaryShims` to use native digest operations and `immutable_input_digests` ([#18184](#18184)) * go: update build tags check from latest go sources ([#18176](#18176)) * [internal] note dependency to `scie-pants` on "testing" env vars. ([#18170](#18170)) * go: allow coverage for standard library packages ([#18171](#18171)) * Support using `scie-pants` for Pants dev work. ([#18158](#18158)) * Prepare the 2.15.0rc3 release ([#18156](#18156)) * Splits up `shell_command.py` ([#18147](#18147))

…d#18225)" (pantsbuild#18247) Symlink support in `Digest`s landed after `2.15.x` was cut, and is too significant to cherry-pick. This reverts pantsbuild#18225. Will re-land a smaller change which can be more easily picked. This reverts commit c478a13.

Internal changes: * go: fix go vet test flakiness ([pantsbuild#18296](pantsbuild#18296) * Add default gha 1000:1000 user to GHA images. ([pantsbuild#18281](pantsbuild#18281)) * Don't specify interpreter_constraints when building the release pex. ([pantsbuild#18280](pantsbuild#18280)) * go: update pass-through `go test` options for Go v1.20 ([pantsbuild#18229](pantsbuild#18229)) * Fix release for /tmp on a seperate filesystem. ([pantsbuild#18272](pantsbuild#18272)) * upgrade to Rust v1.67.1 ([pantsbuild#18269](pantsbuild#18269)) * Prepare the 2.15.0rc6 release. ([pantsbuild#18263](pantsbuild#18263)) * Allow lambdas for `Field` and `Target` help ([pantsbuild#18248](pantsbuild#18248)) * Revert "Avoid bind-mounts for docker environments on macOS (pantsbuild#18225)" ([pantsbuild#18247](pantsbuild#18247)) * Refactors `experimental_shell_command`-related files to better separate concerns ([pantsbuild#18223](pantsbuild#18223)) * Prepare the 2.15.0rc5 release. ([pantsbuild#18224](pantsbuild#18224)) * Give dedicated threads names ([pantsbuild#18214](pantsbuild#18214)) * Prepare 2.15.0rc4. ([pantsbuild#18196](pantsbuild#18196)) * Remove the `boot_script` from `experimental_shell_command`/`experimental_run_in_sandbox` ([pantsbuild#18168](pantsbuild#18168)) * Adjusts `BinaryShims` to use native digest operations and `immutable_input_digests` ([pantsbuild#18184](pantsbuild#18184)) * go: update build tags check from latest go sources ([pantsbuild#18176](pantsbuild#18176)) * [internal] note dependency to `scie-pants` on "testing" env vars. ([pantsbuild#18170](pantsbuild#18170)) * go: allow coverage for standard library packages ([pantsbuild#18171](pantsbuild#18171)) * Support using `scie-pants` for Pants dev work. ([pantsbuild#18158](pantsbuild#18158)) * Prepare the 2.15.0rc3 release ([pantsbuild#18156](pantsbuild#18156)) * Splits up `shell_command.py` ([pantsbuild#18147](pantsbuild#18147))

stuhood added needs-cherrypick category:bugfix Bug fixes for released features labels Feb 11, 2023

stuhood added this to the 2.15.x milestone Feb 11, 2023

stuhood force-pushed the stuhood/tar-pipe-docker-macos branch 5 times, most recently from 76482c7 to caf3651 Compare February 12, 2023 22:06

stuhood added 2 commits February 12, 2023 14:36

Add a tar-file-creation method.

99015f2

Split prepare_workdir from prepare_workdir_digest, to allow the l…

6f82aaa

…atter to be used as tar input.

stuhood force-pushed the stuhood/tar-pipe-docker-macos branch 2 times, most recently from 0360272 to 5c3cacc Compare February 13, 2023 00:39

Ensure that the NamedCache base directory is created before it is u…

0fb5c7b

…sed, to avoid Docker bind mount confusion.

stuhood force-pushed the stuhood/tar-pipe-docker-macos branch from e7c27cb to 387f89f Compare February 13, 2023 22:33

stuhood requested review from huonw, jsirois and tdyas February 13, 2023 22:46

stuhood marked this pull request as ready for review February 13, 2023 22:47

stuhood marked this pull request as draft February 13, 2023 23:00

stuhood removed request for jsirois, tdyas and huonw February 13, 2023 23:01

This comment was marked as outdated.

Sign in to view

stuhood force-pushed the stuhood/tar-pipe-docker-macos branch 2 times, most recently from d2141fa to a55036d Compare February 14, 2023 00:03

stuhood requested review from huonw, jsirois and tdyas February 14, 2023 00:03

stuhood marked this pull request as ready for review February 14, 2023 00:04

stuhood added 2 commits February 13, 2023 16:14

Add a strategy to pipe inputs.

fcfd6ad

Increase the default value of rule_threads_max to avoid deadlocks whe…

9a36c6c

…n using tar file inputs.

stuhood force-pushed the stuhood/tar-pipe-docker-macos branch from a55036d to 9a36c6c Compare February 14, 2023 00:15

Increase grace period of unrelated docker timeout test.

3e58b7a

jsirois approved these changes Feb 14, 2023

View reviewed changes

stuhood merged commit c478a13 into pantsbuild:main Feb 14, 2023

stuhood deleted the stuhood/tar-pipe-docker-macos branch February 14, 2023 17:28

stuhood added a commit to stuhood/pants that referenced this pull request Feb 14, 2023

Revert "Avoid bind-mounts for docker environments on macOS (pantsbuil…

1d5333d

…d#18225)" This reverts commit c478a13.

This was referenced Feb 14, 2023

Testing in Docker environment fails with pex FileNotFoundError #18162

Closed

Revert "Avoid bind-mounts for docker environments on macOS (#18225)" #18247

Merged

stuhood removed the needs-cherrypick label Feb 16, 2023

illicitonion mentioned this pull request Feb 17, 2023

Prep for 2.16.0.dev7 #18297

Merged

stuhood mentioned this pull request Feb 22, 2023

Docker environment may leave behind partially-root-owned sandboxes on Linux #18329

Open

stuhood mentioned this pull request Mar 11, 2023

Finish parallelizing materialization of Process inputs #18469

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid bind-mounts for docker environments on macOS #18225

Avoid bind-mounts for docker environments on macOS #18225

stuhood commented Feb 11, 2023 •

edited

Loading

stuhood commented Feb 13, 2023

This comment was marked as outdated.

jsirois Feb 14, 2023

stuhood Feb 14, 2023

jsirois Feb 14, 2023

tdyas Feb 14, 2023 •

edited

Loading

tdyas Feb 14, 2023

stuhood Feb 14, 2023

jsirois Feb 14, 2023

jsirois Feb 14, 2023

stuhood Feb 14, 2023

Avoid bind-mounts for docker environments on macOS #18225

Avoid bind-mounts for docker environments on macOS #18225

Conversation

stuhood commented Feb 11, 2023 • edited Loading

stuhood commented Feb 13, 2023

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdyas Feb 14, 2023 • 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

Choose a reason for hiding this comment

stuhood commented Feb 11, 2023 •

edited

Loading

tdyas Feb 14, 2023 •

edited

Loading