feat(helm): Add storage and bundled-MariaDB templates.#384
Conversation
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds Spider Helm chart helpers and default values, renders storage configuration and gRPC resources, and conditionally renders a persistent bundled MariaDB with credentials, networking, and probes. ChangesSpider Helm deployment
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Values
participant Helm
participant Kubernetes
participant MariaDB
Values->>Helm: provide chart and component configuration
Helm->>Kubernetes: render storage and optional database resources
Kubernetes->>MariaDB: inject credentials and mount persistent storage
Possibly related issues
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
force-pushed
the
feat/2026-07-09-helm-storage-mariadb
branch
from
ef8a4a6 to
656b17e
Compare
…dalone. `ct lint` requires a values.yaml in every chart, so PR y-scope#383 could not pass its own `task lint:helm` without one. Add the generic scaffold keys (nameOverride/fullnameOverride); the storage/mariadb values are added in y-scope#384.
…dalone. `ct lint` requires a values.yaml in every chart, so PR y-scope#383 could not pass its own `task lint:helm` without one. Add the generic scaffold keys (nameOverride/fullnameOverride); the storage/mariadb values are added in y-scope#384.
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
the rest of the code and the title lgtm.
sorry i just caught an issue that might cause helm lint not to work (?) at https://github.com/y-scope/spider/pull/383/changes#r3567518399 . shall we fix it in the same PR / before this PR is merged?
| port: {{ include "spider.databasePort" . }} | ||
| name: {{ .Values.spiderConfig.database.name | quote }} | ||
| username: {{ .Values.spiderConfig.database.username | quote }} | ||
| password: {{ .Values.spiderConfig.database.password | quote }} |
There was a problem hiding this comment.
CLP keeps these separate: it creates a database Secret, injects its keys with secretKeyRef, and mounts the non-secret ConfigMap separately. It does not map a Secret into a ConfigMap.
Spider already creates the Secret, but MariaDB consumes it while storage reads one complete YAML file. Kubernetes cannot substitute Secret keys into ConfigMap data, so this is not only wiring. The chart-only fix is to render storage.yaml as a Secret and mount it; alternatively, add env/file credential support and use secretKeyRef like CLP.
There was a problem hiding this comment.
Good catch, I DID had some concern about the security stuff when discussing with Chaoyue.
I think now I get the problem, and I think it make sense to just use env variable for consistency with CLP. I will make the rust change now before merging this PR so we dont change back and forth.
There was a problem hiding this comment.
Discussed feature is implemented in this PR
| # task_instance_pool_config: | ||
| # execution_manager_stale_cutoff_sec: 30 | ||
| # gc_interval_sec: 5 | ||
| # message_channel_capacity: 1024 | ||
| # job_cache_gc_config: | ||
| # terminated_job_retention_sec: 300 | ||
| # gc_interval_sec: 10 |
There was a problem hiding this comment.
mind explaining what these are for? would it be better to leave them uncommented so they can serve as default (fallback) values?
There was a problem hiding this comment.
These are advanced configurations supported by Spider, following the convention in spider.yaml. I left them commented since values.yaml serves as the default template, they're here to showcase what Spider supports, which user can override for their own k8s deployment.
Note this is contrary to clp-package's per-variable assignment: we copy the entire YAML through to the component for compatibility, which avoids maintaining that many variables and I think is a better practice for the helm chart.
There was a problem hiding this comment.
These are configurable options for Spider storage service. In CLP, we might need to make these fields configurable, but I'm not sure what's the best way to do it. For example, shall we directly expose this value file in CLP, or shall we map CLP-side config values to fields defined here? May need @junhaoliao's inputs.
|
let's use scope |
20001020ycx
changed the title
20001020ycx
force-pushed
the
feat/2026-07-09-helm-storage-mariadb
branch
from
447b76b to
08a6359
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tools/deployment/spider-helm/templates/database-statefulset.yaml (1)
63-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPVC storage size is hardcoded.
The
20Gistorage request should be configurable via values so users can adjust it without editing the chart template.♻️ Suggested fix
spec: accessModes: ["ReadWriteOnce"] resources: requests: - storage: "20Gi" + storage: {{ .Values.spiderConfig.database.storage | default "20Gi" | quote }}And in
values.yaml:database: # ... existing fields ... storage: "20Gi"🤖 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/database-statefulset.yaml` around lines 63 - 70, Replace the hardcoded storage request in the database StatefulSet’s volumeClaimTemplates with the database storage value from Helm values, and add a database.storage default of "20Gi" to values.yaml so existing deployments retain their current size while users can override it.
🤖 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/database-secret.yaml`:
- Around line 11-13: Update the database credential configuration used by the
database-secret template and its corresponding values so passwords are not
supplied as predictable hardcoded defaults. Require users to provide database
and root passwords using Helm’s required-value behavior, while preserving the
existing username and secret field mappings.
In `@tools/deployment/spider-helm/templates/database-statefulset.yaml`:
- Around line 22-25: Add a configurable resources block to the MariaDB container
in the database StatefulSet, sourcing requests and limits from the chart values.
Define corresponding database.resources defaults in values.yaml for CPU and
memory requests and limits, preserving Helm rendering conventions and existing
container settings.
---
Nitpick comments:
In `@tools/deployment/spider-helm/templates/database-statefulset.yaml`:
- Around line 63-70: Replace the hardcoded storage request in the database
StatefulSet’s volumeClaimTemplates with the database storage value from Helm
values, and add a database.storage default of "20Gi" to values.yaml so existing
deployments retain their current size while users can override it.
🪄 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: 68eaf908-f67c-4ac7-aadb-6d2cf7c39041
📒 Files selected for processing (8)
tools/deployment/spider-helm/templates/_helpers.tpltools/deployment/spider-helm/templates/configmap.yamltools/deployment/spider-helm/templates/database-secret.yamltools/deployment/spider-helm/templates/database-service.yamltools/deployment/spider-helm/templates/database-statefulset.yamltools/deployment/spider-helm/templates/storage-deployment.yamltools/deployment/spider-helm/templates/storage-service.yamltools/deployment/spider-helm/values.yaml
| storage: | ||
| repository: "ghcr.io/y-scope/spider/storage" | ||
| pullPolicy: "Always" | ||
| tag: "main" |
There was a problem hiding this comment.
Does it allow us to switch to different versions using tag?
There was a problem hiding this comment.
Yes. an user can switch to a different version by overriding it at helm install time, e.g. helm install ... --set image.storage.tag=<version>, or through your own values file (helm install ... -f values-prod.yaml).
Once Spider starts publishing versioned image tags, we should update this default to point at a pinned release version instead of main which gets overwritten by every commit (non-stable).
There was a problem hiding this comment.
And just to be clear on the user's interface:
this values.yaml is just the default set for spinning up Spider with minimal setup. A user doing more advanced tuning or a Spider developer wanting to test a specific feature set on k8s would provide their own overrides, as shown above, on top of these defaults.
| # task_instance_pool_config: | ||
| # execution_manager_stale_cutoff_sec: 30 | ||
| # gc_interval_sec: 5 | ||
| # message_channel_capacity: 1024 | ||
| # job_cache_gc_config: | ||
| # terminated_job_retention_sec: 300 | ||
| # gc_interval_sec: 10 |
There was a problem hiding this comment.
These are configurable options for Spider storage service. In CLP, we might need to make these fields configurable, but I'm not sure what's the best way to do it. For example, shall we directly expose this value file in CLP, or shall we map CLP-side config values to fields defined here? May need @junhaoliao's inputs.
Adds the Spider Helm chart's templates and values: the storage gRPC service (Deployment + Service + ConfigMap) and a bundled MariaDB (StatefulSet + Service + Secret), gated on `spiderConfig.bundled`. Remove "database" from `bundled` to connect to an external MariaDB. Builds on the chart scaffolding + linting from the companion PR.
Co-authored-by: Junhao Liao <junhao@junhao.ca>
…har limit. `spider.fullname` truncates to 63, but appending `-storage`/`-config`/ `-database` afterward can exceed Kubernetes' 63-character name limit for long release names. Introduce `spider.componentFullname`, which trims the base to `63 - len(suffix)` before appending, and use it for all resource names, `serviceName`, Secret/ConfigMap references, and `databaseHost`.
…ironment variables. Stop rendering the database username/password into the storage ConfigMap. Storage now reads them from `SPIDER_STORAGE_DB_USERNAME`/`SPIDER_STORAGE_DB_PASSWORD` (spider-storage y-scope#397), injected via `secretKeyRef` from the database Secret. The Secret now renders in both bundled and external modes (`root_password` stays bundled-only) so the credentials are available regardless of deployment mode.
…se` to keep it DB-type agnostic.
Co-authored-by: Junhao Liao <junhao@junhao.ca>
20001020ycx
force-pushed
the
feat/2026-07-09-helm-storage-mariadb
branch
from
e43f5f1 to
bcd7223
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tools/deployment/spider-helm/templates/database-secret.yaml (1)
10-13: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDefault database credentials are weak and hardcoded in values.
The default values are predictable and visible in the chart source. If deployed without overriding, the bundled MariaDB will have known credentials. Consider using
requiredto force users to set passwords, or generate random defaults.🔒️ Suggested fix using `required`
stringData: - username: {{ .Values.spiderConfig.database.username | quote }} - password: {{ .Values.spiderConfig.database.password | quote }} - {{- if has "database" .Values.spiderConfig.bundled }} - root_password: {{ .Values.spiderConfig.database.root_password | quote }} - {{- end }} + username: {{ required "spiderConfig.database.username must be set" .Values.spiderConfig.database.username | quote }} + password: {{ required "spiderConfig.database.password must be set" .Values.spiderConfig.database.password | quote }} + {{- if has "database" .Values.spiderConfig.bundled }} + root_password: {{ required "spiderConfig.database.root_password must be set" .Values.spiderConfig.database.root_password | quote }} + {{- 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/database-secret.yaml` around lines 10 - 13, Update the database credential references in the database secret template to require explicitly configured passwords instead of accepting predictable chart defaults. Apply this to the bundled MariaDB root password and the regular database password while preserving username rendering and the existing bundled-database condition.
🧹 Nitpick comments (2)
tools/deployment/spider-helm/values.yaml (1)
10-12: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider using
IfNotPresentfor pinned image tags.Using
Alwaysfor a fixed version tag like10.11.16forces unnecessary network requests to check the registry. Consider changing it toIfNotPresentto optimize startup time and reduce network overhead.♻️ Proposed refactor
database: repository: "mariadb" - pullPolicy: "Always" + pullPolicy: "IfNotPresent" tag: "10.11.16"🤖 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 10 - 12, Update the pullPolicy setting alongside the pinned tag "10.11.16" from Always to IfNotPresent, while leaving the repository and tag unchanged.tools/deployment/spider-helm/templates/database-statefulset.yaml (1)
68-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the PVC storage size configurable.
Hardcoding the persistent volume claim storage size to
20Girestricts flexibility for users with different storage needs. Consider exposing this as a configuration value (e.g.,spiderConfig.database.persistence.size).♻️ Proposed refactor
resources: requests: - storage: "20Gi" + storage: {{ .Values.spiderConfig.database.persistence.size | quote }}🤖 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/database-statefulset.yaml` around lines 68 - 70, Replace the hardcoded "20Gi" value in the database StatefulSet persistence resources with the configurable spiderConfig.database.persistence.size value, preserving the existing storage request structure and allowing the chart configuration to control PVC capacity.
🤖 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/database-statefulset.yaml`:
- Around line 24-26: Add a configurable resources block to the database
container in the StatefulSet, sourcing it from the database values configuration
and preserving Helm’s standard requests and limits structure. Define
corresponding database.resources defaults in values.yaml for CPU and memory
requests and limits.
- Around line 52-59: Update the &health-check probe command to avoid passing
MYSQL_PASSWORD as a --password argument: invoke the database check through a
shell and expose the password via the MYSQL_PWD environment variable, using
shell $VAR expansion rather than kubelet $(VAR) expansion. Preserve the existing
host, port, and user settings.
---
Duplicate comments:
In `@tools/deployment/spider-helm/templates/database-secret.yaml`:
- Around line 10-13: Update the database credential references in the database
secret template to require explicitly configured passwords instead of accepting
predictable chart defaults. Apply this to the bundled MariaDB root password and
the regular database password while preserving username rendering and the
existing bundled-database condition.
---
Nitpick comments:
In `@tools/deployment/spider-helm/templates/database-statefulset.yaml`:
- Around line 68-70: Replace the hardcoded "20Gi" value in the database
StatefulSet persistence resources with the configurable
spiderConfig.database.persistence.size value, preserving the existing storage
request structure and allowing the chart configuration to control PVC capacity.
In `@tools/deployment/spider-helm/values.yaml`:
- Around line 10-12: Update the pullPolicy setting alongside the pinned tag
"10.11.16" from Always to IfNotPresent, while leaving the repository and tag
unchanged.
🪄 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: 0359adea-a601-414d-9960-79bc99ff4846
📒 Files selected for processing (8)
tools/deployment/spider-helm/templates/_helpers.tpltools/deployment/spider-helm/templates/configmap.yamltools/deployment/spider-helm/templates/database-secret.yamltools/deployment/spider-helm/templates/database-service.yamltools/deployment/spider-helm/templates/database-statefulset.yamltools/deployment/spider-helm/templates/storage-deployment.yamltools/deployment/spider-helm/templates/storage-service.yamltools/deployment/spider-helm/values.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/deployment/spider-helm/templates/_helpers.tpl
3 participants
Description
This PR adds the chart's templates and
values.yaml: the storage gRPC service (Deployment + Service + ConfigMap) and a bundled MariaDB (StatefulSet + Service + Secret). Remove"database"frombundledto connect to an external MariaDB instead.Note
Part of the ongoing Spider Huntsman Kubernetes integration. This PR adds only storage + database; the scheduler, execution manager, and task executor follow in later PRs.
Note
This PR needs review from two teams:
spiderConfigblock in values.yaml and the storage.yaml it renders in configmap.yaml. You can leave the rest of the chart to the Cloud reviewer.A few notes for the reviewers:
For Cloud reviewer:
_helpers.tplis mostly helm create boilerplate. We only added: componentFullname (63-char-safe names), imageRef, readiness/livenessProbeTimings, and databaseHost/databasePort.For Spider reviwer:
spiderConfigin values.yaml is the user-facing config in helm chart; the chart renders it into each component's config file. Naming loosely follows the sample spider.yaml. I would appreciate any suggestions to make it more aligning with the real Spider configuration workflow.Checklist
breaking change.
Validation performed
We perform the steps below to verify the storage + database Helm chart in this PR works end-to-end in a
kindcluster. The goal: submit a task through thespider-clientRust SDK and observe it persisted in the database.kindcluster:Expected:
execution_managers,jobs,resource_groups,schedulers,sessions.spider-clientSDK, the below code are phsedu-code for illustration purpose:The client submits a one-task graph —
math::add(2, 3):Expected:
jobs=1,resource_groups=1.Summary by CodeRabbit