Add default resource limits for MCPRemoteProxy - #5998
Conversation
Apply proxy-runner defaults when MCPRemoteProxy omits resource requests/limits, merge user overrides on top of those defaults, and keep deployment drift detection aligned with the same effective resource calculation. Closes stacklok#3131
…-mcpremoteproxy-1785037451518
github-actions
Bot
added
size/S
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5998 +/- ##
==========================================
+ Coverage 72.16% 72.19% +0.03%
==========================================
Files 721 721
Lines 75069 75100 +31
==========================================
+ Hits 54174 54222 +48
+ Misses 17033 17018 -15
+ Partials 3862 3860 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@clxstart Thanks for the PR! You'll need to resolve the lint failures first before we can merge |
ChrisJBurns
left a comment
ChrisJBurns
left a comment
There was a problem hiding this comment.
Automated multi-agent review — PR #5998
Thanks for this — the field-level merge is a genuine improvement over the VirtualMCPServer precedent it mirrors (which ignores user overrides entirely), the merge helper deep-copies quantities safely, and the happy-path test coverage is good.
This is a COMMENT review (no HIGH-severity design blockers). The inline comments below are the findings that scored consensus ≥ 7 across three specialist reviewers (Kubernetes operator, Go conventions, security). In order of value:
gofmtfails CI today —task lint-fixfixes it.- Drift comparison should use
equality.Semantic.DeepEqual— one-line, in-scope fix that avoids a reconcile loop on non-canonical user overrides. - Upgrade behavior (silent rollout + memory cap on existing proxies) deserves a user-facing-change note.
- Partial-override
request > limitedge case. - Idempotency test doesn't cover the canonicalization path.
Lower-severity items (defaults 2× smaller than vMCP, generic-helper doc comment, unreachable nil-ing, ephemeral-storage/PID scope, stale doc comment) were surfaced in the full terminal review but omitted here per the ≥ 7 consensus filter.
| expectedResources := resourceRequirementsForRemoteProxy(proxy) | ||
| if !reflect.DeepEqual(container.Resources, expectedResources) { |
There was a problem hiding this comment.
[MEDIUM · consensus 8/10] Drift comparison uses reflect.DeepEqual on resource.Quantity.
Semantically-equal quantities built from different source strings are not reflect.DeepEqual — resource.Quantity carries an unexported cached-string field. A valid user override like limits.cpu: "0.2" (canonicalized by the apiserver to 200m), or 1024Mi↔1Gi, 1000m↔1, makes generatedContainerNeedsUpdate report perpetual drift → a reconcile/patch loop that hammers the apiserver. The default values themselves survive a round-trip, so the empty-spec path this PR adds is safe; the loop triggers on non-canonical user overrides.
The sibling remoteProxyContainerFieldsNeedUpdate (this file) and mcpserver_controller.go both already use equality.Semantic.DeepEqual. Since this PR's own comment says drift detection should "stay consistent," switching here is in-scope (equality is already imported).
| expectedResources := resourceRequirementsForRemoteProxy(proxy) | |
| if !reflect.DeepEqual(container.Resources, expectedResources) { | |
| expectedResources := resourceRequirementsForRemoteProxy(proxy) | |
| if !equality.Semantic.DeepEqual(container.Resources, expectedResources) { |
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
[MEDIUM · consensus 9/10] gofmt will fail CI.
gofmt -d against the committed head reports this file (this extra blank line — two consecutive blanks before TestResourceRequirementsForRemoteProxy) and pkg/controllerutil/resources_test.go (a trailing blank line at EOF) both WOULD REFORMAT, so task lint will fail. .claude/rules/go-style.md requires task lint-fix before submitting — running it fixes both.
| assert.False(t, reconciler.deploymentNeedsUpdate(t.Context(), deployment, proxy, "test-checksum")) | ||
|
|
||
| // Simulate a deployment created before defaults existed. | ||
| deployment.Spec.Template.Spec.Containers[0].Resources = corev1.ResourceRequirements{} |
There was a problem hiding this comment.
[MEDIUM · consensus 8/10] Undocumented user-facing change: existing proxies roll and gain a hard memory cap on upgrade.
This case (empty Resources → needs update) correctly encodes the intended migration, but it also means every pre-existing MCPRemoteProxy deployment will roll once after the operator upgrade and gain a 256Mi memory / 200m CPU cap it never had before. A proxy whose steady-state working set exceeds 256Mi can be silently OOMKilled post-upgrade with no operator-facing signal.
Please call this out in the PR's "Does this introduce a user-facing change?" section / release notes, and consider advising operators to audit heavy proxies' memory usage (and pre-set spec.resources) before upgrading.
|
|
||
| // MergeResourceRequirements merges user-provided resource requirements on top of | ||
| // defaults. User values take precedence for any field that is explicitly set. | ||
| func MergeResourceRequirements(defaults, user corev1.ResourceRequirements) corev1.ResourceRequirements { |
There was a problem hiding this comment.
[MEDIUM · consensus 7/10] Partial override can synthesize request > limit.
Because each side is filled independently from defaults, a user who sets only requests.memory: 512Mi (and no memory limit) inherits the default limits.memory: 256Mi → request (512Mi) > limit (256Mi), which Kubernetes rejects at Deployment admission. Same for CPU (requests.cpu: 500m + default limits.cpu: 200m).
Per .claude/rules/operator.md ("Separate terminal from transient errors"), a bad spec should surface as a Valid=False condition with return nil — but as written this fails the Deployment write and requeues with exponential backoff, root cause buried. Consider detecting request > limit after the merge and surfacing a terminal condition, and/or adding a CEL +kubebuilder:validation:XValidation on the CRD ResourceRequirements type to reject it at admission.
| } | ||
|
|
||
| // TestMCPRemoteProxyDeploymentNeedsUpdate_Resources detects resource drift with defaults | ||
| func TestMCPRemoteProxyDeploymentNeedsUpdate_Resources(t *testing.T) { |
There was a problem hiding this comment.
[MEDIUM · consensus 7/10] Idempotency test doesn't exercise the path that can break.
This test passes the just-built deployment straight into deploymentNeedsUpdate, so the comparison holds trivially (identical in-memory Quantity objects) and only the empty-spec case is covered — it would keep passing even with the reflect.DeepEqual issue in generatedContainerNeedsUpdate present.
Add a subtest with a non-canonical override (e.g. limits.cpu: "0.2") whose quantities are re-parsed via resource.MustParse(q.String()) to simulate an apiserver round-trip, then assert deploymentNeedsUpdate == false. It fails under reflect.DeepEqual and passes under equality.Semantic.DeepEqual, load-bearing the fix above. An envtest that reads the Deployment back through the client would be the most faithful (matches the team's envtest-over-chainsaw preference).
2 participants
Summary
MCPRemoteProxyomitsspec.resourcesContext
Fixes #3131
Without defaults, remote proxy containers can run unbounded when
spec.resourcesis empty. This mirrors the existing VirtualMCPServer/MCPServer defaulting intent and keeps create/update paths consistent.Default values
50m/200m64Mi/256MiTest plan
go test ./cmd/thv-operator/pkg/controllerutil -run 'TestBuildDefaultProxyRunnerResourceRequirements|TestMergeResourceRequirements|TestEnsureRequiredEnvVars'go test ./cmd/thv-operator/controllers -run 'TestDeploymentForMCPRemoteProxy|TestBuildResourceRequirements|TestResourceRequirementsForRemoteProxy|TestMCPRemoteProxyDeploymentNeedsUpdate_Resources'spec.resourcesgets defaults