wire spec-driven replicas and Redis password for VirtualMCPServer#4367
wire spec-driven replicas and Redis password for VirtualMCPServer#4367
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the ToolHive operator’s VirtualMCPServer Deployment wiring to support HPA-compatible scaling (nil-passthrough replicas), adds a default graceful shutdown period for vMCP pods, and injects a Redis password env var when session storage is configured for Redis.
Changes:
- Make
Deployment.Spec.Replicasspec-driven with nil-passthrough to avoid fighting HPAs whenspec.replicasis unset. - Set
TerminationGracePeriodSeconds(30s) on the vMCP pod spec. - Inject
THV_SESSION_REDIS_PASSWORDfromspec.sessionStorage.passwordRefwhenprovider == "redis", and sync replica drift on update whenspec.replicasis set.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/thv-operator/controllers/virtualmcpserver_deployment.go | Implements nil-passthrough replicas, adds termination grace period, and injects Redis password env var. |
| cmd/thv-operator/controllers/virtualmcpserver_controller.go | Updates reconciliation logic to sync replicas only when spec.replicas is set; adds replica drift detection. |
| cmd/thv-operator/controllers/virtualmcpserver_deployment_test.go | Updates expectations for nil replicas in the deployment builder test. |
| cmd/thv-operator/controllers/virtualmcpserver_controller_test.go | Updates expectations for nil replicas in ensureDeployment test. |
| cmd/thv-operator/controllers/mcpserver_pod_template_test.go | Removes local int64Ptr helper (now provided by package-level helper). |
| cmd/thv-operator/controllers/mcpserver_controller.go | Adds shared int64Ptr helper used by vMCP deployment builder (and tests). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4367 +/- ##
==========================================
+ Coverage 69.43% 69.47% +0.03%
==========================================
Files 485 485
Lines 49493 49539 +46
==========================================
+ Hits 34367 34415 +48
+ Misses 12466 12458 -8
- Partials 2660 2666 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new 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
- Nil-passthrough for Deployment.Spec.Replicas (hands-off when spec.replicas is nil, spec-driven when set) for HPA compatibility - Add terminationGracePeriodSeconds (30s) to vMCP pod spec - Add buildRedisPasswordEnvVar: injects THV_SESSION_REDIS_PASSWORD from SessionStorageConfig.PasswordRef when provider is redis - Sync Spec.Replicas on update when spec.replicas is non-nil - deploymentNeedsUpdate detects replica drift for spec-driven scaling Closes: #4215
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cmd/thv-operator/controllers/mcpserver_controller.go:472
- Same pattern here: the controller only updates Deployment.Spec.Replicas when spec.replicas is non-nil. If spec.replicas is later unset (nil) to hand scaling back to an HPA/external controller, the existing Deployment.Spec.Replicas will not be cleared and may stay pinned to the previous value. Consider explicitly clearing deployment.Spec.Replicas when the desired replicas resolves to nil.
deployment.Annotations = ctrlutil.MergeAnnotations(newDeployment.Annotations, deployment.Annotations)
if newDeployment.Spec.Replicas != nil {
deployment.Spec.Replicas = newDeployment.Spec.Replicas
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Fixes #4215
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