fix: don't assume initial agent port 14100 - #2799
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
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 4m 51s —— View job Code Review
The core idea is right: rewriting Three things I'd want resolved before merge (details in inline comments): 1. Hardcoded socket path is likely wrong —
|
Graphite Automations"Add anton/matt/sergey/kristina as reviwers on operator PRs" took an action on this PR • (09/07/26)2 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}
| const agentSocket = "/opt/weka/data/agent/agent.sock" | ||
| identityArgs := fmt.Sprintf("-W container-get-identity --container-name %s --json", containerName) | ||
| cmd := fmt.Sprintf( | ||
| "if [ -S %[1]s ]; then weka local run wapi -U %[1]s:/jrpc %[2]s; else weka local run wapi -H localhost:$(cat /opt/weka/k8s-runtime/vars/agent_port)/jrpc %[2]s; fi", | ||
| agentSocket, identityArgs, | ||
| ) |
There was a problem hiding this comment.
The hardcoded socket path looks wrong for containers this operator starts.
weka_runtime.py:3427 launches the agent as:
return f"exec /usr/bin/weka --agent --socket-name weka_agent_ud_socket_{AGENT_PORT}"i.e. the operator deliberately gives every agent a port-suffixed socket name (weka_agent_ud_socket_<AGENT_PORT>), precisely so that multiple agents can coexist on a node. A fixed /opt/weka/data/agent/agent.sock is unlikely to be what that produces.
If the path never exists, [ -S ... ] is always false, the socket branch is dead code, and this reconciler silently keeps using the TCP fallback — so the PR would appear to work only because of the service.conf fix in weka_runtime.py, and would regress the moment an image really does ship port=0.
Suggest deriving the socket from the same source of truth rather than hardcoding, e.g.:
const agentSocketDir = "/opt/weka/data/agent"
// socket name must match get_agent_cmd() in weka_runtime.py
agentSocket := fmt.Sprintf("%s/weka_agent_ud_socket_%d", agentSocketDir, container.GetAgentPort())Could you confirm the actual on-disk path (ls -l /opt/weka/data/agent/ in a running container) before merging? Also worth double-checking that weka local run wapi -U <sock>:/jrpc is the accepted flag/format for the weka versions this operator supports — there's no other wapi -U usage in the repo to compare against.
| const agentSocket = "/opt/weka/data/agent/agent.sock" | ||
| identityArgs := fmt.Sprintf("-W container-get-identity --container-name %s --json", containerName) | ||
| cmd := fmt.Sprintf( | ||
| "if [ -S %[1]s ]; then weka local run wapi -U %[1]s:/jrpc %[2]s; else weka local run wapi -H localhost:$(cat /opt/weka/k8s-runtime/vars/agent_port)/jrpc %[2]s; fi", |
There was a problem hiding this comment.
Unrelated behavioral change: the wekaauthcli branch was dropped.
Before this PR, containers joining an existing cluster (container.Spec.JoinIps != nil) ran the identity call through wekaauthcli, not weka:
if container.Spec.JoinIps != nil {
cmd = fmt.Sprintf("wekaauthcli local run wapi -H 127.0.0.1:$(%s)/jrpc ...", showAgentPortCmd)
}wekaauthcli is used everywhere else in the codebase (internal/services/weka.go, ~60 call sites) for commands that need cluster credentials. Both branches now collapse to plain weka, which is not what "don't assume initial agent port 14100" implies, and isn't mentioned in the commit message.
If container-get-identity genuinely never needs auth (it's an agent-local JRPC call), that's fine — but please say so in the commit message, since it silently changes behavior for every auth-enabled cluster join. Otherwise the wekaauthcli selection should be preserved on top of the socket/TCP selection.
| 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}" |
There was a problem hiding this comment.
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.
cf2e107 to
a1b3530
Compare
4dfb6c3 to
42ef523
Compare
|
Claude finished @assafgi's task in 1m 3s —— View job Code Review
No changes since my last review. HEAD is still Restating the standing review below, since none of the three points have been addressed yet and the inline threads are still open. The core idea is right: rewriting 1. Hardcoded socket path is likely wrong —
|
42ef523 to
0ae8eee
Compare
|
Claude finished @assafgi's task in 2m 36s —— View job Code Review
The diff changed materially since my last review. The That resolves two of my three earlier points by removal:
The earlier inline threads on that file are stale; feel free to close them. What remainsScoping the rewrite to the The assertion doesn't assert the thing its comment describes. The comment warns about an agent left on Dropping Not verifiable from the repo: line 3272 writes the same port into No security concerns. |
| 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}" |
There was a problem hiding this comment.
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:
| 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.
0ae8eee to
973167b
Compare
a1b3530 to
3b65d50
Compare
|
Claude finished @assafgi's task in 22s —— View job Code Review
No changes since my last review. The PR diff vs Exiting early rather than repeating the full review. The two open points from the inline comment still stand, in one line each:
Still not verifiable from the repo: whether |
3b65d50 to
ce9a482
Compare
973167b to
e711be6
Compare
|
Claude finished @assafgi's task in 17s —— View job Code Review
No changes since my last review. The diff vs Exiting early rather than repeating the full review. The two open points from the inline comment still stand:
Still not verifiable from the repo: whether --- · branch |
Merge activity
|
2 participants
No description provided.