fix: run the staged weka CLI in the drivers-builder container - #2790
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
How to use the Graphite Merge QueueAdd the label main-merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has required the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Claude finished @rugggger's task in 3m 9s —— View job Code review: run the staged weka CLI in the drivers-builder container
The core reasoning checks out. I confirmed the premise from the repo: Three things worth resolving before merge: 1. The 2. PATH fallback can now form an exec loop (inline) — 3. The Smaller notes, all optional:
No security concerns: both images are weka-controlled, the privileged init container already existed, and nothing new is exposed to the host. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // the wrapper at /usr/local/bin/weka execs /usr/bin/weka, so shadowing that path makes the | ||
| // container use the CLI staged from the cluster image instead of its own older one | ||
| pod.Spec.Containers[0].VolumeMounts = append(pod.Spec.Containers[0].VolumeMounts, v1.VolumeMount{ | ||
| Name: sharedVolumeName, | ||
| MountPath: "/usr/bin/weka", | ||
| SubPath: "cli/weka", | ||
| }) |
There was a problem hiding this comment.
Blast radius: this also changes the drivers-loader.
copyWekaVersionToContainer is reached from two places (resources/drivers.go:13):
- drivers-builder —
wekacontainer/funcs_pod_ensure.go:133, where the parent PR now makescliImagethe cluster image. This is the case the description is about. - drivers-loader —
operations/load_drivers.go:508, wherecliImageis the loader/builder image andtargetImageis the cluster image.
For the loader, the main container runs DriversLoaderImage (the loader image) and now gets /usr/bin/weka replaced by that same image's wekactl-* binary. load_drivers() in weka_runtime.py:1625-1638 invokes bare weka version get / weka driver download / weka driver install, so a working path is now silently running a different binary than before. Combined with the glob change in copy-cli (which also changes what the loader's copy-weka-version init container executes at line 147), that's two behavior changes to the loader flow in a PR scoped to the builder.
If the intent is builder-only, gate the /usr/bin/weka mount on f.container.IsDriversBuilder(). If it's meant to apply to both, please say so in the description — it's not obvious that wekactl is a drop-in for the release weka in the loader's driver install path.
| if [ -z "$CLI" ]; then | ||
| # older images ship only the weka binary, reachable via PATH | ||
| CLI=$(readlink -f -- "$(command -v weka)") | ||
| fi |
There was a problem hiding this comment.
The PATH fallback can now produce an exec loop that the old code was immune to.
command -v weka resolves through PATH, and /usr/local/bin/weka typically precedes /usr/bin. In the main container that path is the operator's run-weka-cli.sh wrapper (resources/pod.go:260-264), whose last line is exec-equivalent /usr/bin/weka "$@" (charts/weka-operator/resources/run-weka-cli.sh:31). The init container doesn't get that configmap mount, so this only bites if the image itself ships a /usr/local/bin/weka shell wrapper — but if it does, the fallback stages that script and the new mount below puts it at /usr/bin/weka, i.e. the wrapper now execs itself. Previously the staged copy was only read by copy-weka-version, so the loop couldn't form.
Since the mount target is /usr/bin/weka specifically, resolve the fallback against that exact path rather than PATH:
| if [ -z "$CLI" ]; then | |
| # older images ship only the weka binary, reachable via PATH | |
| CLI=$(readlink -f -- "$(command -v weka)") | |
| fi | |
| if [ -z "$CLI" ]; then | |
| # older images ship only the weka binary; resolve the path we shadow, not PATH, | |
| # so a wrapper script can never be staged onto the path it delegates to | |
| CLI=$(readlink -f -- /usr/bin/weka) | |
| fi |
| # the wekactl filename carries a per-image hash and the machine arch | ||
| ARCH=$(uname -m) | ||
| CLI=$(ls -1 /opt/weka/dist/image/wekactl-*-"$ARCH" 2>/dev/null | head -1) |
There was a problem hiding this comment.
Two assumptions here that nothing in this repo can verify — wekactl appears nowhere else in the tree, so this glob is the only encoding of the filename convention:
- The arch suffix is
uname -mform (x86_64/aarch64) rather than Go form (amd64/arm64). If it's the latter the glob never matches and every image silently takes the PATH fallback — i.e. the exact bug this PR fixes, reintroduced without a signal beyond thestaged $CLIline in the log. uname -mreflects the image arch. It reflects the host kernel, so an amd64 image on an arm64 node under emulation reportsaarch64and misses. Niche for this operator, but it's a free win to just globwekactl-*and only disambiguate by arch if more than one match exists.
Worth a comment naming a concrete example filename so the next reader can check the convention without an image handy.
679e69e to
f40c1f2
Compare
f4ae7fe to
7f22ad8
Compare
copy-cli resolved the CLI through PATH, which in the cluster image points at the release binary rather than the wekactl one, and staging it was inert anyway: the builder invokes a bare `weka`, and the operator wrapper at /usr/local/bin/weka execs /usr/bin/weka, so the staged copy was never reached. Glob the wekactl binary by arch, make it executable (the source is 0644), and shadow /usr/bin/weka with a subPath mount so the container runs it without weka_runtime.py having to name the path. Fall back to the previous PATH resolution for images with no wekactl-*, and fail loudly if neither exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7f22ad8 to
f6d58f8
Compare
f40c1f2 to
ded1b12
Compare
Graphite Automations"Add anton/matt/sergey/kristina as reviwers on operator PRs" took an action on this PR • (09/02/26)3 reviewers were added to this PR based on Anton Bykov's automation. |
Merge activity
|
2 participants
copy-cli resolved the CLI through PATH, which in the cluster image points
at the release binary rather than the wekactl one, and staging it was
inert anyway: the builder invokes a bare
weka, and the operator wrapperat /usr/local/bin/weka execs /usr/bin/weka, so the staged copy was never
reached. Glob the wekactl binary by arch, make it executable (the source
is 0644), and shadow /usr/bin/weka with a subPath mount so the container
runs it without weka_runtime.py having to name the path.
Fall back to the previous PATH resolution for images with no wekactl-*,
and fail loudly if neither exists.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com