fix(engine): pull an image before ContainerCreate if it isn't already local - #5
Merged
Conversation
… local internal/engine.Docker.Start and StartJob both called ContainerCreate directly - unlike `docker run`, the Engine API never auto-pulls a missing image, so this failed outright with "No such image" on any node that hadn't already cached it. Never surfaced before because every node Sous had run on already had its images cached (from the single-node Sous era, or from this fleet's own precedent of pre-pulling vLLM images by hand via adhoc scripts). Surfaced for real tonight on aorus-ubuntu's first-ever deployment - a genuinely cold Docker install - and was worked around operationally (manual pre-pull) rather than fixed in code at the time, since this touches the same deploy path every node in the fleet uses, including the currently-serving asus-gx10. New ensureImage(ctx, ref) checks ImageInspect first (matching `docker run`'s own default of pulling only when missing, and avoiding a needless registry round-trip on the common case where a digest-pinned recipe image is already cached) and only calls ImagePull if genuinely absent. The subtle part - draining ImagePull's response stream - is split into drainPullStream and unit-tested directly against crafted byte streams, no real Docker/network access needed: a plain io.Copy(io.Discard, r) would silently report success on a pull that actually failed partway through, since a registry-side failure (bad ref, auth, missing manifest) arrives as an "error" field INSIDE the JSON progress stream, not as a Go error from ImagePull itself. Confirmed empirically (see the commit's own test) that a naive io.Copy-based drain does exactly that on a stream carrying a real embedded pull error. One additional real integration test (skips cleanly without a reachable Docker daemon or without the fixture image already cached, rather than failing an environment without Docker access) exercises ensureImage's already-cached fast path against this sandbox's actual local daemon. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
`internal/engine.Docker.Start`/`StartJob` both called `ContainerCreate` directly - the Engine API never auto-pulls a missing image the way `docker run` does, so this failed outright with "No such image" on any node without it already cached. Never surfaced before tonight because every node Sous has run on already had its images cached; surfaced for real on aorus-ubuntu's first-ever deployment (a genuinely cold Docker install) and was worked around operationally at the time. Now fixed properly in code, per explicit request, since this touches the deploy path every node in the fleet shares, including the currently-serving asus-gx10.
New `ensureImage` checks `ImageInspect` first (skip pulling if already present - matches `docker run`'s own default, and recipe images are digest-pinned so "cached" and "correct" are the same fact) and only pulls if genuinely missing. The subtle part - draining `ImagePull`'s response stream, where a registry-side failure arrives as an in-stream JSON `"error"` field rather than a Go error - is split into `drainPullStream` and unit-tested directly, no real Docker/network access needed. One real integration test exercises the already-cached fast path against an actual local daemon, skipping cleanly if none is reachable.
Reviewed: safe to merge, no Critical/Important findings. Reviewer independently verified `drainPullStream`'s contract against the actual `docker/docker` client source, empirically confirmed (via a throwaway standalone program) that a naive `io.Copy`-based drain silently swallows the exact mid-stream error class the real test catches, confirmed the integration test genuinely runs against a live daemon rather than silently skipping, and confirmed no other `ContainerCreate` call site was missed. go build/vet/test clean, 544/28 passing (+5 new tests). One pre-existing Minor noted (a few catalog images use floating tags, not digests - not a regression from this change).
🤖 Generated with Claude Code