feat(helm): Surface the scheduler's em_registry and the storage's task_instance_pool and job_cache_gc config in the values file. - #434
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe Helm chart adds scheduler and storage runtime settings, renders scheduler runtime fields dynamically, and updates the chart version from ChangesHelm runtime updates
Estimated code review effort: 2 (Simple) | ~10 minutes 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
components/spider-storage/tests/runtime_recovery_test.rs (1)
202-207: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winVerify the serialised configuration contract in a test.
This helper constructs
RuntimeConfigdirectly. It does not exercise deserialisation ofdb,ready_queue,task_instance_pool, orjob_cache_gc. It cannot catch a Helm key mismatch or a legacy-key fallback.Add a fixture with non-default values and deserialise the rendered storage configuration, or add an equivalent integration test.
🤖 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 `@components/spider-storage/tests/runtime_recovery_test.rs` around lines 202 - 207, Extend the runtime configuration tests around create_runtime_config to add a non-default serialized configuration fixture and deserialize the rendered storage configuration into RuntimeConfig. Assert the db, ready_queue, task_instance_pool, and job_cache_gc values so Helm key names and legacy-key fallback behavior are exercised instead of only testing direct struct construction.tools/deployment/spider-helm/values.yaml (1)
44-45: 🚀 Performance & Scalability | 🔵 TrivialValidate the one-second heartbeat defaults under load.
Both heartbeat intervals change from 10 seconds to 1 second. This increases heartbeat traffic and liveness checks by 10x. Confirm CPU, network, and transient-latency behaviour with a representative deployment before keeping these as chart defaults.
🤖 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 44 - 45, Validate the one-second defaults for scheduler_heartbeat_interval_sec and storage_heartbeat_interval_sec under a representative deployment, measuring CPU, network traffic, and transient latency; retain the 1-second chart defaults only if the increased heartbeat load is acceptable, otherwise restore the prior 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/values.yaml`:
- Around line 62-69: Add scheduler fields em_registry and stop_timeout_sec, and
storage fields task_instance_pool and job_cache_gc, using their existing
Rust-default values. Update the scheduler and storage Helm templates to render
the complete runtime configuration, while preserving the computed
advertised_endpoint in the scheduler output.
---
Nitpick comments:
In `@components/spider-storage/tests/runtime_recovery_test.rs`:
- Around line 202-207: Extend the runtime configuration tests around
create_runtime_config to add a non-default serialized configuration fixture and
deserialize the rendered storage configuration into RuntimeConfig. Assert the
db, ready_queue, task_instance_pool, and job_cache_gc values so Helm key names
and legacy-key fallback behavior are exercised instead of only testing direct
struct construction.
In `@tools/deployment/spider-helm/values.yaml`:
- Around line 44-45: Validate the one-second defaults for
scheduler_heartbeat_interval_sec and storage_heartbeat_interval_sec under a
representative deployment, measuring CPU, network traffic, and transient
latency; retain the 1-second chart defaults only if the increased heartbeat load
is acceptable, otherwise restore the prior defaults.
🪄 Autofix
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 Plus
Run ID: 9e6ef66c-63ba-427c-aeb8-e5b74724d902
📒 Files selected for processing (5)
components/spider-storage/src/state/runtime.rscomponents/spider-storage/tests/runtime_recovery_test.rstools/deployment/spider-helm/Chart.yamltools/deployment/spider-helm/templates/configmap.yamltools/deployment/spider-helm/values.yaml
20001020ycx
left a comment
There was a problem hiding this comment.
Verified end-to-end: rendered the chart with the new runtime fields for each component and confirmed the generated scheduler.yaml/storage.yaml deserialize correctly.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/configmap.yaml`:
- Around line 29-31: Filter the reserved advertised_endpoint key from
.Values.spiderConfig.scheduler.runtime before rendering it with toYaml in the
configmap template. Preserve all other runtime values and keep the existing
nindent output behavior, ensuring advertised_endpoint is emitted only by its
dedicated configuration path.
In `@tools/deployment/spider-helm/values.yaml`:
- Around line 59-61: Add the missing stop_timeout_sec entry under the
spiderConfig.scheduler.runtime configuration in values.yaml, placing it at the
level specified by the Rust schema and assigning the schema’s default value.
Preserve the existing em_registry settings.
🪄 Autofix
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 Plus
Run ID: 355b02be-4d86-4e43-b70c-5946892d03bf
📒 Files selected for processing (3)
tools/deployment/spider-helm/Chart.yamltools/deployment/spider-helm/templates/configmap.yamltools/deployment/spider-helm/values.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/deployment/spider-helm/Chart.yaml
Co-authored-by: ChenXing Yang <60459812+20001020ycx@users.noreply.github.com>
…ntime passthrough.
20001020ycx
left a comment
There was a problem hiding this comment.
Pushed with a fix in incorporating key omission with toYaml copy
Description
This PR depends on #433.
Several runtime config sections are absent from
values.yaml, so a Helm user has no way to see or override them: they silently fall back to the RustDefaultimpls. This PR surfaces the remaining ones, with values that mirror those defaults exactly, so deployments behave identically to before:runtime.em_registrydead_em_cutoff_secliveness_tracking_interval_msruntime.task_instance_poolexecution_manager_stale_cutoff_secgc_interval_secmessage_channel_capacityruntime.job_cache_gcterminated_job_retention_secgc_interval_secThe storage entries need no template change:
configmap.yamlalready rendersstorage.yaml'sruntimeby emitting the computeddbblock and passing the rest ofspiderConfig.storage.runtimethrough withtoYaml, so new keys flow through automatically.The scheduler's
runtimeblock instead enumerated each field explicitly, which is whyem_registrywas missing in the first place. This PR switches it to the same passthrough shape used by storage, sinceadvertised_endpointis the only field the template needs to compute:Besides removing the per-field plumbing, this makes every current and future
RuntimeConfigfield reachable fromvalues.yamlwithout a template edit.stop_timeout_secis one that was already unreachable and now works.Two consequences of the passthrough worth noting for reviewers:
toYamlsorts keys, so the rendered scheduler policy block now emitsconfigbeforepolicy, andpolicy's value is no longer explicitly quoted.SchedulerConfigis an adjacently tagged enum, and Serde accepts the content field ahead of the tag, so both forms deserialize identically. This was verified by deserializing the rendered config throughServerConfigrather than by inspection alone (see below).Defaultimpl will not reach Helm deployments unlessvalues.yamlis updated in step.The chart version is bumped to satisfy the chart linting workflow.
Checklist
Validation performed
Summary by CodeRabbit
New Features
Chores