Skip to content

chore(localmode): sync compose template with volcano-hosting - #156

Open
tkkhq wants to merge 2 commits into
mainfrom
tkkhq/vol-788-sync-compose-asset
Open

chore(localmode): sync compose template with volcano-hosting#156
tkkhq wants to merge 2 commits into
mainfrom
tkkhq/vol-788-sync-compose-asset

Conversation

@tkkhq

@tkkhq tkkhq commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Tracking

Summary

Regenerates the embedded local-mode Compose asset from volcano-hosting, so volcano start produces a server that can launch a v2 agent worker.

  • adds the /var/run/docker.sock mount on the server service, which the local-mode server needs to start the agent worker as a sibling container
  • generated by make volcano-cli-localmode-assets-generate in volcano-hosting; the file is not hand-edited

volcano-hosting owns infrastructure/local-mode/docker-compose.template.yml. This repository embeds a generated copy so the CLI can start the stack without a hosting checkout. Hosting PR #807 changed the template, which left this copy stale: a developer running volcano start got a server that silently could not launch a worker.

The mount is local-mode only. It grants the server container the ability to start containers, which is the local equivalent of the ECS RunTask permission the Fargate launcher holds in cloud.

Merge order

Land volcano-hosting PR #807 first, so the source template is final.

Verification

  • make volcano-cli-localmode-assets-check in volcano-hosting, pointed at this branch -> volcano-cli local-mode assets check passed
  • go build ./... -> pass
  • go vet ./... -> pass
  • go test ./... -count=1 -> all pass
  • git diff --check -> clean
  • the diff is limited to the server service volumes block; no other service, port, or volume changed

@tkkhq
tkkhq requested a review from a team as a code owner August 9, 2026 21:22
Copilot AI balanced review requested due to automatic review settings August 9, 2026 21:22

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Syncs the generated local-mode Compose asset with volcano-hosting to enable v2 agent worker containers.

Changes:

  • Mounts the host Docker socket into the server.
  • Retains the existing local-storage volume.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

# WARNING: This grants the server container permission to run any container
# with the server's privileges. Only safe in local development on your machine.
volumes:
- /var/run/docker.sock:/var/run/docker.sock
Comment on lines +76 to +78
# Mount docker socket to allow agent worker launch (local mode only).
# WARNING: This grants the server container permission to run any container
# with the server's privileges. Only safe in local development on your machine.

@tkkhq tkkhq left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See inline feedback.

Verdict: Approve — The diff is a minimal, correctly-scoped sync of a generated Compose asset; no P0/P1 defects were found, and the one P2 finding is a documentation-accuracy nit in a generated file, not a functional or blocking issue.

Comment on lines +76 to +78
# Mount docker socket to allow agent worker launch (local mode only).
# WARNING: This grants the server container permission to run any container
# with the server's privileges. Only safe in local development on your machine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[P2] Docker-socket warning understates actual blast radius

The new comment at lines 76-78 says the socket mount "grants the server container permission to run any container with the server's privileges." That understates the real risk: /var/run/docker.sock talks to the host Docker daemon, which normally runs as root. Any process with access to the socket can ask the daemon to start a new container with --privileged and a host-root bind mount (e.g. -v /:/host), giving effective root on the host, regardless of what privilege level the server process itself runs at inside its own container. The warning's phrasing could lead a developer to believe the blast radius is capped at "whatever the server can already do," when it is actually host-root-equivalent.

Since this file is generated ("Do not edit by hand") from volcano-hosting's infrastructure/local-mode/docker-compose.template.yml, the wording fix belongs in that upstream template, not this copy.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not changing here — this file is generated verbatim from volcano-hosting's infrastructure/local-mode/docker-compose.template.yml ("Do not edit by hand"), so a wording fix in this synced copy would be overwritten on the next sync. The blast-radius point is valid; it belongs in the upstream volcano-hosting template, not this PR.

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