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
3 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