fix: add wekactl_as_default ff and fix ensure_weka_version - #2794
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 @assafgi's task in 3m 18s —— View job Code Review
SummaryThe change is small and the intent is sound: gate the new wekactl-flavored What I verified
Findings (details inline)
No security concerns — no new external input reaches the shell; all command strings are static literals. No performance concerns beyond finding #1's extra file read per invocation, which is negligible. Only #1 is worth resolving before merge, and it may well be a non-issue if the release spec is guaranteed present in every image variant — that's the question I couldn't answer from the code alone. I made no code changes, since this was a review request. |
Graphite Automations"Add anton/matt/sergey/kristina as reviwers on operator PRs" took an action on this PR • (09/03/26)4 reviewers were added to this PR based on Anton Bykov's automation. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| async def ensure_weka_version(force_set=False): | ||
| if force_set: | ||
| ff = await get_feature_flags() |
There was a problem hiding this comment.
New hard dependency on /opt/weka/dist/release earlier in the boot flow.
get_feature_flags() → get_release_spec() (line 1517) does a bare os.listdir("/opt/weka/dist/release") plus assert len(files) == 1 — no os.path.isdir guard, unlike the sibling get_weka_version() (line 1499) which explicitly tolerates the directory being absent.
ensure_weka_version() is called at line 4517 for every mode except adhoc-op, and this is now the earliest get_feature_flags() call in the whole flow — every pre-existing caller (configure_traces 2559, ensure_weka_container 2943, assert_ssdproxy_iommu_supported 4211, write_feature_flags_json) runs strictly later. In particular envoy and telemetry modes return before any of those and therefore never called get_feature_flags() at all before this change.
Failure scenario: in a mode/image where /opt/weka/dist/release isn't populated yet at agent-start time, ensure_weka_version() now raises FileNotFoundError instead of doing its job — turning a previously working boot into a crash-loop, and with a misleading traceback (nothing to do with versions).
Worth either confirming the release dir is guaranteed present at this point for envoy/telemetry/drivers-dist/drivers-builder, or defaulting to the legacy branch when the spec can't be read.
| set_cmd = "weka version set $(weka version -J | jq -r '.[0].version')" | ||
| cmd = set_cmd if force_set else f"weka version -J | jq -e 'any(.[]; .current)' >/dev/null || {set_cmd}" |
There was a problem hiding this comment.
jq -r will happily emit the literal string null.
jq -r '.[0].version' returns null (printed as null) when the array is empty or when the object has no version key — and run_command prefixes set -e, which does not abort on a failed/empty command substitution used as an argument. The result is weka version set null, which is either a confusing error or, worse, a silent no-op.
Concrete path into it: weka version -J returns [] (no versions installed yet) → jq -e 'any(.[]; .current)' is false → falls through to set_cmd → weka version set null.
jq -er makes the substitution fail loudly on null/false so the set -e shell surfaces a real error:
| set_cmd = "weka version set $(weka version -J | jq -r '.[0].version')" | |
| cmd = set_cmd if force_set else f"weka version -J | jq -e 'any(.[]; .current)' >/dev/null || {set_cmd}" | |
| set_cmd = "weka version set $(weka version -J | jq -er '.[0].version')" | |
| cmd = set_cmd if force_set else f"weka version -J | jq -e 'any(.[]; .current)' >/dev/null || {set_cmd}" |
Separately, .[0] carries over the legacy assumption that the first array entry is the version we want. That was at least a flat string list before; with objects it's worth double-checking wekactl's ordering guarantee. Since get_weka_version() (line 1499) already derives the exact version from the release dir, passing it explicitly (weka version set {version}) would remove the positional guess entirely.
| if ff.wekactl_as_default: | ||
| # the wekactl-backed CLI renders the version table with a '✔' column instead of | ||
| # a '*' marker and emits objects (not strings) in -J, which silently breaks the | ||
| # legacy marker grep and $(weka version) substitution | ||
| set_cmd = "weka version set $(weka version -J | jq -r '.[0].version')" | ||
| cmd = set_cmd if force_set else f"weka version -J | jq -e 'any(.[]; .current)' >/dev/null || {set_cmd}" | ||
| elif force_set: | ||
| cmd = "weka version set $(weka version -J | jq -r '.[0]')" | ||
| else: | ||
| cmd = "weka version | grep '*' || weka version set $(weka version)" |
There was a problem hiding this comment.
Readability nit (non-blocking): the mixed ternary + elif chain makes it hard to see that there are really two orthogonal axes — CLI flavor and force/check. Splitting them reads better and makes the legacy/wekactl parity obvious:
| if ff.wekactl_as_default: | |
| # the wekactl-backed CLI renders the version table with a '✔' column instead of | |
| # a '*' marker and emits objects (not strings) in -J, which silently breaks the | |
| # legacy marker grep and $(weka version) substitution | |
| set_cmd = "weka version set $(weka version -J | jq -r '.[0].version')" | |
| cmd = set_cmd if force_set else f"weka version -J | jq -e 'any(.[]; .current)' >/dev/null || {set_cmd}" | |
| elif force_set: | |
| cmd = "weka version set $(weka version -J | jq -r '.[0]')" | |
| else: | |
| cmd = "weka version | grep '*' || weka version set $(weka version)" | |
| if ff.wekactl_as_default: | |
| # the wekactl-backed CLI renders the version table with a '✔' column instead of | |
| # a '*' marker and emits objects (not strings) in -J, which silently breaks the | |
| # legacy marker grep and $(weka version) substitution | |
| set_cmd = "weka version set $(weka version -J | jq -er '.[0].version')" | |
| check_cmd = "weka version -J | jq -e 'any(.[]; .current)' >/dev/null" | |
| else: | |
| set_cmd = "weka version set $(weka version -J | jq -r '.[0]')" | |
| check_cmd = "weka version | grep -q '*'" | |
| cmd = set_cmd if force_set else f"{check_cmd} || {set_cmd}" |
Note this also fixes a latent inconsistency in the legacy path that the current diff leaves in place: the legacy force_set branch uses weka version set $(weka version -J | jq -r '.[0]'), but the legacy fallback uses weka version set $(weka version) — which substitutes the entire multi-line version table as arguments, not a single version. That fallback only fires when no version is marked current, which is presumably why it's gone unnoticed. Given the PR title says "fix ensure_weka_version", it may be worth fixing both branches rather than only the new one.
Happy to leave the legacy branch untouched if you'd rather keep the blast radius to the flagged path — just flagging it.
There was a problem hiding this comment.
🟢 Approval recommended
The change is small, localized, and the updated ensure_weka_version() logic aligns with the stated wekactl JSON/marker behavior without impacting other flows.
Pull request overview
This PR updates the Helm chart runtime script to recognize a new image feature flag indicating the wekactl-backed CLI is the default, and adjusts the ensure_weka_version() logic to correctly detect/set the active Weka version when the CLI JSON output format differs.
Changes:
- Added a new
wekactl_as_defaultfeature flag bit to theFeaturesFlagsbitmap map. - Updated
ensure_weka_version()to useweka version -J+jqchecks compatible with wekactl’s JSON/object output and current-version marker semantics.
File summaries
| File | Description |
|---|---|
| charts/weka-operator/resources/weka_runtime.py | Adds the wekactl_as_default feature flag and updates ensure_weka_version() to correctly set/check the current version under wekactl-backed CLI output. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
53b507c to
783ccc6
Compare
|
Claude finished @assafgi's task in 3m 1s —— View job Code Review (re-run — commit was amended since my last pass)
What changed since my last reviewThe The new
|
| # | Severity | Issue |
|---|---|---|
| 1 | Medium-High | /opt/weka/bin becomes ephemeral — it was host-persistent, it's now the container overlay. If weka version set writes there, version selection stops being persistent state. Worse, if bin/ is a symlink in the image, the mount -o bind follows it, mkdir -p at line 3113 materializes a real directory, and line 3114 makes it a mountpoint — a later symlink replacement then fails EBUSY, surfacing as Failed to set weka version from the other hunk in this same commit. ls -ld /opt/weka/bin in the image settles it. |
| 2 | Medium | Unguarded bind under set -e — configure_persistency() runs for every mode and the only gate is [ -d /host-binds/opt-weka ] (a pod volume check, not an image-content check). Any variant shipping no bin/ goes from working to crash-loop. Flagged that the naive mkdir -p fix is worse than the crash, and suggested a directory guard on both halves. |
Also worth a thought (no inline comment): on nodes upgraded from a prior operator version, {WEKA_PERSISTENCE_DIR}/bin already holds real content on the host. It is now permanently masked — never read, never cleaned. Harmless today, but if the bind is ever removed that stale content resurfaces and shadows the image's bin/. Not a blocker; just don't let it become a surprise later.
What I verified as fine: ordering of all five mounts is correct; --make-private is applied consistently with dist/; the staging dirs are siblings of /opt/weka, not children, so they aren't clobbered by the persistence bind; {WEKA_PERSISTENCE_DIR}/dist/drivers re-bind at line 3117 is unaffected; no new external input reaches the shell (all literals); no .ainav file covers persistency mounts, so nothing goes stale.
Still open from my previous review
Unchanged code, so re-flagging briefly rather than re-commenting: get_feature_flags() is now the earliest such call in the boot flow and get_release_spec() has no isdir guard; jq -r emits literal null where jq -er would fail loudly. The first is the only one I'd want resolved before merge, and it may well be a non-issue if the release spec is guaranteed present in every image variant.
I made no code changes — this was a review.
• Branch
There was a problem hiding this comment.
🔵 Needs a closer look
It changes early-boot mount/persistence behavior and version-selection logic in a critical runtime path where subtle regressions can prevent containers from starting correctly.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
| # --- bin/ ships only in the image, so the bind below would mask it entirely. | ||
| # --- wekactl resolves the agent through /opt/weka/bin/agent, and bin/ also holds | ||
| # --- netns_vlan.sh and weka-bootstrap.sh as real files with no copy under dist/. | ||
| mkdir -p /opt/weka-bin-save | ||
| mount -o bind /opt/weka/bin /opt/weka-bin-save | ||
| mount --make-private /opt/weka-bin-save |
There was a problem hiding this comment.
/opt/weka/bin becomes ephemeral — and weka version set (changed in this same commit) is the prime suspect for writing there.
Before this change /opt/weka/bin resolved into {WEKA_PERSISTENCE_DIR} on the host (survives pod restarts). After it, it's a bind of the image's bin/, i.e. the container's ephemeral overlay. Two things follow that are worth confirming against the actual weka image layout before merge:
-
If
weka version setmutates/opt/weka/bin(repointing entries at the selected dist version — which is the usual weka layout), those mutations now land on the container overlay and vanish on every pod restart.ensure_weka_version()at line 4527 would silently redo the work each boot. Functionally survivable, but it means the version selection is no longer persistent state, which is a behavior change nobody downstream is told about. -
If
/opt/weka/binis a symlink in the image rather than a real directory, this sequence quietly converts it into a directory mountpoint:- line 3102
mount -o bind /opt/weka/bin /opt/weka-bin-savefollows the symlink and binds its target; - line 3113
mkdir -p /opt/weka/binthen materializes a real directory inside the persistence dir; - line 3114 makes that directory a mountpoint.
Any later attempt to unlink/replace the symlink — exactly what a version switch would do — then fails with
EBUSY, and the failure surfaces fromensure_weka_version()asFailed to set weka version, pointing at entirely the wrong code. - line 3102
A single ls -ld /opt/weka/bin in the weka image settles both. If it is a symlink, the save/restore needs mount -o bind on the resolved target plus recreating the symlink, not a directory bind.
| # --- wekactl resolves the agent through /opt/weka/bin/agent, and bin/ also holds | ||
| # --- netns_vlan.sh and weka-bootstrap.sh as real files with no copy under dist/. | ||
| mkdir -p /opt/weka-bin-save | ||
| mount -o bind /opt/weka/bin /opt/weka-bin-save |
There was a problem hiding this comment.
Unguarded bind under set -e → crash-loop for any image variant without /opt/weka/bin.
run_command prefixes set -e (line 2337) and configure_persistency() is called unconditionally at line 4498 for every mode — the only gate is [ -d /host-binds/opt-weka ] (line 3084), which is the pod-level volume mount from pod.go:585, not an image-content check. So a mode whose image ships no bin/ (driver-only / envoy / telemetry variants) goes from working to a hard failure: the bind fails, the whole persistency block aborts, configure_persistency() raises at line 3178, and the pod crash-loops before ever reaching write_generation().
dist/ (line 3094) carries the same exposure but has been field-proven present; bin/ is a brand-new assumption on every image variant at once.
Note the obvious fix is the wrong one: adding mkdir -p /opt/weka/bin before the save-bind would bind an empty directory over the host's bin/ and silently mask it — strictly worse than crashing. Guard both halves instead:
| mount -o bind /opt/weka/bin /opt/weka-bin-save | |
| if [ -d /opt/weka/bin ]; then | |
| mkdir -p /opt/weka-bin-save | |
| mount -o bind /opt/weka/bin /opt/weka-bin-save | |
| mount --make-private /opt/weka-bin-save | |
| fi |
…with the matching if [ -d /opt/weka-bin-save ] (or a shell flag) around lines 3112-3115.
783ccc6 to
2ba2379
Compare
|
Claude finished @assafgi's task in 3m 0s —— View job Code Review (re-run — new commit)
What changed since my last reviewOnly one thing: the This resolves my previous finding #2. Images that ship no
Findings
Still open from earlier reviews (unchanged code, not re-commented)
Also worth knowing (no inline comment)
Clean: mount ordering across all six binds; Nothing here blocks merge on its own — finding #1 is a question about the image, not a defect I can confirm from this repo. I made no code changes; this was a review. |
| BIN_EXISTED=0 | ||
| if [ -d /opt/weka/bin ]; then | ||
| BIN_EXISTED=1 | ||
| mkdir -p /opt/weka-bin-save | ||
| mount -o bind /opt/weka/bin /opt/weka-bin-save | ||
| mount --make-private /opt/weka-bin-save | ||
| fi |
There was a problem hiding this comment.
Guard looks right — this resolves the crash-loop exposure I flagged last round. Verified: run_command shells out via create_subprocess_shell (/bin/sh), and [ "$BIN_EXISTED" = "1" ] / BIN_EXISTED=0 are POSIX; $BIN_EXISTED has no braces so it survives the dedent(f"""…""") intact, same as the existing $BOOT_DIR/$ENVOY_DIR.
One thing the guard does not cover, and it's the half of my earlier comment that's still open: [ -d ] dereferences symlinks. If /opt/weka/bin is a symlink-to-directory in the image, BIN_EXISTED=1 and the sequence silently converts it into a real directory mountpoint:
- line 3106 binds the symlink's target;
- line 3111 masks the image's
/opt/weka(symlink included) with the host persistence dir; - line 3119
mkdir -p /opt/weka/binthen creates a real directory on the HostPath; - line 3120 makes it a mountpoint.
Callers that readlink it see a directory, and any later unlink/replace — which is exactly what a version switch does — fails EBUSY and surfaces as Failed to set weka version from line 3204, pointing at the wrong hunk entirely.
ls -ld /opt/weka/bin in the weka image settles it. If it's a real directory, this is a non-issue and nothing needs changing. If it's a symlink, the guard wants [ -d /opt/weka/bin ] && [ ! -L /opt/weka/bin ] plus explicit symlink recreation on the restore side rather than a directory bind.
There was a problem hiding this comment.
🔵 Needs a closer look
It modifies mount/bind behavior and version-selection shell logic in a critical runtime path, which merits careful human validation across container runtimes and image variants.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
Merge activity
|
No description provided.