Skip to content

Mount daemon Docker socket for nested workloads#148

Open
gtsiolis wants to merge 5 commits intomainfrom
george/des-179
Open

Mount daemon Docker socket for nested workloads#148
gtsiolis wants to merge 5 commits intomainfrom
george/des-179

Conversation

@gtsiolis
Copy link
Member

@gtsiolis gtsiolis commented Mar 20, 2026

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.

@coderabbitai
Copy link

coderabbitai bot commented Mar 20, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Added 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 /var/run/docker.sock; otherwise it returns the extracted unix socket path. Tests and an integration helper were added/updated.

Changes

Cohort / File(s) Summary
Docker runtime implementation & unit tests
internal/runtime/docker.go, internal/runtime/docker_test.go
Added (*DockerRuntime).isVM() to detect VM-style unix sockets; SocketPath() now maps detected VM socket paths to /var/run/docker.sock and otherwise returns the extracted unix socket path. Unit tests expanded into subtests and add VM-detection cases.
Integration test: start command
test/integration/start_test.go
Test now asserts the host-side source of the Docker socket bind is /var/run/docker.sock as well as the container target; added helper hasBindSource to check bind sources.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • carole-lavillonniere
  • anisaoshafi
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Mount daemon Docker socket for nested workloads' accurately describes the main change: routing Docker socket mounts through the VM daemon instead of the client socket for nested workloads on VM-based Docker runtimes.
Description check ✅ Passed The PR description is directly related to the changeset, explaining the specific problem (nested-workload startup on VM-based Docker runtimes), the root cause (non-standard socket paths), and the solution (detecting VM runtimes and mounting the appropriate socket path).

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch george/des-179

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 9925dae and ea31bd9.

📒 Files selected for processing (3)
  • internal/runtime/docker.go
  • internal/runtime/docker_test.go
  • test/integration/start_test.go

@silv-io
Copy link
Member

silv-io commented Mar 20, 2026

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.

@gtsiolis
Copy link
Member Author

Somehow the changes make LS start with Colima.

Changed direction in b61f9b3, still works as expected.

Let me know what you think!

Copy link
Member

silv-io commented Mar 20, 2026

Yes they make it start, but the container won't work with Lambda 😀

@gtsiolis gtsiolis changed the title Skip Docker socket bind for Colima and OrbStack Mount daemon Docker socket for nested workloads Mar 20, 2026
@gtsiolis
Copy link
Member Author

Rebased to keep fresh. 🥗

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between b61f9b3 and fe1e43f.

📒 Files selected for processing (3)
  • internal/runtime/docker.go
  • internal/runtime/docker_test.go
  • test/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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between fe1e43f and 8b9109e.

📒 Files selected for processing (3)
  • internal/runtime/docker.go
  • internal/runtime/docker_test.go
  • test/integration/start_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/runtime/docker.go

@gtsiolis gtsiolis force-pushed the george/des-179 branch 2 times, most recently from 00340c8 to e7ad21d Compare March 23, 2026 14:57
@gtsiolis
Copy link
Member Author

Rebased to keep fresh. 🥗

@gtsiolis
Copy link
Member Author

Rebased to keep fresh. 🥗

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants