Wire spec-driven replicas and session storage validation for MCPServer#4364
Wire spec-driven replicas and session storage validation for MCPServer#4364
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b3a4aa972
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4364 +/- ##
==========================================
- Coverage 69.50% 69.48% -0.02%
==========================================
Files 482 485 +3
Lines 49223 49493 +270
==========================================
+ Hits 34213 34391 +178
- Misses 12365 12438 +73
- Partials 2645 2664 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR wires MCPServer replica handling to be spec-driven while preserving external autoscaling compatibility (nil-passthrough), adds a default pod termination grace period, and introduces status-condition validations for unsupported/unsafe scaling configurations.
Changes:
- Set
Deployment.Spec.Replicasfromspec.replicaswith nil-passthrough; enforce stdio replica cap (defense-in-depth) viaresolveDeploymentReplicas. - Add
TerminationGracePeriodSeconds(default 30s) to the proxy runner pod spec. - Add reconciler validations that emit status conditions for (a) stdio + replicas > 1 and (b) replicas > 1 without Redis session storage; expand unit tests accordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/thv-operator/controllers/mcpserver_controller.go | Implements spec-driven replicas, termination grace period default, and new status-condition validations. |
| cmd/thv-operator/api/v1alpha1/mcpserver_types.go | Adds new MCPServer condition type/reason constants for stdio replica capping and session storage warnings. |
| cmd/thv-operator/controllers/mcpserver_replicas_test.go | Updates default replica expectation to nil and adds unit tests for replica resolution, termination grace period, and conditions. |
| cmd/thv-operator/controllers/mcpserver_pod_template_test.go | Removes now-redundant int64Ptr helper (provided by controller package). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
31e22a5 to
6788d84
Compare
Add SessionStorageConfig struct and scaling fields to MCPServer and VirtualMCPServer CRDs as the foundation for horizontal scaling support. - Add SessionStorageConfig struct with Provider, Address, DB, KeyPrefix, and PasswordRef fields; CEL rule enforces address is required for redis - Add Replicas *int32, BackendReplicas *int32, SessionStorage to MCPServerSpec - Add Replicas *int32, SessionStorage to VirtualMCPServerSpec - Nil pointer fields allow HPA to manage replicas without operator interference - Regenerate zz_generated.deepcopy.go and CRD manifests Closes: #4206
- resolveDeploymentReplicas: nil-passthrough preserves HPA compatibility, stdio cap enforced at 1 as defense-in-depth - Add terminationGracePeriodSeconds (30s default) to proxy runner pod spec - validateStdioReplicaCap: Warning condition when stdio + spec.replicas > 1 - validateSessionStorageForReplicas: Warning condition when replicas > 1 without Redis session storage configured Closes #4217
Summary
Fixes #4217
Type of change
Test plan
task test)task test-e2e)task lint-fix)Changes
Does this introduce a user-facing change?
Special notes for reviewers