-
Notifications
You must be signed in to change notification settings - Fork 12
feat(helm): Add execution manager service to the Helm chart. #411
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.
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
Changes from all commits
eaadd32
8e4456f
27b7e7a
233e43b
9d695cb
016bf89
db62d43
8660491
c8d01f9
048fad8
970ee69
67d8bbd
7835342
053b119
3860435
2c6ecdb
eda3a30
0fd1dca
431c8e9
20133e0
82fd8c6
14081b2
a952898
4352c6a
a2f38f3
87d29ea
8234c56
f3cbc9a
0b0efa1
6cd96d1
36be511
7a23b79
c788de6
397b981
274c0a1
8b08049
a02bf00
02e706f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| apiVersion: "apps/v1" | ||
| kind: "Deployment" | ||
| metadata: | ||
| name: {{ include "spider.componentFullname" (dict "root" . "component" "worker") }} | ||
| labels: | ||
| {{- include "spider.labels" . | nindent 4 }} | ||
| app.kubernetes.io/component: "worker" | ||
| spec: | ||
| replicas: {{ .Values.spiderConfig.worker.replicas }} | ||
| selector: | ||
| matchLabels: | ||
| {{- include "spider.selectorLabels" . | nindent 6 }} | ||
| app.kubernetes.io/component: "worker" | ||
| template: | ||
| metadata: | ||
| labels: | ||
| {{- include "spider.labels" . | nindent 8 }} | ||
| app.kubernetes.io/component: "worker" | ||
| spec: | ||
| containers: | ||
| - name: "execution-manager" | ||
|
Comment on lines
+20
to
+21
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. User can set a hard timeout on a task, and task executor will be killed and retried if it exceeds the timeout.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess the execution manager could still deadlock and this pod just hangs forever?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question. For this PR we just assume that it will not hang. We will design and implement a proper liveness hook later. |
||
| 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"] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i believe pod termination skips the graceful path here. Kubernetes uses can we handle
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i guess this is already tracked in #393?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we're aware of this issue. We (the Spider team) should try to address it next week. |
||
| {{- with .Values.spiderConfig.worker.extra_envs }} | ||
| env: | ||
| {{- range $name, $value := . }} | ||
| - name: {{ $name | quote }} | ||
| value: {{ $value | quote }} | ||
| {{- end }} | ||
| {{- end }} | ||
| volumeMounts: | ||
| - name: "config" | ||
| mountPath: "/etc/spider/execution-manager.yaml" | ||
| subPath: "execution-manager.yaml" | ||
| readOnly: true | ||
|
Comment on lines
+32
to
+36
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 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 🤖 Prompt for AI AgentsSource: MCP tools |
||
| volumes: | ||
| - name: "config" | ||
| configMap: | ||
| name: {{ include "spider.componentFullname" (dict "root" . "component" "config") }} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,10 @@ image: | |
| pullPolicy: "Always" | ||
| repository: "ghcr.io/y-scope/spider/storage" | ||
| tag: "main" | ||
| worker: | ||
| pullPolicy: "Always" | ||
| repository: "ghcr.io/y-scope/spider/worker" | ||
| tag: "main" | ||
|
|
||
| spiderConfig: | ||
| # List of third-party services bundled (deployed) as part of the chart. | ||
|
|
@@ -34,6 +38,18 @@ spiderConfig: | |
| root_password: "spider-root-password" | ||
| username: "spider-user" | ||
|
|
||
| execution_manager: | ||
| connection_pool_size: 4 | ||
| liveness: | ||
| scheduler_heartbeat_interval_sec: 10 | ||
| storage_heartbeat_interval_sec: 10 | ||
| scheduler_poll_wait_ms: 1000 | ||
| task_executor: | ||
| bin_path: "/usr/local/bin/spider-task-executor" | ||
| inherited_env: [] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think this is missed.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the key is to allow users to define environment variables in the execution manager container. For now, we can leave
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is exactly what I am doing in #2408
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think defining it in image works. Some envs are only known at deployment time. My plan is to add a |
||
| log_dir: "/tmp/spider/task-executor" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logs are written to stderr, and our goal is to route it into container's stdout. But I'll change the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm right. How about
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or maybe we just mount an emptydir to /var/log/spider and set uid so that spider-user is writable?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My bad. The logs are written to stderr, and currently redirected to
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to an offline discussion with Zhihao, we decided to use |
||
| package_dir: "/opt/spider/packages" | ||
|
|
||
| scheduler: | ||
| connection_pool_size: 4 | ||
| port: 50052 | ||
|
|
@@ -57,3 +73,7 @@ spiderConfig: | |
| cleanup_capacity: 256 | ||
| commit_capacity: 256 | ||
| task_capacity: 1048576 | ||
|
|
||
| worker: | ||
| extra_envs: {} | ||
| replicas: 4 | ||
Uh oh!
There was an error while loading. Please reload this page.