Mount daemon Docker socket for nested workloads#148
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded VM-detection to Docker runtime and updated SocketPath behavior: for unix:// daemon hosts that match VM-style socket locations (Colima/OrbStack under user home) SocketPath now returns Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/runtime/docker_test.go (1)
50-55: Consider adding OrbStack socket path to test coverage.While the Colima test validates the behavior, adding an OrbStack-specific subtest would improve documentation of supported runtimes and ensure the path pattern is correctly handled.
📝 Suggested additional test case
t.Run("orbstack socket returns empty", func(t *testing.T) { cli, err := client.NewClientWithOpts(client.WithHost("unix:///Users/user/.orbstack/run/docker.sock")) require.NoError(t, err) rt := &DockerRuntime{client: cli} assert.Equal(t, "", rt.SocketPath(), "OrbStack sockets should return empty to skip bind-mount") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runtime/docker_test.go` around lines 50 - 55, Add a new subtest in the existing test block that verifies OrbStack socket paths are treated like Colima: create a Docker client via client.NewClientWithOpts(client.WithHost("unix:///Users/user/.orbstack/run/docker.sock")), assert no error, instantiate rt := &DockerRuntime{client: cli}, and assert rt.SocketPath() returns "" with the same message used in the other subtest; reference DockerRuntime and its SocketPath() method and reuse the test pattern already implemented for the Colima case to maintain consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/runtime/docker.go`:
- Around line 66-82: The doc comment for DockerRuntime.SocketPath is inverted
relative to the implementation: update the comment to state that it returns the
actual socket path for the standard /var/run/docker.sock (so callers can
bind-mount it) and returns an empty string for non-standard sockets (Colima,
OrbStack) which cannot be bind-mounted; reference the SocketPath method and its
use of d.client.DaemonHost() and the behavior relied on by
internal/container/start.go when rewording the comment to clearly describe this
behavior.
---
Nitpick comments:
In `@internal/runtime/docker_test.go`:
- Around line 50-55: Add a new subtest in the existing test block that verifies
OrbStack socket paths are treated like Colima: create a Docker client via
client.NewClientWithOpts(client.WithHost("unix:///Users/user/.orbstack/run/docker.sock")),
assert no error, instantiate rt := &DockerRuntime{client: cli}, and assert
rt.SocketPath() returns "" with the same message used in the other subtest;
reference DockerRuntime and its SocketPath() method and reuse the test pattern
already implemented for the Colima case to maintain consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 18e80b9b-5974-4aab-bdfd-0f597c6216e5
📒 Files selected for processing (3)
internal/runtime/docker.gointernal/runtime/docker_test.gotest/integration/start_test.go
ea31bd9 to
49f1d34
Compare
|
I'm not sure this is the actual fix for this :/ We have to bind mount the socket into the container because we need it for launching things like Lambda Functions. |
49f1d34 to
b61f9b3
Compare
|
Somehow the changes make LS start with Colima. Changed direction in b61f9b3, still works as expected. Let me know what you think! |
|
Yes they make it start, but the container won't work with Lambda 😀 |
b61f9b3 to
fe1e43f
Compare
|
Rebased to keep fresh. 🥗 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/runtime/docker.go`:
- Around line 71-75: The SocketPath implementation currently hard-codes
"/var/run/docker.sock" for any unix:// daemon host; instead parse
d.client.DaemonHost() and if it has the "unix://" prefix strip it and return the
actual socket path (e.g., "/run/user/<uid>/docker.sock"); only remap to
"/var/run/docker.sock" for cases where the daemon is actually in a VM (add or
use a VM detection helper such as DockerRuntime.isVM() or d.client.IsVM(),
creating one if missing) so internal/container/start.go:133 will bind the real
host socket path for rootless/custom setups and preserve the VM remap behavior
for Colima/OrbStack.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6aa586d3-83df-4071-914a-6f1bbfa1e0d3
📒 Files selected for processing (3)
internal/runtime/docker.gointernal/runtime/docker_test.gotest/integration/start_test.go
✅ Files skipped from review due to trivial changes (1)
- test/integration/start_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/runtime/docker_test.go
fe1e43f to
8b9109e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/runtime/docker_test.go`:
- Around line 74-79: The test mutates the real user HOME and creates fixed files
under /tmp; change the test to use an isolated temporary home and temp sockets:
in the t.Run("colima socket exists returns remapped path", ...) and other
subtests that currently call os.UserHomeDir(), replace that with a test-specific
temp dir from t.TempDir() and set HOME for the test (os.Setenv("HOME", tempDir))
or pass the tempDir explicitly when constructing colimaSock/orbstack paths
(colimaSock := filepath.Join(tempDir, ".colima", "default", "docker.sock") etc.
Likewise replace fixed /tmp filenames with files created inside t.TempDir() (or
use os.CreateTemp) to avoid collisions; ensure any environment changes are
restored after the test. Update occurrences referenced by symbol names
colimaSock, orbstackSock and the test subruns to use these temp paths.
In `@test/integration/start_test.go`:
- Around line 173-174: The test's bind-source assertion is too strict by
requiring "/var/run/docker.sock"; update the assertion to accept either the
literal "/var/run/docker.sock" or the runtime SocketPath() value so non-VM Unix
daemons pass. Locate the assertion using hasBindSource(inspect.HostConfig.Binds,
...) in start_test.go and change it to assert that hasBindSource(...) is true
for either "/var/run/docker.sock" or SocketPath(); update the failure message to
show inspect.HostConfig.Binds and the expected socket paths for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 2ffd9364-626b-4d37-ae1f-f3573fbd40f6
📒 Files selected for processing (3)
internal/runtime/docker.gointernal/runtime/docker_test.gotest/integration/start_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/runtime/docker.go
00340c8 to
e7ad21d
Compare
|
Rebased to keep fresh. 🥗 |
e7ad21d to
d1af720
Compare
|
Rebased to keep fresh. 🥗 |
This fixes nested-workload startup on VM-based Docker runtimes such as Colima and OrbStack.
The CLI was previously skipping the Docker socket bind-mount entirely for non-standard sockets (e.g. Colima), so Lambda and other nested workloads couldn't reach the daemon. This change detects VM-based runtimes and mounts
/var/run/docker.sock, the path the daemon exposes inside the VM. For rootless Docker or custom socket setups, the actual socket path is used.