feat(clp-package): Parameterize clp-package Docker image by Ubuntu codename; auto-detect host version. - #2273
Conversation
…me; auto-detect host version.
WalkthroughDocker build ARG UBUNTU_VERSION_CODENAME added and used as the base image tag; Dockerfile now removes any existing UID owner before creating the runtime user. build.sh detects host Ubuntu codename and passes it as a build-arg, and the GitHub Action/workflow expose and forward the same codename. ChangesDocker Build Parameterization
Sequence Diagram(s)sequenceDiagram
participant Workflow as clp-artifact-build workflow
participant Action as clp-build-runtime-image action
participant DockerCLI as docker build
participant Dockerfile as Dockerfile (build-time)
Workflow->>Action: invoke with platform_version_codename="jammy"
Action->>DockerCLI: docker build --build-arg UBUNTU_VERSION_CODENAME=platform_version_codename
DockerCLI->>Dockerfile: provide build-arg UBUNTU_VERSION_CODENAME
Dockerfile->>Dockerfile: ARG UBUNTU_VERSION_CODENAME used in FROM and runtime setup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
junhaoliao
changed the title
| USER="clp-user" | ||
|
|
||
| RUN useradd --uid ${UID} --shell /bin/bash --home-dir ${CLP_HOME} ${USER} | ||
| RUN userdel -r ubuntu 2>/dev/null || true \ |
There was a problem hiding this comment.
Instead of hardcoding ubuntu, we should resolve the username from the UID, using something like getent passwd "${UID}" | cut -d: -f1.
Also, is it possible to raise an error when encountering a UID conflict?
There was a problem hiding this comment.
sg. i tried to address it in 5af1885 . let me know if i understand correctly
|
|
||
| ubuntu_version_codename="jammy" | ||
| if [[ -f /etc/os-release ]]; then | ||
| host_codename="$(. /etc/os-release && echo "$VERSION_CODENAME")" |
There was a problem hiding this comment.
Right now, task package builds a clp-package image using the same Ubuntu release as the host invoking the task command. However, I believe a 22.04 host should still be able to build a 24.04 clp-package image.
Should we expose the Ubuntu version codename as a Task argument or environment variable so the package target is not tied to the host release?
There was a problem hiding this comment.
dynamic ubuntu version detection on host should make more sense. the main reason why we want to avoid hardcoding a version of the ubuntu image is because whatever (Python wheels, glibc dependent binaries, etc.) a non-Jammy host builds do not run inside the Jammy-based image container.
the long-term plan is to migrate the base image from Ubuntu to Muslinux / Manylinux as @jackluo923 proposed. we (or whoever has bandwidth) can work on it in the next month. also, i think we shall explore setting up https://containers.dev/ too.
for now, this PR can unblock our developers to develop on different versions of Ubuntu hosts, without use of Ubuntu Jammy build containers
let me know what you think
There was a problem hiding this comment.
just realized that tools/docker-images/clp-package/Dockerfile copies built artifacts instead of building the package itself. Yes I agree we should keep host and host-built container image consistent.
Bill-hbrhbr
left a comment
Bill-hbrhbr
left a comment
There was a problem hiding this comment.
Need to update docs:
| default: "amd64" | ||
| required: false |
There was a problem hiding this comment.
| default: "amd64" | |
| required: false | |
| required: true |
Maybe open an issue to make this required?
There was a problem hiding this comment.
@coderabbitai open an issue to track making the arch input required for .github/actions/clp-build-runtime-image. The issue should cover updating all current and future callers to pass arch explicitly, removing the default value, and verifying behavior for both amd64 and arm64. This is a follow-up cleanup and is out of scope for this PR.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo "y-scope/clp" \
--title "refactor(clp-build-runtime-image): make \`arch\` input required" \
--assignee "junhaoliao" \
--body "## Summary
The \`arch\` input for the \`.github/actions/clp-build-runtime-image\` composite action currently has a default value of \`\"amd64\"\`. This default masks potential misconfiguration and makes call sites less explicit about their intent.
## Tasks
- [ ] Remove the \`default: \"amd64\"\` from the \`arch\` input and set \`required: true\`.
- [ ] Audit all current callers of \`.github/actions/clp-build-runtime-image\` and update them to pass \`arch\` explicitly.
- [ ] Ensure any future callers are documented/required to pass \`arch\` explicitly.
- [ ] Verify the action behaves correctly for both \`amd64\` and \`arm64\`.
## Context
Raised during review of PR `#2273` (comment: https://github.com/y-scope/clp/pull/2273#discussion_r3268557767). This is a follow-up cleanup and was considered out of scope for that PR."Length of output: 1091
@junhaoliao, I've opened #2301 to track this. It covers:
- Removing the
default: "amd64"and settingrequired: trueon thearchinput. - Auditing and updating all current callers to pass
archexplicitly. - Documenting the requirement for future callers.
- Verifying behaviour for both
amd64andarm64.
|
|
||
| ubuntu_version_codename="jammy" | ||
| if [[ -f /etc/os-release ]]; then | ||
| host_codename="$(. /etc/os-release && echo "$VERSION_CODENAME")" |
There was a problem hiding this comment.
just realized that tools/docker-images/clp-package/Dockerfile copies built artifacts instead of building the package itself. Yes I agree we should keep host and host-built container image consistent.
| RUN existing_user="$(getent passwd "${UID}" | cut --delimiter=: --fields=1)" || true; \ | ||
| [ -z "${existing_user}" ] || userdel --remove "${existing_user}" \ | ||
| || { echo "ERROR: Cannot delete user '${existing_user}' that occupies UID ${UID}"; exit 1; } | ||
| RUN useradd --uid ${UID} --shell /bin/bash --home-dir ${CLP_HOME} ${USER} |
There was a problem hiding this comment.
To make the user handling more coherent, we can maybe merge these two inside a single RUN. Your choice.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/clp-artifact-build.yaml (1)
680-742: 🧹 Nitpick | 🔵 TrivialConsider an explicit least-privilege
permissionsblock (pre-existing).
zizmorflags this job as using default token permissions. Since the composite action pushes images to GHCR, an explicit job-levelpermissions:(e.g.contents: read,packages: write) would tighten the security posture. This is a pre-existing, repo-wide pattern and not introduced by this PR, so it's optional here.🤖 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 @.github/workflows/clp-artifact-build.yaml around lines 680 - 742, The package-image job uses default token permissions; add an explicit least-privilege permissions block to the job to limit token scope (e.g., contents: read and packages: write). Edit the job named package-image and insert a permissions: section under that job (near the strategy/matrix or runs-on) specifying at minimum contents: read and packages: write so the composite action that pushes to GHCR (./.github/actions/clp-build-runtime-image) has only the needed rights; adjust other scopes if additional actions require them.
🤖 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.
Outside diff comments:
In @.github/workflows/clp-artifact-build.yaml:
- Around line 680-742: The package-image job uses default token permissions; add
an explicit least-privilege permissions block to the job to limit token scope
(e.g., contents: read and packages: write). Edit the job named package-image and
insert a permissions: section under that job (near the strategy/matrix or
runs-on) specifying at minimum contents: read and packages: write so the
composite action that pushes to GHCR (./.github/actions/clp-build-runtime-image)
has only the needed rights; adjust other scopes if additional actions require
them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5dde365-bb61-4043-95db-d893c123a368
📒 Files selected for processing (4)
.github/actions/clp-build-runtime-image/action.yaml.github/workflows/clp-artifact-build.yamldocs/src/dev-docs/building-package.mdtools/docker-images/clp-package/Dockerfile
2 participants
Description
Previously, the
clp-packageDocker image was hardcoded toubuntu:jammy. Binaries built on an Ubuntu 24.04 (Noble) host (glibc 2.39) cannot run inside a Jammy container (glibc 2.35), causingtaskto fail on Noble hosts at the package-image stage. This affects both the C++ binaries (which require GLIBC_2.38) and Python packages with native extensions (e.g.,_mysql_connector.so,psutil,greenlet) that are compiled on the host and linked against its glibc.This PR makes two changes to
tools/docker-images/clp-package/:Dockerfile — The base image is now parameterized via
ARG UBUNTU_VERSION_CODENAME=jammysoFROM ubuntu:${UBUNTU_VERSION_CODENAME}resolves to the host-matching Ubuntu version. Additionally,userdel -r ubuntuis called beforeuseradd clp-userto handle the defaultubuntuuser present in Noble cloud images (UID 1000), which would otherwise conflict with theclp-userUID.build.sh — Auto-detects the host's Ubuntu codename from
/etc/os-release(VERSION_CODENAME) and passes it as--build-arg UBUNTU_VERSION_CODENAME=<codename>. Defaults tojammywhen detection fails, preserving backward compatibility.Checklist
breaking change.
Validation performed
Scenario 1: Full build on Noble host
Task
Run
taskon an Ubuntu 24.04 (Noble) host to verify the build succeeds end-to-end.Command
Output
Explanation
The Docker image build resolved the base image to
ubuntu:noble, and the fulltaskpipeline completed successfully on a Noble host.Scenario 2: build.sh auto-detects the host Ubuntu codename
Task
Verify that
build.shreads the host's codename from/etc/os-releaseand passes it as a build-arg.Command
Output
Explanation
The script defaults to
jammybut correctly overrides with the host'sVERSION_CODENAME=noble, ensuring the runtime image's glibc matches the host.Scenario 3: Package Dockerfile builds with UBUNTU_VERSION_CODENAME=noble
Task
Verify the Dockerfile accepts
--build-arg UBUNTU_VERSION_CODENAME=nobleand pullsubuntu:nobleas the base image.Command
Output
Explanation
Docker resolved the base image to
docker.io/library/ubuntu:noble, confirming the build-arg is correctly substituted into theFROMdirective.Scenario 4: Package Dockerfile syntax check with default (jammy)
Task
Verify backward compatibility — the Dockerfile still works with the default
jammycodename.Command
Output
Explanation
The Dockerfile passes syntax validation with the default
jammyvalue, confirming no breaking change for existing builds.Scenario 5: Container runs Noble and has correct user setup
Task
Verify the built container is based on Ubuntu Noble and the
ubuntuuser is removed whileclp-userexists with UID 1000.Command
Output
Explanation
The container runs Ubuntu Noble; the default
ubuntuuser (present in Noble cloud images, UID 1000) was removed, andclp-userwas created with UID 1000, avoiding the UID conflict that would otherwise occur.Scenario 6: Start CLP package on Noble host
Task
Verify
start-clp.shstarts all CLP services successfully.Command
Output
Explanation
All services (database, queue, redis, results-cache, compression worker/scheduler, query worker/scheduler, api-server, webui, reducer, garbage-collector) started and became healthy.
Scenario 7: Compress logs
Task
Verify log compression works end-to-end in the Noble-based package.
Command
Output
Explanation
Compression completed successfully with a 38.70x compression ratio, confirming that the Noble-based runtime image correctly executes the CLP binaries built on the Noble host.
Scenario 8: Search logs
Task
Verify log search works end-to-end in the Noble-based package.
Command
Output
Explanation
Search completed successfully, confirming the query pipeline works correctly with the Noble-based package.
Summary by CodeRabbit
Chores
Documentation