Wire spec-driven replicas and session storage validation for MCPServer#4364
Wire spec-driven replicas and session storage validation for MCPServer#4364yrobla wants to merge 1 commit intofeat/thv-0047-crd-type-changesfrom
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 @@
## feat/thv-0047-crd-type-changes #4364 +/- ##
==================================================================
- Coverage 69.58% 69.54% -0.04%
==================================================================
Files 480 480
Lines 48664 48700 +36
==================================================================
+ Hits 33861 33870 +9
- Misses 12195 12224 +29
+ Partials 2608 2606 -2 ☔ 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.
- 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
| require.NoError(t, err) | ||
| assert.Equal(t, int32(1), *deployment.Spec.Replicas, | ||
| "Default deployment should start with 1 replica") | ||
| assert.Nil(t, deployment.Spec.Replicas, |
There was a problem hiding this comment.
nitpick: Test name TestDefaultCreationHasOneReplica no longer matches the assertion — it now asserts nil replicas (hands-off mode), not 1. Something like TestDefaultCreationHasNilReplicas would be clearer.
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