build(huntsman): Add local build tasks for Spider Huntsman images.#381
Conversation
Adds `taskfiles/docker.yaml` (wired into the root `taskfile.yaml` as the `docker` namespace) with `build`, `build-storage`, `build-scheduler`, and `build-worker` tasks that delegate to `tools/docker/build.sh <service>`. For each service the script builds the matching multi-stage Dockerfile target, records the built image's ID to `build/spider-<service>-image.id` (so a local docker-compose deployment can pin the exact image), tags it `spider-<service>:dev-<user>-<short-id>`, and removes the previously built image. The script closely mirrors CLP's `tools/docker-images/clp-package/build.sh`, omitting the Ubuntu-codename detection (Spider pins `ubuntu:jammy`) and the Git OCI labels (Spider's CI applies labels via `docker/metadata-action`).
WalkthroughThis PR adds Docker build tooling: a new Taskfile with storage, scheduler, and worker build tasks, a Bash helper that builds and tags Docker images, and a top-level include wiring the Docker Taskfile into the task configuration. ChangesDocker build tooling
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Task as Task Runner
participant Script as build.sh
participant Docker
Task->>Script: run build-storage, build-scheduler, or build-worker
Script->>Script: validate service and load previous image ID
Script->>Docker: docker build --pull --target service
Docker-->>Script: new image ID
Script->>Script: persist image ID and compute short tag
Script->>Docker: tag the built image
Script->>Docker: remove the previous image on exit
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
20001020ycx
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/docker/build.sh`:
- Around line 9-19: The cleanup in remove_temp_file_and_prev_image is deleting
the prior working image even when docker build fails and new_image_id was never
set. Update the guard in the trap handler so it exits early when no new image
was built, using the existing remove_temp_file_and_prev_image function and the
new_image_id / prev_image_id variables. Keep the previous image intact unless a
new image ID was successfully populated and differs from prev_image_id, so
iid_file continues to point at a valid image for downstream consumers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3b8f5491-6740-4e23-a029-0ddd3d5fbde3
📒 Files selected for processing (3)
taskfile.yamltaskfiles/docker.yamltools/docker/build.sh
| remove_temp_file_and_prev_image() { | ||
| rm -f "$temp_iid_file" | ||
|
|
||
| [[ -z "$prev_image_id" || "$prev_image_id" == "$new_image_id" ]] && return | ||
|
|
||
| docker image inspect "$prev_image_id" >/dev/null 2>&1 || return | ||
|
|
||
| echo "Removing previous image $prev_image_id." | ||
| docker image remove "$prev_image_id" || true | ||
| } | ||
| trap remove_temp_file_and_prev_image EXIT |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Previous image is deleted on build failure.
When docker build fails, errexit exits the script before new_image_id is populated (it stays "" from line 31). The trap then evaluates line 12: -z "$prev_image_id" is false (prior build exists) and "$prev_image_id" == "$new_image_id" is false (old_id != ""), so the guard does not short-circuit. The previous working image is removed, yet iid_file still references it — breaking downstream docker compose consumers that read that file.
🔒 Proposed fix: short-circuit when no new image was built
- [[ -z "$prev_image_id" || "$prev_image_id" == "$new_image_id" ]] && return
+ [[ -z "$prev_image_id" || -z "$new_image_id" || "$prev_image_id" == "$new_image_id" ]] && return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| remove_temp_file_and_prev_image() { | |
| rm -f "$temp_iid_file" | |
| [[ -z "$prev_image_id" || "$prev_image_id" == "$new_image_id" ]] && return | |
| docker image inspect "$prev_image_id" >/dev/null 2>&1 || return | |
| echo "Removing previous image $prev_image_id." | |
| docker image remove "$prev_image_id" || true | |
| } | |
| trap remove_temp_file_and_prev_image EXIT | |
| remove_temp_file_and_prev_image() { | |
| rm -f "$temp_iid_file" | |
| [[ -z "$prev_image_id" || -z "$new_image_id" || "$prev_image_id" == "$new_image_id" ]] && return | |
| docker image inspect "$prev_image_id" >/dev/null 2>&1 || return | |
| echo "Removing previous image $prev_image_id." | |
| docker image remove "$prev_image_id" || true | |
| } | |
| trap remove_temp_file_and_prev_image EXIT |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/docker/build.sh` around lines 9 - 19, The cleanup in
remove_temp_file_and_prev_image is deleting the prior working image even when
docker build fails and new_image_id was never set. Update the guard in the trap
handler so it exits early when no new image was built, using the existing
remove_temp_file_and_prev_image function and the new_image_id / prev_image_id
variables. Keep the previous image intact unless a new image ID was successfully
populated and differs from prev_image_id, so iid_file continues to point at a
valid image for downstream consumers.
There was a problem hiding this comment.
@junhaoliao should we add -z "$prev_image_id" to the guard? Does this mean CLP also have the same bug?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Fetch CLP's build.sh to check whether it has the same guard logic
gh api repos/y-scope/clp/contents/tools/docker/build.sh --jq '.content' 2>/dev/null | base64 -d || echo "NOT_FOUND_default_path"Length of output: 410
🏁 Script executed:
#!/bin/bash
set -euo pipefail
gh search code "remove_temp_file_and_prev_image" --repo y-scope/clp
echo "---"
gh api repos/y-scope/clp/git/trees/main?recursive=1 --jq '.tree[].path' | grep -i docker | grep -i build || trueLength of output: 1027
🏁 Script executed:
#!/bin/bash
set -euo pipefail
gh api repos/y-scope/clp/contents/tools/docker-images/clp-package/build.sh --jq '.content' | base64 -dLength of output: 2172
@20001020ycx Good catch, and yes — confirmed.
I fetched CLP's tools/docker-images/clp-package/build.sh from y-scope/clp (main branch), and it has the exact same guard:
[[ -z "$prev_image_id" || "$prev_image_id" == "$new_image_id" ]] && returnBoth scripts initialize new_image_id="" before running docker build. If the build fails, errexit fires the EXIT trap before new_image_id is ever reassigned, so it stays "". At that point:
-z "$prev_image_id"→ false (a previous image exists)"$prev_image_id" == "$new_image_id"→ false (old_idvs"")
So the guard doesn't short-circuit, and the previous working image gets deleted even though the build failed — leaving iid_file pointing at a now-nonexistent image. This confirms the bug exists in CLP as well, not just here.
Regarding the fix: to be precise, it's -z "$new_image_id" (not prev_image_id, which is already checked) that needs to be added:
- [[ -z "$prev_image_id" || "$prev_image_id" == "$new_image_id" ]] && return
+ [[ -z "$prev_image_id" || -z "$new_image_id" || "$prev_image_id" == "$new_image_id" ]] && returnSince this is inherited from CLP, it'd be worth fixing here in Spider regardless, and optionally flagging it upstream to the CLP maintainers as a separate issue if you'd like — happy to help draft that if you want to go that route.
There was a problem hiding this comment.
right, i agree that:
- since this is potentially a new build, we should always clean the older image if the new build is faulty, just so we don't mislead developers running services with older images
- if the image has been cleaned, we should not leave a stale iid file
i proposed https://github.com/y-scope/spider/pull/381/changes#r3553514597 instead then
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
sitaowang1998
left a comment
sitaowang1998
left a comment
There was a problem hiding this comment.
Before going into details, I have a high level question: Why use a script to manage the download instead of using tasks?
This is how clp handles it at https://github.com/y-scope/clp/blob/main/tools/docker-images/clp-package/build.sh, we could it do it in task.yaml but it will be rewriting this build.sh in the yaml itself, which raises the concern in readability. |
|
From the perspective of a user, I only need the image name to launch the containers in Docker Compose. If I was given only the iid file, I need to parse it and regenerate the image name. The best output of image building for use in Docker Compose is probably a file containing the image names. |
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
for the title, how about simply:
build(docker): Add local build tasks for Spider images.
(personally i feel the unique tag is some implementation detail that is less important, but feel free to keep it if you think otherwise
| remove_temp_file_and_prev_image() { | ||
| rm -f "$temp_iid_file" | ||
|
|
||
| [[ -z "$prev_image_id" || "$prev_image_id" == "$new_image_id" ]] && return | ||
|
|
||
| docker image inspect "$prev_image_id" >/dev/null 2>&1 || return | ||
|
|
||
| echo "Removing previous image $prev_image_id." | ||
| docker image remove "$prev_image_id" || true | ||
| } | ||
| trap remove_temp_file_and_prev_image EXIT |
There was a problem hiding this comment.
right, i agree that:
- since this is potentially a new build, we should always clean the older image if the new build is faulty, just so we don't mislead developers running services with older images
- if the image has been cleaned, we should not leave a stale iid file
i proposed https://github.com/y-scope/spider/pull/381/changes#r3553514597 instead then
Co-authored-by: Junhao Liao <junhao@junhao.ca>
Co-authored-by: Junhao Liao <junhao@junhao.ca>
20001020ycx
changed the title
My bad in my explanation with you during offline, please look at the example in the PR description. With the iid file, no parsing is required as you can just do Therefore, you dont need to reformat it to something like |
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
note there's also a pr title proposal at #381 (review)
20001020ycx
changed the title
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
A nit comment which I'm not sure if it's already a decision from Junhao.
Otherwise lgtm.
|
|
||
| build-storage: | ||
| deps: [":init"] | ||
| cmd: "'{{.G_BUILD_SCRIPT}}' storage" | ||
|
|
||
| build-scheduler: | ||
| deps: [":init"] | ||
| cmd: "'{{.G_BUILD_SCRIPT}}' scheduler" | ||
|
|
||
| build-worker: | ||
| deps: [":init"] | ||
| cmd: "'{{.G_BUILD_SCRIPT}}' worker" |
There was a problem hiding this comment.
Shall we consider making these tasks private?
There was a problem hiding this comment.
I think public also makes sense, developer may just run task docker:build-storage for faster docker build.
There was a problem hiding this comment.
@junhaoliao what do you think? :)
I am fine with both
4 participants
Description
The Taskfile wraps the
docker buildfor each Spider service image, following CLP's approach — in particular the image-ID (iid) handling introduced in y-scope/clp#1335. For each service,tools/docker/build.sh:tools/docker/Dockerfile,build/spider-<service>-image.id,spider-<service>:dev-<user>-<short-id>.The tag naming convention exists to avoid conflicts; that discussion can be traced in y-scope/clp#1335 and related issues, so we won't expand on it here.
Below, I would like to clarify how docker compose consumes the images this taskfile produces, and how a compose author should resolve the image reference when spinning up the Spider services.
How docker-compose consumes the local build images
The docker compose file references each service through an env var; an env script can resolves it from the
.idfile for a local build. Usingstorageas an example:End to end use case:
Checklist
breaking change.
Validation performed
task docker:build-storage: the image builds, the digest inbuild/spider-storage-image.idmatches the built image's ID, the image is taggedspider-storage:dev-<user>-<short-id>, and the previously built image is pruned.docker run "$(cat build/spider-storage-image.id)" spider-storage --helpruns, as the non-rootuid=1000(spider-user).Summary by CodeRabbit