feat(spider)!: Remove legacy Spider support from Docker Compose deployment. - #2452
feat(spider)!: Remove legacy Spider support from Docker Compose deployment.#2452sitaowang1998 wants to merge 14 commits into
Conversation
|
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 changes remove Spider orchestration from configuration models, database initialization, compression scheduling, controller wiring, templates, and deployment configuration. Celery remains the compression scheduler implementation. ChangesSpider orchestration removal
Estimated code review effort: 3 (Moderate) | ~25 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 |
sitaowang1998
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
tools/deployment/package-helm/templates/configmap.yaml (1)
70-95: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove the remaining Spider configuration block.
This template still reads
.Values.spider.enabledand emitscompression_coordinatorandspider. The updated Python and Rust configuration models no longer define these fields. If an existing values file keepsspider.enabled: true, Helm generates aclp-config.yamlthat the updated parsers cannot load. Delete this block and the remaining Spider value reference.🤖 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/package-helm/templates/configmap.yaml` around lines 70 - 95, Remove the Spider-specific template block from configmap.yaml by deleting the .Values.spider.enabled guard and the emitted compression_coordinator and spider sections, since the updated configuration models no longer support those fields. Update the template to stop referencing spider-enabled values entirely and keep the remaining ConfigMap generation path unchanged so Helm no longer produces clp-config.yaml entries the parsers cannot load.tools/deployment/package-helm/Chart.yaml (1)
18-22: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRemove the legacy Spider Helm integration as one change.
The chart still retains the Spider dependency and public values while
clpConfig.database.names.spiderwas removed. This leaves an enableable deployment path with an incomplete configuration. Remove the dependency and both Spider values blocks, or update all three sites to the supported Huntsman contract before release.
tools/deployment/package-helm/Chart.yaml#L18-L22: remove thespiderdependency andspider.enabledcondition.tools/deployment/package-helm/values.yaml#L192-L193: keep the database contract aligned with the remaining chart components.tools/deployment/package-helm/values.yaml#L386-L423: remove the legacy Spider values block.🤖 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/package-helm/Chart.yaml` around lines 18 - 22, Remove the legacy Spider integration consistently: delete the spider dependency and condition in tools/deployment/package-helm/Chart.yaml lines 18-22, align the database names contract in tools/deployment/package-helm/values.yaml lines 192-193 with the remaining components, and remove the Spider values block in lines 386-423. Do not replace it with an unsupported integration.
🤖 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/package-helm/Chart.yaml`:
- Around line 18-22: Remove the legacy Spider integration consistently: delete
the spider dependency and condition in tools/deployment/package-helm/Chart.yaml
lines 18-22, align the database names contract in
tools/deployment/package-helm/values.yaml lines 192-193 with the remaining
components, and remove the Spider values block in lines 386-423. Do not replace
it with an unsupported integration.
In `@tools/deployment/package-helm/templates/configmap.yaml`:
- Around line 70-95: Remove the Spider-specific template block from
configmap.yaml by deleting the .Values.spider.enabled guard and the emitted
compression_coordinator and spider sections, since the updated configuration
models no longer support those fields. Update the template to stop referencing
spider-enabled values entirely and keep the remaining ConfigMap generation path
unchanged so Helm no longer produces clp-config.yaml entries the parsers cannot
load.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f4dc6421-ad3b-47f5-a204-c00023d7d891
📒 Files selected for processing (3)
tools/deployment/package-helm/Chart.yamltools/deployment/package-helm/templates/configmap.yamltools/deployment/package-helm/values.yaml
There was a problem hiding this comment.
TODO and future work are listed as following:
- In the user doc here, we still have
(if using Celery)labels which should be removed (since Celery is the only option now). - When cross-referencing PRs for Spider Wolf integration, the coding agent found a bug that was missed from the very beginning in this line:
self.database_config.namewill be an unresolved key; it has been renamed tonames. I'm ok for either fixing it in this PR or creating a separate PR for it. - The current
clp_config.pyexplicitly rejectsdatabase.names.spider, but it will not complain ifcompression_scheduler.typeorspider_scheduleris given as they will be silently ignored. Consider we probably don't have real users for the old Spider scheduler, I'm ok with this behavior. Just need a sanity check with @junhaoliao.- The proper fix for this is to add
@model_validator(mode="before")validator to reject any removed keys. But I'm not sure if we ever need to do it.
- The proper fix for this is to add
- This PR doesn't remove Spider Wolf's build and installation flow. I've added a followup comment in #2440 (comment) to keep track of this. I think it's worth to cut Spider from our build flow for this release, while we can keep the Python side implementation untouched since we may want to remove
compression-schedulerall at once.
| SPIDER = auto() | ||
|
|
||
|
|
||
| class ClpDbNameType(KebabCaseStrEnum): |
There was a problem hiding this comment.
Rust mirrors this config in clp-rust-utils which is not updated:
There was a problem hiding this comment.
The rust side has already removed the spider option.
1 and 2 fixed. |
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 `@components/clp-py-utils/clp_py_utils/sql_adapter.py`:
- Around line 142-143: Update the logging.exception call surrounding the
database-existence error in the SQL adapter to avoid eager f-string
interpolation: use a logging format placeholder and pass
self.database_config.names[ClpDbNameType.CLP] as a separate argument, preserving
the existing message.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 601ae902-24af-40ed-ac92-a59e61e9576d
📒 Files selected for processing (2)
components/clp-py-utils/clp_py_utils/sql_adapter.pydocs/src/user-docs/guides-docker-compose-deployment.md
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 (2)
components/clp-tdl-package/src/task/compression/compress.rs (2)
406-411: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear an inherited
AWS_SESSION_TOKENwhen no token is selected.
s3_credential_envonly addsAWS_SESSION_TOKENwhen credentials contain a token, butrun_log_converterandrun_clp_sinherit the parent environment by default. This lets the child keep the parent token while using different access credentials. Clear the child environment or explicitly removeAWS_SESSION_TOKENbefore applying the resolved credentials.🤖 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/clp-tdl-package/src/task/compression/compress.rs` around lines 406 - 411, Update the environment setup in the compression credential flow around s3_credential_env so AWS_SESSION_TOKEN is explicitly cleared or removed when session_token is absent, while preserving the existing insertion when a token is present. Ensure run_log_converter and run_clp_s receive the resolved credentials without inheriting an unrelated parent session token.
384-401: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBind or refresh short-lived default credentials for
clp-s.Default
AwsAuthentication::Defaultresolves one credential set and passes onlyAWS_ACCESS_KEY_ID,AWS_SECRET_ACCESS_KEY, andAWS_SESSION_TOKENtoclp-s.clp-sauthenticates with these env vars directly, not through the AWS SDK provider chain. If the default provider returns an expiring role or session credential and the compression task outlives it, the child will keep using the stale credentials and fail. Pass an expiring credential boundary as theclp-stimeout, or refresh the child credentials during the run.🤖 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/clp-tdl-package/src/task/compression/compress.rs` around lines 384 - 401, Update the AwsAuthentication::Default credential handoff in the compression task so clp-s does not retain stale short-lived credentials: propagate the resolved credential expiration as the child’s timeout boundary, or refresh and rebind the AWS environment credentials before they expire. Preserve the existing access key, secret key, and optional session token values while ensuring long-running compression remains authenticated.
🤖 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 `@components/clp-tdl-package/src/task/compression/compress.rs`:
- Around line 406-411: Update the environment setup in the compression
credential flow around s3_credential_env so AWS_SESSION_TOKEN is explicitly
cleared or removed when session_token is absent, while preserving the existing
insertion when a token is present. Ensure run_log_converter and run_clp_s
receive the resolved credentials without inheriting an unrelated parent session
token.
- Around line 384-401: Update the AwsAuthentication::Default credential handoff
in the compression task so clp-s does not retain stale short-lived credentials:
propagate the resolved credential expiration as the child’s timeout boundary, or
refresh and rebind the AWS environment credentials before they expire. Preserve
the existing access key, secret key, and optional session token values while
ensuring long-running compression remains authenticated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c602d59a-9050-48d4-aed9-eef7714ad2b4
📒 Files selected for processing (1)
components/clp-tdl-package/src/task/compression/compress.rs
LinZhihao-723
changed the title
LinZhihao-723
changed the title
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
- Directly modified the PR title. Notice that it should be "Docker Compose" not "Docker compose".
- Make sure CI passes before you merge.
2 participants
Description
This PR removes the user-facing configuration and docker compose deployment for the legacy Spider orchestration, as tracked by #2440:
Note
Note to reviewer: Make sure the changes do not conflict with #2418, which adds helm support for Spider huntsman.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Removed Features
Configuration
Maintenance