feat(helm)!: Add the Spider subchart and compression-coordinator wiring for Spider-orchestrated compression. - #2418
Conversation
…ordinator wiring for Spider-orchestrated compression.
… restore the orchestration helper.
… log-ingestor template.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe Helm chart adds a Spider dependency, resolves it during packaging and test setup, introduces Spider-backed coordinator configuration, and conditionally replaces compression scheduler and worker deployments with a compression coordinator deployment when Spider mode is enabled. ChangesSpider Helm integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Helm
participant Kubernetes
participant DbTableCreator
participant StorageService
Helm->>Kubernetes: Render compression-coordinator Deployment
Kubernetes->>DbTableCreator: Wait for db-table-creator job
Kubernetes->>StorageService: Wait for storage service
Kubernetes->>Kubernetes: Start compression-coordinator
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 |
…tor's new schema.
…mit the lockfile.
… `.Values.spider.enabled`.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/package-helm/templates/compression-coordinator-deployment.yaml`:
- Around line 27-37: Update the service wait target in the compression
coordinator’s initContainers to use the Spider storage service name, resolving
to “spider-storage” via the existing fullname override, instead of the component
name “storage”; leave the db-table-creator job wait 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3ee44ae5-21c4-4843-808b-d682a7ff3558
⛔ Files ignored due to path filters (1)
tools/deployment/package-helm/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
taskfiles/helm.yamltaskfiles/lint.yamltools/deployment/package-helm/.gitignoretools/deployment/package-helm/Chart.yamltools/deployment/package-helm/set-up-multi-dedicated-test.shtools/deployment/package-helm/set-up-multi-shared-test.shtools/deployment/package-helm/set-up-test.shtools/deployment/package-helm/templates/_helpers.tpltools/deployment/package-helm/templates/compression-coordinator-deployment.yamltools/deployment/package-helm/templates/compression-scheduler-deployment.yamltools/deployment/package-helm/templates/compression-worker-deployment.yamltools/deployment/package-helm/templates/configmap.yamltools/deployment/package-helm/values.yaml
…r-storage-subchart # Conflicts: # tools/deployment/package-helm/Chart.yaml
…chines can resolve the Spider repo.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/package-helm/set-up-multi-dedicated-test.sh`:
- Line 141: Replace unconditional helm dependency update in
tools/deployment/package-helm/set-up-multi-dedicated-test.sh:141,
tools/deployment/package-helm/set-up-multi-shared-test.sh:47, and
tools/deployment/package-helm/set-up-test.sh:31 with dependency handling that
does not require the Spider repository when spider.enabled=false; either vendor
the Spider chart alongside each chart or make Spider acquisition explicit and
conditional on its availability or enabled state.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2c871999-cb32-4c01-97cf-08a373853f6d
📒 Files selected for processing (3)
tools/deployment/package-helm/set-up-multi-dedicated-test.shtools/deployment/package-helm/set-up-multi-shared-test.shtools/deployment/package-helm/set-up-test.sh
…blish job's fresh runner can resolve the Spider repo.
… stream to the worker's stdout).
…subchart resolves CLP resource names consistently.
…pider storage host via `.Subcharts`.
…r-storage-subchart
…'s ServiceAccount (live once the Spider pin reaches 0.1.8).
…Name` support and `log_level` values).
…less of `spider.enabled` (mixed orchestration).
|
We have deployed the current state on the helm chart on the staging namespace of YScope Cloud; all pods are functioning, compression and search jobs complete successfully, provided that the credential-fix PR lands (#2438). Greatly appreciate for the help from @hoophalab for the Cloud deployment testing. |
junhaoliao
left a comment
There was a problem hiding this comment.
the rest lgtm. everything looks quite clean and the changes are idiomatic
| fullnameOverride: "" | ||
| global: | ||
| fullnameOverride: "" | ||
| nameOverride: "" |
There was a problem hiding this comment.
mind quickly explaining why we have to move those two keys to global? do we need this to also override spider's fullnameOverride and nameOverride (but spider's Helm chart would need modifications in the first place?)
There was a problem hiding this comment.
This is a great question which had haunted me for a while, and I am happy to explain as this is the problem with subchart.
Consider the below config we must set at Spider side such that worker can talk to CLP database (for commit task).
- name: "CLP_DB_USER"
valueFrom:
secretKeyRef:
name: "{{ include \"clp.fullname\" . }}-database"
However, clp.fullname is only recognized by CLP's helm chart. Without declaring it as global, Spider subchart resolves it to the literal string {{ include \"clp.fullname\" . }} rather the actual full name of the clp subchart.
There was a problem hiding this comment.
I see
shall we mark this as a breaking change? as in, those who need to override those values should be aware they need to change different keys now
Co-authored-by: Junhao Liao <junhao@junhao.ca>
|
I also performed regression test to ensure the old compression path using Celery does not fail. Below steps are performed:
|
Co-authored-by: Junhao Liao <junhao@junhao.ca>
junhaoliao
left a comment
There was a problem hiding this comment.
lgtm. the title lgtm too but let's consider marking this as a breaking change before merging
Description
See PR title. At a high level, this PR integrates Spider into the CLP Helm chart as an optional subchart that, when enabled, orchestrates compression jobs in place of the Celery-based scheduler and workers. Note, compression coordinator, compression scheduler, and compression worker are on regardless of Spider's toggle; their internal implementation handles the routing of the jobs with respect to the types.
Note: the scope of the integration is only limited to compression jobs, search jobs routing shall fall back to the Celery's query scheduler + worker.
Important
Blocked on y-scope/spider#413,y-scope/spider#415, #2408, complete compression coordinator implementation (#2417, #2420, and #2421 ), and complete TDL package implementation (#2406, #2411, #2415).
Note
This PR is breaking because we set
fullnameOverrideto global. A user now needs to runhelm install --set global.fullnameOverriderather thanhelm install --set fullnameOverride.Validation performed
Note
The validation below uses only official artifacts:
Full-Spider compression E2E: the job executes in Spider and the result is searchable
Environment: a single-node kind cluster; an S3 bucket
clp-logswith 4 JSONL log objects underdemo-logs/, e.g.{"ts":1700000001,"level":"INFO","msg":"demo log line 1"}; an empty S3 bucketclp-archivesfor archive output.Overlay runtime values for
values.yamlduring deployment, you may review the completevalues.yamlhere.Expected (no
compression-scheduler/compression-workerpods exist):Expected:
{"id":1}. ~30 s later, in the CLP DB:Expected:
and in the CLP DB (
status=2isSUCCEEDED):Expected:
jobsresource_groupsexecution_managersThe commit task registered the same archive in CLP's metadata table:
The compress task uploaded the same archive to the output bucket:
msg: "demo log line*":Expected:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores