Support `mounts` field on `docker_environment` #20322

thejcannon · 2023-12-20T19:21:09Z

This change adds a new mounts field to docker_environment to become mounts in the destination container.

The expected usage can be -anything, however the specific use-case that motivated this was mounting AWS credentials.

thejcannon · 2023-12-20T19:21:52Z

src/rust/engine/process_execution/src/lib.rs

+            // NB: We don't include the mount info in the cache.
+            Self::Docker(image, _) => format!("docker_execution: {image}"),


This is the only thing I'm not 100% on. Any thoughts?

From my local hacking, I think maybe including it in they key would be good for edits.

OK, so I changed this to use the bind mounts here, but I'm not seeing Pants re-start the container

Ah solved it

Keeping this open, since the original comment now applies again 😄

Omitting info from the process cache keys seems a bit dangerous, and something that has to be done carefully. (I'm not the right person to make a final call about the consequences.)

thejcannon · 2023-12-20T19:30:45Z

I'm also not sure the best way to test this.

Local tests show "it works":

Mounting a file, and seeing the file is readable in container
Trying to write to a mounted file, I get "Read-only file system"
Mounting a dir and seeing the dir is readable in container
Trying to write to a file in the mounted dir, I get "Read-only file system"

huonw

I'm also not sure the best way to test this.

What do you think about integration test(s) that run a test (e.g. experimental_test_shell_command) that checks a particular file provided by a bind mount has particular contents?

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

src/python/pants/core/util_rules/environments.py

thejcannon · 2023-12-20T20:54:53Z

I'm also not sure the best way to test this.

What do you think about integration test(s) that run a test (e.g. experimental_test_shell_command) that checks a particular file provided by a bind mount has particular contents?

I'd like that. We'd have to make sure we use a path that we know exists on the host OS however. Claude.ai seems to suggest /etc/passwd is such a file we could rely on, so I'll whip up that test

huonw · 2023-12-20T20:56:02Z

What about a temporary file that's created in the test? (It'll change from run, but that seems fine.)

thejcannon · 2023-12-20T20:57:44Z

I'm not sure that's doable. If we're using experimental_test_shell_command, we don't control the host environment.

Unless you're suggesting a Python-implemented integration test which runs Pants and uses docker_environment? In that case I'd rather just implement the test as a experimental_test_shell_command in the root BUILD of the project (or somewhere) and not deal with the added complexity of Pants-runs-Python-runs-Pants 😅

huonw · 2023-12-20T21:02:45Z

Unless you're suggesting a Python-implemented integration test which runs Pants and uses docker_environment?

Yeah, that's right. I was under the impression that a normal RuleRunner-based integration test would handle this fine, or do environments not work as expected there?

In that case I'd rather just implement the test as a experimental_test_shell_command in the root BUILD of the project (or somewhere) and not deal with the added complexity of Pants-runs-Python-runs-Pants 😅

I think a test like this won't do the right thing if/when we switch the Pants repo to be a normal Pants build root, using a previously released version to orchestrate the build of the new version of Pants (ala #19774 or #18688). In particular, a test like that will be using the orchestration-Pants docker environments, not validating the behaviour of new-Pants. That is, we'll need "orchestration-Pants"-runs-Python-runs-"new-Pants".

thejcannon · 2023-12-20T21:07:24Z

Unless you're suggesting a Python-implemented integration test which runs Pants and uses docker_environment?

Yeah, that's right. I was under the impression that a normal RuleRunner-based integration test would handle this fine, or do environments not work as expected there?

In that case I'd rather just implement the test as a experimental_test_shell_command in the root BUILD of the project (or somewhere) and not deal with the added complexity of Pants-runs-Python-runs-Pants 😅

I think a test like this won't do the right thing if/when we switch the Pants repo to be a normal Pants build root, using a previously released version to orchestrate the build of the new version of Pants (ala #19774 or #18688). In particular, a test like that will be using the orchestration-Pants docker environments, not validating the behaviour of new-Pants. That is, we'll need "orchestration-Pants"-runs-Python-runs-"new-Pants".

