wire spec-driven replicas and Redis password for VirtualMCPServer#4367
wire spec-driven replicas and Redis password for VirtualMCPServer#4367yrobla wants to merge 1 commit intofeat/thv-0047-crd-type-changesfrom
Conversation
yrobla
requested review from
ChrisJBurns,
JAORMX,
jerm-dro and
jhrozek
as code owners
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 @@
## feat/thv-0047-crd-type-changes #4367 +/- ##
===============================================================
Coverage 69.58% 69.59%
===============================================================
Files 480 480
Lines 48664 48665 +1
===============================================================
+ Hits 33861 33866 +5
- Misses 12195 12196 +1
+ Partials 2608 2603 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- 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
github-actions
bot
added
size/S
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.
| // buildRedisPasswordEnvVar returns the THV_SESSION_REDIS_PASSWORD env var when | ||
| // sessionStorage.provider == "redis" and passwordRef is set; returns nil otherwise. | ||
| func (*VirtualMCPServerReconciler) buildRedisPasswordEnvVar(vmcp *mcpv1alpha1.VirtualMCPServer) []corev1.EnvVar { | ||
| if vmcp.Spec.SessionStorage == nil || | ||
| vmcp.Spec.SessionStorage.Provider != "redis" || | ||
| vmcp.Spec.SessionStorage.PasswordRef == nil { | ||
| return nil | ||
| } | ||
| return []corev1.EnvVar{{ | ||
| Name: "THV_SESSION_REDIS_PASSWORD", | ||
| ValueFrom: &corev1.EnvVarSource{ | ||
| SecretKeyRef: &corev1.SecretKeySelector{ | ||
| LocalObjectReference: corev1.LocalObjectReference{ | ||
| Name: vmcp.Spec.SessionStorage.PasswordRef.Name, | ||
| }, | ||
| Key: vmcp.Spec.SessionStorage.PasswordRef.Key, | ||
| }, | ||
| }, | ||
| }} | ||
| } | ||
|
|
There was a problem hiding this comment.
question: Does this follow the established pattern for how the proxy runner consumes the Redis password? Want to make sure the env var name and secret mounting approach are consistent.
| deployment.Annotations = ctrlutil.MergeAnnotations(newDeployment.Annotations, deployment.Annotations) | ||
| if newDeployment.Spec.Replicas != nil { | ||
| deployment.Spec.Replicas = newDeployment.Spec.Replicas | ||
| } |
There was a problem hiding this comment.
question: MCPServer (#4364) adds a SessionStorageWarning condition when replicas > 1 without Redis session storage configured. Is the omission of an equivalent condition for VirtualMCPServer intentional?
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