Skip to content

fix: add wekactl_as_default ff and fix ensure_weka_version #2794

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
assafgi wants to merge 1 commit into from
Closed

fix: add wekactl_as_default ff and fix ensure_weka_version #2794

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion charts/weka-operator/resources/weka_runtime.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,8 @@ class FeaturesFlags:
ssd_proxy_includes_dpdk_memory: Union[bool, int] = 9
# flags 10, 11 are not used by the operator
weka_manages_non_ionode_affinity: Union[bool, int] = 12
# flag 13 is not used by the operator
wekactl_as_default: Union[bool, int] = 14

def __init__(self, b64_flags: Optional[str]) -> None:
active: Set[int] = set(parse_feature_bitmap(b64_flags or ""))
Expand Down Expand Up @@ -3093,6 +3095,17 @@ async def configure_persistency():
# --- defensive: kernel peer-group inheritance for subdirectory binds is subtle;
# --- make-private ensures /opt/weka cannot propagate here under any propagation mode.
mount --make-private /opt/weka-dist-save
# --- 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/.
# --- guarded: images without bin/ must not fail the whole block under set -e.
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
Comment on lines +3102 to +3108

@claude claude Bot Sep 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/bin then 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.

Fix this →

# --- WEKA_PERSISTENCE_DIR - is HostPath (persistent volume)
mkdir -p {WEKA_PERSISTENCE_DIR}/dist/drivers
mount -o bind {WEKA_PERSISTENCE_DIR} /opt/weka
Expand All @@ -3101,6 +3114,12 @@ async def configure_persistency():
mount -o bind /opt/weka-dist-save /opt/weka/dist
# --- /opt/weka/dist now holds its own reference; release the staging mount
umount /opt/weka-dist-save
# --- restore image bin the same way
if [ "$BIN_EXISTED" = "1" ]; then
mkdir -p /opt/weka/bin
mount -o bind /opt/weka-bin-save /opt/weka/bin
umount /opt/weka-bin-save
fi
# --- make drivers dir persistent
mount -o bind {WEKA_PERSISTENCE_DIR}/dist/drivers /opt/weka/dist/drivers
fi
Expand Down Expand Up @@ -3169,7 +3188,14 @@ async def configure_persistency():


async def ensure_weka_version(force_set=False):
if force_set:
ff = await get_feature_flags()

@claude claude Bot Sep 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix this →

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}"
Comment on lines +3196 to +3197

@claude claude Bot Sep 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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.

Fix this →

elif force_set:
cmd = "weka version set $(weka version -J | jq -r '.[0]')"
else:
cmd = "weka version | grep '*' || weka version set $(weka version)"
Comment on lines +3192 to 3201

@claude claude Bot Sep 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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.

Expand Down
Loading