feat(helm): Add execution manager service to the Helm chart. - #411
Conversation
Adds the scheduler Deployment, Service, and rendered config so `helm install` brings up storage + scheduler, with the scheduler registering and polling storage for submitted jobs. The scheduler's `runtime.host` and `storage_endpoint.host` deserialize into `IpAddr`, so a render-config initContainer resolves storage's Service DNS to its ClusterIP and injects the Pod IP (via the Downward API) before the scheduler starts.
WalkthroughThe Helm chart adds execution-manager values, renders an ChangesExecution manager deployment
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant HelmValues
participant ConfigMap
participant WorkerDeployment
participant SpiderExecutionManager
HelmValues->>ConfigMap: Render execution-manager settings
ConfigMap->>WorkerDeployment: Mount execution-manager.yaml
WorkerDeployment->>SpiderExecutionManager: Start with configuration path
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
# Conflicts: # tools/deployment/spider-helm/Chart.yaml # tools/deployment/spider-helm/templates/configmap.yaml # tools/deployment/spider-helm/values.yaml
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tools/deployment/spider-helm/values.yaml (1)
58-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate non-zero execution-manager settings at chart input.
The runtime deserializes
connection_pool_sizeand both heartbeat intervals as non-zero types. Helm currently permits0, allowing a rendered chart to install a worker that exits during startup. Addvalues.schema.jsonconstraints or Helmfailchecks for these fields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/deployment/spider-helm/values.yaml` around lines 58 - 63, Validate the chart inputs for connection_pool_size, liveness.scheduler_heartbeat_interval_sec, and liveness.storage_heartbeat_interval_sec so Helm rejects zero or negative values before deployment. Add appropriate values.schema.json minimum constraints or equivalent Helm fail checks, while preserving the existing positive defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/deployment/spider-helm/templates/worker-deployment.yaml`:
- Around line 25-29: Add a checksum annotation under the Deployment pod template
metadata for the ConfigMap containing execution-manager.yaml, using Helm’s
rendered ConfigMap content as the checksum source. Ensure the annotation changes
whenever the ConfigMap changes so Kubernetes creates a new ReplicaSet and
workers receive the updated subPath-mounted configuration.
---
Nitpick comments:
In `@tools/deployment/spider-helm/values.yaml`:
- Around line 58-63: Validate the chart inputs for connection_pool_size,
liveness.scheduler_heartbeat_interval_sec, and
liveness.storage_heartbeat_interval_sec so Helm rejects zero or negative values
before deployment. Add appropriate values.schema.json minimum constraints or
equivalent Helm fail checks, while preserving the existing positive defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b84d932b-2348-4fa0-9295-a574b640b342
📒 Files selected for processing (4)
tools/deployment/spider-helm/Chart.yamltools/deployment/spider-helm/templates/configmap.yamltools/deployment/spider-helm/templates/worker-deployment.yamltools/deployment/spider-helm/values.yaml
| volumeMounts: | ||
| - name: "config" | ||
| mountPath: "/etc/spider/execution-manager.yaml" | ||
| subPath: "execution-manager.yaml" | ||
| readOnly: true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the chart files and inspect the worker deployment plus related templates.
git ls-files tools/deployment/spider-helm/templates
printf '\n--- worker-deployment.yaml ---\n'
cat -n tools/deployment/spider-helm/templates/worker-deployment.yaml | sed -n '1,220p'
printf '\n--- configmap-related files ---\n'
fd -a -t f 'configmap.yaml|_helpers.tpl|deployment.yaml' tools/deployment/spider-helm/templates
for f in tools/deployment/spider-helm/templates/configmap.yaml tools/deployment/spider-helm/templates/_helpers.tpl tools/deployment/spider-helm/templates/deployment.yaml; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
cat -n "$f" | sed -n '1,260p'
fi
done
printf '\n--- search for checksum/config and subPath usage ---\n'
rg -n "checksum/config|subPath:|mountPath:|configmap" tools/deployment/spider-helm -SRepository: y-scope/spider
Length of output: 11654
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the worker deployment file if it is short enough and map the template structure.
wc -l tools/deployment/spider-helm/templates/worker-deployment.yaml
ast-grep outline tools/deployment/spider-helm/templates/worker-deployment.yaml --view expandedRepository: y-scope/spider
Length of output: 293
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the current Helm chart's values and any rollout annotations already present.
for f in tools/deployment/spider-helm/values.yaml tools/deployment/spider-helm/templates/*.yaml; do
[ -f "$f" ] || continue
printf '\n--- %s ---\n' "$f"
rg -n "checksum|rollout|annotations|subPath|ConfigMap|config" "$f" -n -S
doneRepository: y-scope/spider
Length of output: 729
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the proposed annotation expression references an existing template path.
python3 - <<'PY'
from pathlib import Path
p = Path('tools/deployment/spider-helm/templates/configmap.yaml')
print(p.exists(), p)
if p.exists():
print(p.read_text()[:500])
PYRepository: y-scope/spider
Length of output: 712
Trigger a worker rollout when the ConfigMap changes. This Deployment mounts execution-manager.yaml through subPath, so ConfigMap updates won’t reach running workers. Add a checksum annotation under spec.template.metadata to force a new ReplicaSet when the config changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/deployment/spider-helm/templates/worker-deployment.yaml` around lines
25 - 29, Add a checksum annotation under the Deployment pod template metadata
for the ConfigMap containing execution-manager.yaml, using Helm’s rendered
ConfigMap content as the checksum source. Ensure the annotation changes whenever
the ConfigMap changes so Kubernetes creates a new ReplicaSet and workers receive
the updated subPath-mounted configuration.
Source: MCP tools
20001020ycx
left a comment
20001020ycx
left a comment
There was a problem hiding this comment.
I appreciate the effort for the e2e testing in your branch. I think it covers what you have implemented pretty well. However, can you first summarize what you have done in the PR description briefly, I had to ask AI to explain it for me.
Also, a note to myself, we might want to add similar testing as what Sitao did, at least the testing scope, as part of the helm's testing infra. Currently, none is provided.
20001020ycx
left a comment
20001020ycx
left a comment
There was a problem hiding this comment.
LGTM, just some nits for you to address.
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
In general, the use of "worker" makes sense to me.
Shall we update the PR title as well?
Still need review from someone with more k8s background.
I second this proposal, at least worker is the terminology that we use extensively in CLP package integration as well. |
| task_executor: | ||
| bin_path: "/usr/local/bin/spider-task-executor" | ||
| inherited_env: [] | ||
| log_dir: "/tmp/spider/task-executor" |
There was a problem hiding this comment.
I guess /tmp is meant for disposable files. /var/log/spider/task-executor might be more suitable.
There was a problem hiding this comment.
The logs are written to stderr, and our goal is to route it into container's stdout. But I'll change the log_dir in case we change the behavior latter.
There was a problem hiding this comment.
The problem is /var/log is not writtable by a non-root user, such as spider-user. Should we use a user-owned directory?
There was a problem hiding this comment.
hmm right. How about ~/.cache/spider/task-executor? But I haven't seen any pod use user home folder for log.
There was a problem hiding this comment.
Or maybe we just mount an emptydir to /var/log/spider and set uid so that spider-user is writable?
There was a problem hiding this comment.
My bad. The logs are written to stderr, and currently redirected to {log_dir}/{em_id}-{executor_id}.log.
There was a problem hiding this comment.
According to an offline discussion with Zhihao, we decided to use /tmp/spider/task-executor/ for now. Next week we will fix the log routing and remove the log_dir entirely.
| containers: | ||
| - name: "execution-manager" |
There was a problem hiding this comment.
Is there any way to inject a liveness probe? I'm not sure how spider is designed: I guess the scheduler will reschedule the job if one worker doesn't respond in X minutes, but will the worker restart automatically if there is an unexpected deadlock in user.so, or the execution manager itself?
There was a problem hiding this comment.
User can set a hard timeout on a task, and task executor will be killed and retried if it exceeds the timeout.
There was a problem hiding this comment.
I guess the execution manager could still deadlock and this pod just hangs forever?
There was a problem hiding this comment.
Good question. For this PR we just assume that it will not hang. We will design and implement a proper liveness hook later.
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
the rest lgtm. deferring to @hoophalab 's review
| - name: "execution-manager" | ||
| image: {{ include "spider.imageRef" (dict "root" . "component" "worker") | quote }} | ||
| imagePullPolicy: {{ .Values.image.worker.pullPolicy | quote }} | ||
| command: ["spider-execution-manager", "--config", "/etc/spider/execution-manager.yaml"] |
There was a problem hiding this comment.
i believe pod termination skips the graceful path here. Kubernetes uses SIGTERM because the worker image has no STOPSIGNAL, while the binary only awaits ctrl_c() / SIGINT; the scheduler shutdown RPC is therefore bypassed and assignments wait for cutoff recovery.
can we handle SIGTERM in the binary?
There was a problem hiding this comment.
Yes, we're aware of this issue. We (the Spider team) should try to address it next week.
| scheduler_poll_wait_ms: 1000 | ||
| task_executor: | ||
| bin_path: "/usr/local/bin/spider-task-executor" | ||
| inherited_env: [] |
There was a problem hiding this comment.
inherited_env names variables to forward into the task executor, but the Deployment exposes no env / envFrom hook, so operators cannot inject Secret-backed values through the chart. can we expose worker environment configuration alongside this option?
There was a problem hiding this comment.
Yeah, I think this is missed.
There was a problem hiding this comment.
I think the key is to allow users to define environment variables in the execution manager container. For now, we can leave inherited_env empty: the current configuration will ensure all the forked processes to automatically inherit the execution manager's environment. We only need to make sure env variables can be passed into the execution manager. @sitaowang1998 @20001020ycx Can you check this?
There was a problem hiding this comment.
I think the key is to allow users to define environment variables in the execution manager container.
This is exactly what I am doing in #2408
There was a problem hiding this comment.
But I do wonder should we plan out adding some test .so within spider's worker container, such that the spider k8s can be tested as a standalone entity?
Of course, not in this PR, I am okay with leaving it empty in this PR. In fact, this should be left as empty even we have the test.so as the CLP_HOME shall be defined in the container image rather than here.
There was a problem hiding this comment.
I don't think defining it in image works. Some envs are only known at deployment time. My plan is to add a extra_envs map in values, default to empty, and fill it in worker deployment's env. User can define extra_envs and pass in the values they need.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tools/deployment/spider-helm/templates/worker-deployment.yaml (1)
32-40: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoll out workers when
execution-manager.yamlchanges.Because the ConfigMap key is mounted through
subPath, existing Pods will retain stale configuration after the ConfigMap changes. Add a checksum of the rendered ConfigMap underspec.template.metadata.annotationsso Helm creates a new ReplicaSet.Suggested fix
metadata: + annotations: + checksum/config: {{ include (print $.Template.BasePath "/configmap.yaml") . | sha256sum }} labels:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/deployment/spider-helm/templates/worker-deployment.yaml` around lines 32 - 40, Update the worker Pod template metadata annotations in the worker deployment to include a checksum derived from the rendered ConfigMap containing execution-manager.yaml. Place the checksum under spec.template.metadata.annotations so any ConfigMap content change produces a new ReplicaSet and rolls out workers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tools/deployment/spider-helm/templates/worker-deployment.yaml`:
- Around line 32-40: Update the worker Pod template metadata annotations in the
worker deployment to include a checksum derived from the rendered ConfigMap
containing execution-manager.yaml. Place the checksum under
spec.template.metadata.annotations so any ConfigMap content change produces a
new ReplicaSet and rolls out workers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d0d23b3-a6b7-4f94-8eb1-b34bc24c277b
📒 Files selected for processing (2)
tools/deployment/spider-helm/templates/worker-deployment.yamltools/deployment/spider-helm/values.yaml
5 participants
Description
This PR adds the execution manager service so
helm installbrings up all Spider services: database, storage, scheduler, and execution manager.The chart now:
Note
Part of the ongoing Spider Huntsman Kubernetes integration.
This PR is the final PR in this series.
Checklist
breaking change.
Validation performed
helm upgrade --install.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Chores
0.1.2to0.1.3.