I don't think that's quite correct. In those situations we really should treat the "backends" we activate as the ones "in-repo" (similar to how we treat those in non-Pants repos) precisely for situations like this.

huonw · 2023-12-20T21:11:33Z

Ah, I see. That's quite surprising to me so I wasn't thinking along those lines. I'm curious how it'd work with the pants.core "backend" that provides the environments functionality.

src/python/pants/core/util_rules/environments.py

src/rust/engine/process_execution/src/lib.rs

thejcannon · 2023-12-21T16:45:36Z

Ah, I see. That's quite surprising to me so I wasn't thinking along those lines. I'm curious how it'd work with the pants.core "backend" that provides the environments functionality.

Me too 😄 Really there's a slider of how we want support to go which probably isn't worth going into detail in this PR other than to say "we aren't sure exactly how it'll look yet" if/when we get there.

huonw · 2023-12-21T21:42:27Z

Hm, okay. I'll register now that I think using the in-repo backends (especially pants.core) with an older released orchestrating pants seems like it'd be pretty surprising as a long term approach.

I won't block merging this with a test that relies on it, but it would be good to remember/make a note that that test will need reconsideration depending on the approach we take.

Also, this appears to be a new feature, rather than a bugfix. We're pretty late in the 2.19 release timeline (e.g. we document a 6 week release spacing, and we're at 5 weeks and 6 days since 2.18.0 right now!), what do you think about not cherry-picking and leaving it for 2.20 (either that, or cherry-pick for 2.19.1)?

huonw

This looks good other than a minor thing about the location of the test, and the still-open question about what should be included in the process cache keys.

BUILD

thejcannon · 2023-12-22T16:11:52Z

what do you think about not cherry-picking and leaving it for 2.20 (either that, or cherry-pick for 2.19.1)?

Yeah, we can hold off until 2.19.0 is out and then merge/cherry-pick for 2.19.1.

src/python/pants/backend/shell/goals/test.py

tdyas · 2023-12-26T21:09:16Z

src/python/pants/core/util_rules/environments.py

