Skip to content

fix: don't assume initial agent port 14100 #2799

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: don't assume initial agent port 14100 #2799

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
5 changes: 4 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 @@ -3264,7 +3264,10 @@ async def configure_agent(agent_handle_drivers=False):
{no_reserve_space_cmd}
sed -i 's/cgroups_mode=auto/cgroups_mode=none/g' /etc/wekaio/service.conf || true
sed -i 's/override_core_pattern=true/override_core_pattern=false/g' /etc/wekaio/service.conf || true
sed -i "s/port=14100/port={AGENT_PORT}/g" /etc/wekaio/service.conf || true
sed -i "/^\\[agent\\]/,/^\\[/ s/^port=.*/port={AGENT_PORT}/" /etc/wekaio/service.conf
# sed exits 0 on no-match, so assert the rewrite landed: an agent left on
# port=0 listens on no TCP port and the operator cannot read its identity.
sed -n "/^\\[agent\\]/,/^\\[/p" /etc/wekaio/service.conf | grep -qx "port={AGENT_PORT}"
Comment on lines +3267 to +3270

@claude claude Bot Sep 7, 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.

Scoping the rewrite to the [agent] section is a real improvement over the old global s/port=14100/.../g (which could also clobber port=14100 in other sections). Two issues with the assertion though.

1. The assertion is vacuous in exactly the case the comment warns about.

The comment says an agent left on port=0 "listens on no TCP port and the operator cannot read its identity" — but if AGENT_PORT is "0" the sed writes port=0 and grep -qx "port=0" passes happily. AGENT_PORT reaches configure_agent() as "0" whenever the pod env carries GetAgentPort() == 0 (pod.go:282) and wait_for_resources() (line 3941) returns early — it only resolves the port for client mode or the ['drive','s3','compute','nfs','smbw','envoy','client','telemetry','data-services'] set, so drivers-*, adhoc-op-with-container, etc. fall through with the env value untouched. Same for AGENT_PORT == "": sed writes a bare port= and grep -qx "port=" still matches.

Asserting the value in Python before building the command would actually catch this:

if parse_port(AGENT_PORT) <= 0:
    raise Exception(f"Refusing to configure agent with invalid AGENT_PORT={AGENT_PORT!r}")

2. The removal of || true turns any format deviation into a container-start failure.

run_command prepends set -e (line 2337), so the grep -qx failure does propagate — good, the assertion is live. But s/^port=.*/ only matches a literal port= anchored at column 0. On any image where the [agent] section is missing, or writes port = 0 / port=0, the sed no-ops, the grep fails, and configure_agent() raises → the pod never starts, where previously it degraded silently. That's arguably the point, but it's a hard fail across every supported weka image version, and the next line writes the port into /etc/wekaio/service.json anyway. Worth confirming service.json doesn't already take precedence over service.conf — if it does, the strict service.conf assertion buys little and risks a lot. A tolerant ^[[:space:]]*port[[:space:]]*= pattern would cover the formatting variants cheaply.

Comment on lines +3267 to +3270

@claude claude Bot Sep 7, 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.

The section-scoped rewrite is the right fix, but the same function already solves this exact problem 30 lines up and the two blocks now disagree. no_reserve_space_cmd (3228-3236) does:

grep -q "^\[mounts\]" /etc/wekaio/service.conf || printf '\n[mounts]\n' >> /etc/wekaio/service.conf
if grep -qE "^[[:space:]]*allocate_reserved_space[[:space:]]*=" /etc/wekaio/service.conf; then
    sed -i -E "s/^[[:space:]]*allocate_reserved_space[[:space:]]*=.*/allocate_reserved_space=false/g" ...
else
    sed -i "/^\[mounts\]/a allocate_reserved_space=false" ...
fi

— create-section-if-missing, whitespace-tolerant match, insert-if-key-absent. The new [agent] block does none of those, and it's now set -e-fatal (run_command prepends set -e at 2337, so the grep -qx failure does propagate and configure_agent raises → pod never starts). That combination means any image where the [agent] section is absent, or writes port = 0 / port=0, goes from "silently degraded" to "container won't boot".

Two concrete gaps:

1. The assertion is vacuous in exactly the case the comment names. If AGENT_PORT == "0", sed writes port=0 and grep -qx "port=0" passes — the "agent left on port=0" state the comment warns about sails straight through. Same for AGENT_PORT == "": sed writes a bare port= and grep -qx "port=" matches. AGENT_PORT defaults to "" (line 63) and can stay unresolved: wait_for_resources() (3941) only fills it in for client mode or the ['drive','s3','compute','nfs','smbw','envoy','client','telemetry','data-services'] set, while configure_agent() (4524) runs for every mode except adhoc-op. adhoc-op-with-container (4549) takes that path with the raw env value from pod.go:292, which is strconv.Itoa(GetAgentPort()) — and agent-port allocation is gated on HasAgent() (funcs_allocate_resources.go:98), so 0 is reachable.

A Python-side guard catches the real failure mode, and parse_port (3926) is already there:

if parse_port(AGENT_PORT) <= 0:
    raise Exception(f"Refusing to configure agent with invalid AGENT_PORT={AGENT_PORT!r}")

2. Make the shell match as tolerant as its neighbour. Suggested replacement for these four lines:

Suggested change
sed -i "/^\\[agent\\]/,/^\\[/ s/^port=.*/port={AGENT_PORT}/" /etc/wekaio/service.conf
# sed exits 0 on no-match, so assert the rewrite landed: an agent left on
# port=0 listens on no TCP port and the operator cannot read its identity.
sed -n "/^\\[agent\\]/,/^\\[/p" /etc/wekaio/service.conf | grep -qx "port={AGENT_PORT}"
grep -q "^\\[agent\\]" /etc/wekaio/service.conf || printf '\\n[agent]\\n' >> /etc/wekaio/service.conf
if sed -n "/^\\[agent\\]/,/^\\[/p" /etc/wekaio/service.conf | grep -qE "^[[:space:]]*port[[:space:]]*="; then
sed -i -E "/^\\[agent\\]/,/^\\[/ s/^[[:space:]]*port[[:space:]]*=.*/port={AGENT_PORT}/" /etc/wekaio/service.conf
else
sed -i "/^\\[agent\\]/a port={AGENT_PORT}" /etc/wekaio/service.conf
fi
# sed exits 0 on no-match, so assert the rewrite landed
sed -n "/^\\[agent\\]/,/^\\[/p" /etc/wekaio/service.conf | grep -qx "port={AGENT_PORT}"

One thing I can't check from the repo: line 3272 writes the same port into /etc/wekaio/service.json. If service.json already takes precedence over service.conf for the agent port, the strict service.conf assertion buys little and risks a lot — worth confirming which file the agent actually reads before making this branch fatal.

# sed -i "s/serve_static=false/serve_static=true/g" /etc/wekaio/service.conf || true
echo '{{"agent": {{"port": \'{AGENT_PORT}\'}}}}' > /etc/wekaio/service.json
""")
Expand Down
Loading