diff --git a/backend/tests/unit/onyx/server/features/craft/sandbox/test_docker_manager_config.py b/backend/tests/unit/onyx/server/features/craft/sandbox/test_docker_manager_config.py index 1bd8e1509a..3f95f262d3 100644 --- a/backend/tests/unit/onyx/server/features/craft/sandbox/test_docker_manager_config.py +++ b/backend/tests/unit/onyx/server/features/craft/sandbox/test_docker_manager_config.py @@ -844,3 +844,63 @@ def test_proxy_kwargs_requires_ca_volume() -> None: sandbox_proxy_host="sandbox-proxy", proxy_ca_volume_name=None, ) + + +def _craft_compose_services() -> dict: + compose_path = REPO_ROOT / "deployment/docker_compose/docker-compose.craft.yml" + return yaml.safe_load(compose_path.read_text())["services"] + + +def test_compose_prepulls_the_image_the_sandboxes_run() -> None: + """The prepull entry exists so `docker compose pull` fetches the sandbox + image, and must name the *same* ref the sandbox containers get. A drifted + ref warms an image nobody runs while every sandbox still cold-pulls, and + nothing looks wrong — the only symptom is the slow provision it exists to + remove. + """ + services = _craft_compose_services() + prepull_image = services["sandbox-image-prepull"]["image"] + + for service_name in ("api_server", "background"): + assert ( + f"SANDBOX_CONTAINER_IMAGE={prepull_image}" + in services[service_name]["environment"] + ) + + +def test_compose_prepull_never_starts_a_container() -> None: + """It is an image reference, not a workload. `replicas: 0` keeps `up` from + creating anything, so nothing idles doing nothing and `up --wait` — which + install.sh passes by default — stays green. A one-shot that exited would + fail that wait; a long-lived idler would leave a pointless container. + """ + prepull = _craft_compose_services()["sandbox-image-prepull"] + + assert prepull["deploy"]["replicas"] == 0 + # No socket, no volumes, no sandbox bridge: nothing is supposed to run. + for key in ("volumes", "networks", "restart"): + assert key not in prepull + + +def test_compose_prepull_is_inert_even_if_it_does_start() -> None: + """`deploy` is ignored by the legacy standalone docker-compose, which + install.sh accepts with no minimum version. If `replicas: 0` is dropped, a + container *is* created — and this image's own ENTRYPOINT starts + `opencode serve` on 0.0.0.0:4096 with no password, staying up so nothing + looks broken. The override is the only thing standing between a dropped + `deploy` key and an unauthenticated agent server on the compose network. + """ + prepull = _craft_compose_services()["sandbox-image-prepull"] + + assert prepull["entrypoint"] == ["/bin/true"] + + +def test_embedded_craft_compose_copy_is_in_sync() -> None: + """onyx-cli go:embeds a byte-identical copy (go:embed can't reach outside + the cli module). See tools/ods/internal/deployfilessync.""" + source = REPO_ROOT / "deployment/docker_compose/docker-compose.craft.yml" + embedded = ( + REPO_ROOT + / "cli/internal/deploy/deployfiles/embedded/docker_compose/docker-compose.craft.yml" + ) + assert embedded.read_text() == source.read_text() diff --git a/cli/internal/deploy/deployfiles/embedded/docker_compose/docker-compose.craft.yml b/cli/internal/deploy/deployfiles/embedded/docker_compose/docker-compose.craft.yml index cc31bca154..f8a735109b 100644 --- a/cli/internal/deploy/deployfiles/embedded/docker_compose/docker-compose.craft.yml +++ b/cli/internal/deploy/deployfiles/embedded/docker_compose/docker-compose.craft.yml @@ -127,6 +127,28 @@ services: max-size: "50m" max-file: "6" + # Declared solely so `docker compose pull` fetches the sandbox image. Without + # it the first sandbox pays the ~1 GB download inside its provisioning + # request, because api_server creates sandbox containers from this image + # directly rather than compose managing them. + # + # replicas: 0 means no container is ever created -- `up` skips it, so nothing + # idles and `up --wait` stays green. It is not a workload; it is an image + # reference that the normal pull/upgrade lifecycle can see. + sandbox-image-prepull: + # Must match api_server/background's SANDBOX_CONTAINER_IMAGE below, or this + # warms an image nobody runs while every sandbox still cold-pulls. + image: ${SANDBOX_CONTAINER_IMAGE:-onyxdotapp/sandbox:${IMAGE_TAG:-latest}} + deploy: + replicas: 0 + # Defence in depth. `replicas: 0` is honoured by Compose v2 (even when the + # service is named explicitly), but the legacy standalone docker-compose + # ignores `deploy` outside swarm, and install.sh accepts a standalone binary + # with no minimum version. Unoverridden, the image's own ENTRYPOINT starts + # `opencode serve` on 0.0.0.0:4096 with no password -- and stays running, so + # nothing looks wrong. Anything that does start here must be inert. + entrypoint: ["/bin/true"] + networks: # Pre-created by install.sh --include-craft. onyx_craft_sandbox: diff --git a/deployment/docker_compose/docker-compose.craft.yml b/deployment/docker_compose/docker-compose.craft.yml index cc31bca154..f8a735109b 100644 --- a/deployment/docker_compose/docker-compose.craft.yml +++ b/deployment/docker_compose/docker-compose.craft.yml @@ -127,6 +127,28 @@ services: max-size: "50m" max-file: "6" + # Declared solely so `docker compose pull` fetches the sandbox image. Without + # it the first sandbox pays the ~1 GB download inside its provisioning + # request, because api_server creates sandbox containers from this image + # directly rather than compose managing them. + # + # replicas: 0 means no container is ever created -- `up` skips it, so nothing + # idles and `up --wait` stays green. It is not a workload; it is an image + # reference that the normal pull/upgrade lifecycle can see. + sandbox-image-prepull: + # Must match api_server/background's SANDBOX_CONTAINER_IMAGE below, or this + # warms an image nobody runs while every sandbox still cold-pulls. + image: ${SANDBOX_CONTAINER_IMAGE:-onyxdotapp/sandbox:${IMAGE_TAG:-latest}} + deploy: + replicas: 0 + # Defence in depth. `replicas: 0` is honoured by Compose v2 (even when the + # service is named explicitly), but the legacy standalone docker-compose + # ignores `deploy` outside swarm, and install.sh accepts a standalone binary + # with no minimum version. Unoverridden, the image's own ENTRYPOINT starts + # `opencode serve` on 0.0.0.0:4096 with no password -- and stays running, so + # nothing looks wrong. Anything that does start here must be inert. + entrypoint: ["/bin/true"] + networks: # Pre-created by install.sh --include-craft. onyx_craft_sandbox: diff --git a/docs/craft/sandbox/image-and-spinup.md b/docs/craft/sandbox/image-and-spinup.md index 1172cc9df8..9e6b01a229 100644 --- a/docs/craft/sandbox/image-and-spinup.md +++ b/docs/craft/sandbox/image-and-spinup.md @@ -122,10 +122,12 @@ so the prod image still has it but dev/CI images can opt out. ## Cold pulls vs. image warming — decision and roadmap -**Current state: we pre-pull the sandbox image onto the sandbox node -pool with a DaemonSet.** Chart: `sandboxImagePrepull` in `values.yaml`, -template `sandbox-image-prepuller.yaml`. On by default when -`ENABLE_CRAFT` is set. +**Current state: we pre-pull the sandbox image on both backends.** +Kubernetes uses a DaemonSet over the sandbox node pool +(`sandboxImagePrepull` in `values.yaml`, template +`sandbox-image-prepuller.yaml`); Docker declares the image in +`docker-compose.craft.yml` so `docker compose pull` fetches it — see +"Docker" below. Both on by default when `ENABLE_CRAFT` is set. This section previously recorded the opposite decision — that we accept cold pulls — on an estimate of ~3–6 s per pull from AZ-local bandwidth. @@ -263,6 +265,77 @@ does a fresh install into an empty kind cluster, never an upgrade from the previously released chart, so the patch path where immutable-field violations live is untested. `ct install --upgrade` would cover it. +### Docker + +Same problem, materially easier shape. The Docker backend pulls lazily in +`DockerSandboxManager._ensure_sandbox_image()`, called from +`provision()` — so on a host without the image, whoever asks for the +first sandbox pays the ~1 GB download inside their own request. + +Two differences from Kubernetes drive a different fix: + +- **One image store, not one per node.** A compose deployment is a single + Docker daemon, so a single pull serves `api_server`, `background`, and + every sandbox container. There is no per-node fan-out to arrange, and + no `maxUnavailable` to tune. +- **Docker never garbage-collects images.** kubelet reclaims them at + `imageGCHighThresholdPercent`, which is why the Kubernetes fix needs a + *running* pod to pin the layers. Docker only removes images on an + explicit `docker image prune`. Pinning is therefore a bonus here, not + the point. + +So the fix is a `sandbox-image-prepull` entry in +`docker-compose.craft.yml` carrying `deploy.replicas: 0`. The sandbox +image is not otherwise a compose service — `api_server` creates sandbox +containers from it directly — so `docker compose pull` has nothing to +tell it about. Declaring it puts the image on the normal pull/upgrade +lifecycle; `replicas: 0` means no container is ever created. + +That combination is load-bearing and was verified rather than assumed: + +| shape | `compose pull` fetches it | `up --wait` | +|---|---|---| +| `profiles: [...]` | **no** — skipped | n/a | +| one-shot that exits 0 | yes | **exits 1** | +| long-lived idler | yes | ok, but a pointless container forever | +| `deploy.replicas: 0` | yes | ok, no container at all | + +So profiles can't be used (a plain `pull` skips them, which defeats the +point for anyone not using the installer), and a one-shot breaks the +`up --wait` that `install.sh` passes by default. + +**One mechanism, and it covers every path.** `install.sh --include-craft` +already runs `docker compose pull` with the craft overlay layered in, so +the installer needs no special case. Anyone who brings the stack up by +hand — a supported path, see `deployment/docker_compose/README.md` option +2 — gets it from the same plain `docker compose pull`, with nothing extra +to remember. + +**One fallback.** If the image is missing later (pruned, or an operator +who never pulls), `provision()` pulls it on demand as it always has. The +cost lands on one request and the error surfaces against the request that +caused it. + +Earlier revisions added a long-lived compose service to hold the image +*and* a background warm at api_server startup. Both were removed. They +were mutually redundant — each was justified by gaps the other covered — +and neither survives the question "if the other exists, why do I?". The +long-lived service also left a container idling forever for no reason +beyond satisfying `docker compose up --wait` — which `replicas: 0` sidesteps +entirely, since there is nothing for the wait to observe. + +Do not warm at api_server startup. Blocking there would put all of Onyx's +readiness behind the registry, and not just on first install: `:latest` +is the default and is in `_MUTABLE_SANDBOX_IMAGE_TAGS`, for which +`_ensure_sandbox_image` skips the local-presence check and always pulls. +Every restart would contact the registry. Making it non-blocking instead +just converts a broken deployment into a slow one that fails later, at a +user's first sandbox. + +The `background` worker also provisions (waking `SLEEPING` sandboxes via +`scheduled_tasks/executor.py`) and needs nothing of its own: it shares +the host's image store, so whichever pull happened first covers it. + ### Private registries Relevant only if you mirror the sandbox image into your own registry;