+        """
+        Mounts to use when starting the container.
+
+        The values should be in the form "<path on host>:<path on container>(:<options>)".


Is "path on host" correct when volume mounts are taken into consideration?

tdyas · 2023-12-26T21:11:32Z

src/rust/engine/src/externs/process.rs

+    }
+
+    #[getter]
+    fn docker_mounts(&self) -> Option<Vec<String>> {


Nit-pick: Maybe docker_mounts could return Option<Vec<&str>> (similar to docker_image's return value) to allow the caller to decide whether it needs a copy or not?

mbmccoy · 2024-01-25T16:20:35Z

One use case that I stumbled on today is mounting /var/run/docker.sock to allow the container to access a docker API without setting up a true "docker in docker" situation. My specific situation involves tests that spin up docker containers with a database in them.

thejcannon · 2024-01-25T16:22:21Z

This PR/branch is going stale for at least a few weeks if not months. So if anyone wants to pick it up, let me know.

benjyw · 2024-03-10T19:40:01Z

For now I will remove the needs-cherrypick label from this, since it's a new feature, so should not generally be cherry-picked to a stable branch.

thejcannon · 2024-03-11T14:49:30Z

I no longer require this. I'll close the PR. Others are welcome to pick it up.

thejcannon added 3 commits December 20, 2023 13:05

It works!

beaf9ef

read only

9f6547b

improve error message

13462fe

thejcannon added the category:new feature label Dec 20, 2023

thejcannon added this to the 2.19.x milestone Dec 20, 2023

thejcannon requested review from stuhood, tdyas and huonw December 20, 2023 19:21

thejcannon added the needs-cherrypick label Dec 20, 2023

thejcannon commented Dec 20, 2023

View reviewed changes

thejcannon added 2 commits December 20, 2023 13:35

cache the bind mounts

4d8df68

ok fix the cache

d3279b7

huonw reviewed Dec 20, 2023

View reviewed changes

src/rust/engine/process_execution/docker/src/docker.rs Outdated Show resolved Hide resolved

src/python/pants/core/util_rules/environments.py Outdated Show resolved Hide resolved

debian slim

b940f4c

tdyas reviewed Dec 20, 2023

View reviewed changes

src/python/pants/core/util_rules/environments.py Outdated Show resolved Hide resolved

src/python/pants/core/util_rules/environments.py Outdated Show resolved Hide resolved

src/rust/engine/process_execution/src/lib.rs Outdated Show resolved Hide resolved

thejcannon added 2 commits December 21, 2023 10:29

rename

0a2f217

mounts

bce5418

thejcannon changed the title ~~Support bind_mounts field on docker_environment~~ Support mounts field on docker_environment Dec 21, 2023

thejcannon added 2 commits December 21, 2023 10:50

open it up

a73f50e

fix some tests

ee59d13

huonw reviewed Dec 21, 2023

View reviewed changes

BUILD Outdated Show resolved Hide resolved

move it move it

2a00aa6

tdyas reviewed Dec 26, 2023

View reviewed changes

Merge branch 'main' into jcannon/bind_mounts

c31b93a

benjyw removed the needs-cherrypick label Mar 10, 2024

benjyw removed this from the 2.19.x milestone Mar 10, 2024

thejcannon closed this Mar 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `mounts` field on `docker_environment` #20322

Support `mounts` field on `docker_environment` #20322

thejcannon commented Dec 20, 2023 •

edited

Loading

thejcannon Dec 20, 2023

thejcannon Dec 20, 2023

thejcannon Dec 20, 2023

thejcannon Dec 20, 2023

huonw Dec 20, 2023

thejcannon commented Dec 20, 2023

huonw left a comment

thejcannon commented Dec 20, 2023

huonw commented Dec 20, 2023

thejcannon commented Dec 20, 2023

huonw commented Dec 20, 2023 •

edited

Loading

thejcannon commented Dec 20, 2023

huonw commented Dec 20, 2023

thejcannon commented Dec 21, 2023

huonw commented Dec 21, 2023

huonw left a comment

thejcannon commented Dec 22, 2023

tdyas Dec 26, 2023

tdyas Dec 26, 2023

mbmccoy commented Jan 25, 2024

thejcannon commented Jan 25, 2024

benjyw commented Mar 10, 2024

thejcannon commented Mar 11, 2024

		// NB: We don't include the mount info in the cache.
		Self::Docker(image, _) => format!("docker_execution: {image}"),

Support mounts field on docker_environment #20322

Support mounts field on docker_environment #20322

Conversation

thejcannon commented Dec 20, 2023 • edited Loading

thejcannon Dec 20, 2023

Choose a reason for hiding this comment

thejcannon Dec 20, 2023

Choose a reason for hiding this comment

thejcannon Dec 20, 2023

Choose a reason for hiding this comment

thejcannon Dec 20, 2023

Choose a reason for hiding this comment

huonw Dec 20, 2023

Choose a reason for hiding this comment

thejcannon commented Dec 20, 2023

huonw left a comment

Choose a reason for hiding this comment

thejcannon commented Dec 20, 2023

huonw commented Dec 20, 2023

thejcannon commented Dec 20, 2023

huonw commented Dec 20, 2023 • edited Loading

thejcannon commented Dec 20, 2023

huonw commented Dec 20, 2023

thejcannon commented Dec 21, 2023

huonw commented Dec 21, 2023

huonw left a comment

Choose a reason for hiding this comment

thejcannon commented Dec 22, 2023

tdyas Dec 26, 2023

Choose a reason for hiding this comment

tdyas Dec 26, 2023

Choose a reason for hiding this comment

mbmccoy commented Jan 25, 2024

thejcannon commented Jan 25, 2024

benjyw commented Mar 10, 2024

thejcannon commented Mar 11, 2024

Support `mounts` field on `docker_environment` #20322

Support `mounts` field on `docker_environment` #20322

thejcannon commented Dec 20, 2023 •

edited

Loading

huonw commented Dec 20, 2023 •

edited

Loading