feat(helm): Add scheduler service to the Helm chart.#403
Conversation
20001020ycx
changed the title
50e28a7 to
3f97d66
Compare
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.
3f97d66 to
db62d43
Compare
WalkthroughThe Helm chart adds scheduler image and runtime configuration, renders a scheduler ConfigMap entry, and deploys the scheduler with a gRPC Service, health probes, and ConfigMap-mounted configuration. The chart version is updated to ChangesScheduler Helm deployment
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HelmValues
participant ConfigMap
participant SchedulerDeployment
participant SchedulerService
participant Scheduler
HelmValues->>ConfigMap: Render scheduler.yaml settings
ConfigMap->>SchedulerDeployment: Supply mounted scheduler.yaml
SchedulerDeployment->>Scheduler: Start scheduler with gRPC port
SchedulerService->>Scheduler: Route gRPC traffic to named port
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 |
20001020ycx
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tools/deployment/spider-helm/templates/scheduler-deployment.yaml (2)
70-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSupport standard scheduling and resource configuration hooks.
To fully align with the opt-in pattern for configuration overrides, consider rendering standard Helm hooks for
resources,nodeSelector,tolerations, andaffinity. This enables users to customize scheduling and limits if they define them in theirvalues.yaml, while safely defaulting to nothing if omitted. As per learnings, use an opt-in/CLP-style pattern to renderresourcesand scheduling override blocks rather than relying on implicit/hardcoded constraints.⚙️ Proposed addition of configuration hooks
For example, appending these blocks to the container and pod specs enables the opt-in pattern (adjust the
.Values.*paths depending on how you plan to namespace them in your values):livenessProbe: {{- include "spider.livenessProbeTimings" . | nindent 12 }} tcpSocket: port: "grpc" + {{- with .Values.resources }} + resources: + {{- toYaml . | nindent 12 }} + {{- end }} + {{- with .Values.nodeSelector }} + nodeSelector: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.affinity }} + affinity: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.tolerations }} + tolerations: + {{- toYaml . | nindent 8 }} + {{- end }}🤖 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/scheduler-deployment.yaml` around lines 70 - 74, Update the scheduler Deployment template around the container and pod specifications to conditionally render Helm values for resources, nodeSelector, tolerations, and affinity. Use the chart’s established opt-in/CLP-style helpers or conditional blocks so each section is emitted only when configured in values.yaml, otherwise preserving the current unconstrained defaults.Source: Learnings
31-35: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a retry loop for DNS resolution to improve startup resilience.
If the storage Service and the scheduler Deployment are created simultaneously (e.g., during
helm install), the CoreDNS record may not be instantly resolvable during the pod's first second of life. Using a retry loop prevents unnecessaryCrashLoopBackOffevents and delays.🛡️ Proposed refactor for robust DNS resolution
- storage_ip="$(getent hosts "$STORAGE_HOST" | head -n1 | tr -s ' ' | cut -d' ' -f1)" - if [ -z "$storage_ip" ]; then - echo "Failed to resolve storage host: $STORAGE_HOST" >&2 - exit 1 - fi + storage_ip="" + attempts=0 + while [ -z "$storage_ip" ] && [ "$attempts" -lt 15 ]; do + storage_ip="$(getent hosts "$STORAGE_HOST" | head -n1 | tr -s ' ' | cut -d' ' -f1)" + if [ -z "$storage_ip" ]; then + echo "Waiting for storage host $STORAGE_HOST to be resolvable..." >&2 + sleep 2 + attempts=$((attempts + 1)) + fi + done + + if [ -z "$storage_ip" ]; then + echo "Failed to resolve storage host: $STORAGE_HOST" >&2 + exit 1 + fi🤖 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/scheduler-deployment.yaml` around lines 31 - 35, Update the startup DNS-resolution logic around storage_ip in the scheduler container to retry resolving STORAGE_HOST for a short bounded period before failing. Keep using the resolved address on success, and only emit the existing failure message and exit after all retries are exhausted.
🤖 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.
Nitpick comments:
In `@tools/deployment/spider-helm/templates/scheduler-deployment.yaml`:
- Around line 70-74: Update the scheduler Deployment template around the
container and pod specifications to conditionally render Helm values for
resources, nodeSelector, tolerations, and affinity. Use the chart’s established
opt-in/CLP-style helpers or conditional blocks so each section is emitted only
when configured in values.yaml, otherwise preserving the current unconstrained
defaults.
- Around line 31-35: Update the startup DNS-resolution logic around storage_ip
in the scheduler container to retry resolving STORAGE_HOST for a short bounded
period before failing. Keep using the resolved address on success, and only emit
the existing failure message and exit after all retries are exhausted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a36d880e-f306-4826-b51b-a72efb6b43fa
📒 Files selected for processing (5)
tools/deployment/spider-helm/Chart.yamltools/deployment/spider-helm/templates/configmap.yamltools/deployment/spider-helm/templates/scheduler-deployment.yamltools/deployment/spider-helm/templates/scheduler-service.yamltools/deployment/spider-helm/values.yaml
sitaowang1998
left a comment
sitaowang1998
left a comment
There was a problem hiding this comment.
The scheduler configuration conforms to the current Spider's structure. Though I strongly suggest we merge #401 first and get rid of the hostname resolution part.
|
Let's merge hostname resolution PR first. I guess the current solution might not work (need a full restart) when a pod is rescheduled to another node and got assigned another IP. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/deployment/spider-helm/templates/scheduler-deployment.yaml (1)
37-44: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd opt-in blocks for resources and scheduling overrides.
The deployment template completely omits the
resources,nodeSelector,affinity, andtolerationsblocks. Based on learnings, the chart should use an opt-in pattern that renders these blocks only when explicitly set invalues.yaml. Leaving them out entirely prevents users from configuring necessary compute limits and pod placement rules.🔧 Proposed fix (adjust `.Values` paths to match your schema)
livenessProbe: {{- include "spider.livenessProbeTimings" . | nindent 12 }} tcpSocket: port: "grpc" + {{- with .Values.scheduler.resources }} + resources: + {{- toYaml . | nindent 12 }} + {{- end }} + {{- with .Values.scheduler.nodeSelector }} + nodeSelector: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.scheduler.affinity }} + affinity: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.scheduler.tolerations }} + tolerations: + {{- toYaml . | nindent 8 }} + {{- end }} volumes: - name: "config" configMap:🤖 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/scheduler-deployment.yaml` around lines 37 - 44, Update the scheduler deployment template around the container and pod specification to conditionally render resources, nodeSelector, affinity, and tolerations using the chart’s established values paths. Ensure each block is emitted only when its corresponding value is explicitly configured, while preserving valid Helm indentation and existing behavior when unset.Source: Learnings
🤖 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.
Outside diff comments:
In `@tools/deployment/spider-helm/templates/scheduler-deployment.yaml`:
- Around line 37-44: Update the scheduler deployment template around the
container and pod specification to conditionally render resources, nodeSelector,
affinity, and tolerations using the chart’s established values paths. Ensure
each block is emitted only when its corresponding value is explicitly
configured, while preserving valid Helm indentation and existing behavior when
unset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3266147d-b421-4a48-91dc-a248a9f2980a
📒 Files selected for processing (2)
tools/deployment/spider-helm/templates/configmap.yamltools/deployment/spider-helm/templates/scheduler-deployment.yaml
hoophalab
left a comment
hoophalab
left a comment
There was a problem hiding this comment.
LGTM. Validated that the charts start correctly.
|
|
||
| scheduler: | ||
| connection_pool_size: 4 | ||
| port: 50052 |
There was a problem hiding this comment.
I have tested that setting it to 50051 works. However, I don't see a reason to make it the same with storage's port.
| host: "0.0.0.0" | ||
| port: {{ .Values.spiderConfig.scheduler.port }} | ||
| runtime: | ||
| advertised_endpoint: |
There was a problem hiding this comment.
(note to self: would public_endpoint be more idiomatic?
There was a problem hiding this comment.
though advertised_endpoint clearly reflects the purpose. i guess it's fine to keep advertised_endpoint then
| round_robin: | ||
| active_job_queue_capacity: 64 | ||
| cleanup_ready_task_capacity: 256 | ||
| commit_ready_task_capacity: 256 | ||
| dispatch_queue_capacity: 64 | ||
| finalizing_job_expiration_timeout_sec: 300 | ||
| ready_task_capacity: 65536 | ||
| storage_poll_timeout_ms: 10 | ||
| tick_interval_ms: 5 |
There was a problem hiding this comment.
current interface
runtime:
scheduler:
round_robin:
tick_interval_ms: 5
dispatch_queue_capacity: 64assuming we will support FIFO - users might want to helm install with:
--set spiderConfig.scheduler.runtime.scheduler.fifo.queue_capacity=128Helm produces:
runtime:
scheduler:
round_robin:
tick_interval_ms: 5
dispatch_queue_capacity: 64
fifo:
queue_capacity: 128now two algorithms are selected
->
can we make the scheduling algorithm an explicit value instead of encoding it as the only key in a map?
runtime:
scheduler:
algorithm: "round_robin"
config:
active_job_queue_capacity: 64
cleanup_ready_task_capacity: 256
commit_ready_task_capacity: 256
dispatch_queue_capacity: 64
finalizing_job_expiration_timeout_sec: 300
ready_task_capacity: 65536
storage_poll_timeout_ms: 10
tick_interval_ms: 5the explicit structure is easier to read and allows focused Helm overrides (in helm install):
--set spiderConfig.scheduler.runtime.scheduling.algorithm=round_robin
--set spiderConfig.scheduler.runtime.scheduling.config.tick_interval_ms=10it also gives us a clear place to validate the algorithm name and produces more useful errors for unsupported algorithms. adding another implementation no longer requires interpreting a map key as both the selector and part of the configuration structure
There was a problem hiding this comment.
or this could be even better
runtime:
scheduling:
algorithm: "round_robin"
algorithms:
round_robin:
tick_interval_ms: 5
dispatch_queue_capacity: 64
fifo:
queue_capacity: 128
this way, we keep all interfaces deterministic
There was a problem hiding this comment.
| {{- include "spider.labels" . | nindent 4 }} | ||
| app.kubernetes.io/component: "scheduler" | ||
| spec: | ||
| replicas: 1 |
There was a problem hiding this comment.
i suppose we don't plan to support rolling update with helm update yet, but when we do, we should consider adding
strategy:
type: "Recreate"because we expect exactly one instance of the scheduler at a time?
the default strategy is "RollingUpdate" which might cause multiple schedulers to exist
i don't think we need to add "Recreate" today, but can we declare that we are lacking such proper helm update support, create an issue to track, and note this singleton requirement should be one of the considerations?
| scheduler: !{{ $algorithm }} | ||
| {{- toYaml (index $scheduler $algorithm) | nindent 8 }} |
There was a problem hiding this comment.
i could be understanding the end user usages wrong but this could create a problem for end users to supply alternative algorithms when we do support such. see below values.yaml for discussion
4 participants
Description
This PR adds the scheduler service so
helm installbrings up storage + scheduler, with the scheduler registering and polling storage for submitted jobs.Note
Part of the ongoing Spider Huntsman Kubernetes integration. Up to this PR, Helm chart has storage + database and scheduler, leaving execution manager, and task executor to be integrated in the follow-up PRs.
Note
This PR needs review from two teams:
Checklist
breaking change.
Validation performed
Expected table:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Chores