Skip to content

Add default resource limits for MCPRemoteProxy #5998

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
clxstart wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add default resource limits for MCPRemoteProxy #5998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9cead90
fix(operator): add default resource limits for MCPRemoteProxy
clxstart Jul 26, 2026
42da122
Merge branch 'main' into openmeta/3131-add-default-resource-limits-to…
ChrisJBurns Jul 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions cmd/thv-operator/controllers/mcpremoteproxy_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1639,8 +1639,9 @@ func (r *MCPRemoteProxyReconciler) generatedContainerNeedsUpdate(
return true
}

// Check if resources have changed
expectedResources := ctrlutil.BuildResourceRequirements(proxy.Spec.Resources)
// Check if resources have changed. Use the same default+override merge as
// deployment generation so drift detection stays consistent.
expectedResources := resourceRequirementsForRemoteProxy(proxy)
if !reflect.DeepEqual(container.Resources, expectedResources) {
Comment on lines +1644 to 1645

@ChrisJBurns ChrisJBurns Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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).

Suggested change
expectedResources := resourceRequirementsForRemoteProxy(proxy)
if !reflect.DeepEqual(container.Resources, expectedResources) {
expectedResources := resourceRequirementsForRemoteProxy(proxy)
if !equality.Semantic.DeepEqual(container.Resources, expectedResources) {

return true
}
Expand Down
10 changes: 9 additions & 1 deletion cmd/thv-operator/controllers/mcpremoteproxy_deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func (r *MCPRemoteProxyReconciler) deploymentForMCPRemoteProxy(
volumeMounts = append(volumeMounts, authServerMounts...)
env = append(env, authServerEnvVars...)
}
resources := ctrlutil.BuildResourceRequirements(proxy.Spec.Resources)
resources := resourceRequirementsForRemoteProxy(proxy)
deploymentLabels, deploymentAnnotations := r.buildDeploymentMetadata(ls, proxy)
deploymentTemplateLabels, deploymentTemplateAnnotations := r.buildPodTemplateMetadata(ls, proxy, runConfigChecksum)
podSecurityContext, containerSecurityContext := r.buildSecurityContexts(ctx, proxy)
Expand Down Expand Up @@ -121,6 +121,14 @@ func (r *MCPRemoteProxyReconciler) deploymentForMCPRemoteProxy(
return dep
}

// resourceRequirementsForRemoteProxy returns effective container resources for
// an MCPRemoteProxy, applying proxy-runner defaults and merging any user overrides.
func resourceRequirementsForRemoteProxy(proxy *mcpv1beta1.MCPRemoteProxy) corev1.ResourceRequirements {
defaultResources := ctrlutil.BuildDefaultProxyRunnerResourceRequirements()
userResources := ctrlutil.BuildResourceRequirements(proxy.Spec.Resources)
return ctrlutil.MergeResourceRequirements(defaultResources, userResources)
}

// buildContainerArgs builds the container arguments for the proxy
func (*MCPRemoteProxyReconciler) buildContainerArgs() []string {
// The third argument is required by proxyrunner command signature but is ignored
Expand Down
109 changes: 109 additions & 0 deletions cmd/thv-operator/controllers/mcpremoteproxy_deployment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
rbacv1 "k8s.io/api/rbac/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -79,6 +80,12 @@
// Verify service account
assert.Equal(t, proxyRunnerServiceAccountNameForRemoteProxy("basic-proxy"),
dep.Spec.Template.Spec.ServiceAccountName)

// Verify default resource requirements when Spec.Resources is empty
assert.Equal(t, resource.MustParse("50m"), container.Resources.Requests[corev1.ResourceCPU])
assert.Equal(t, resource.MustParse("64Mi"), container.Resources.Requests[corev1.ResourceMemory])
assert.Equal(t, resource.MustParse("200m"), container.Resources.Limits[corev1.ResourceCPU])
assert.Equal(t, resource.MustParse("256Mi"), container.Resources.Limits[corev1.ResourceMemory])
},
},
{
Expand Down Expand Up @@ -106,6 +113,30 @@
assert.Equal(t, "128Mi", container.Resources.Requests.Memory().String())
},
},
{
name: "with partial resource overrides",
proxy: v1beta1test.NewMCPRemoteProxy("partial-resources-proxy", "default",
v1beta1test.MutateRemoteProxy(func(p *mcpv1beta1.MCPRemoteProxy) {
p.Spec.Resources = mcpv1beta1.ResourceRequirements{
Limits: mcpv1beta1.ResourceList{
CPU: "1",
},
Requests: mcpv1beta1.ResourceList{
Memory: "128Mi",
},
}
}),
),
validate: func(t *testing.T, dep *appsv1.Deployment) {
t.Helper()
container := dep.Spec.Template.Spec.Containers[0]
// User-provided fields take precedence; missing fields keep defaults.
assert.Equal(t, resource.MustParse("50m"), container.Resources.Requests[corev1.ResourceCPU])
assert.Equal(t, resource.MustParse("128Mi"), container.Resources.Requests[corev1.ResourceMemory])
assert.Equal(t, resource.MustParse("1"), container.Resources.Limits[corev1.ResourceCPU])
assert.Equal(t, resource.MustParse("256Mi"), container.Resources.Limits[corev1.ResourceMemory])
},
},
{
name: "with resource overrides",
proxy: v1beta1test.NewMCPRemoteProxy("override-proxy", "default",
Expand Down Expand Up @@ -333,6 +364,84 @@
}
}


Check failure on line 367 in cmd/thv-operator/controllers/mcpremoteproxy_deployment_test.go

View workflow job for this annotation

GitHub Actions / Linting / Lint Go Code 

File is not properly formatted (gci)

@ChrisJBurns ChrisJBurns Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change

// TestResourceRequirementsForRemoteProxy tests default injection and merge behavior
func TestResourceRequirementsForRemoteProxy(t *testing.T) {
t.Parallel()

tests := []struct {
name string
proxy *mcpv1beta1.MCPRemoteProxy
validate func(*testing.T, corev1.ResourceRequirements)
}{
{
name: "defaults when resources omitted",
proxy: v1beta1test.NewMCPRemoteProxy("defaults-proxy", "default"),
validate: func(t *testing.T, res corev1.ResourceRequirements) {
t.Helper()
assert.Equal(t, resource.MustParse("50m"), res.Requests[corev1.ResourceCPU])
assert.Equal(t, resource.MustParse("64Mi"), res.Requests[corev1.ResourceMemory])
assert.Equal(t, resource.MustParse("200m"), res.Limits[corev1.ResourceCPU])
assert.Equal(t, resource.MustParse("256Mi"), res.Limits[corev1.ResourceMemory])
},
},
{
name: "user overrides win over defaults",
proxy: v1beta1test.NewMCPRemoteProxy("override-resources-proxy", "default",
v1beta1test.MutateRemoteProxy(func(p *mcpv1beta1.MCPRemoteProxy) {
p.Spec.Resources = mcpv1beta1.ResourceRequirements{
Limits: mcpv1beta1.ResourceList{
CPU: "1",
Memory: "512Mi",
},
Requests: mcpv1beta1.ResourceList{
CPU: "100m",
Memory: "128Mi",
},
}
}),
),
validate: func(t *testing.T, res corev1.ResourceRequirements) {
t.Helper()
assert.Equal(t, resource.MustParse("100m"), res.Requests[corev1.ResourceCPU])
assert.Equal(t, resource.MustParse("128Mi"), res.Requests[corev1.ResourceMemory])
assert.Equal(t, resource.MustParse("1"), res.Limits[corev1.ResourceCPU])
assert.Equal(t, resource.MustParse("512Mi"), res.Limits[corev1.ResourceMemory])
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
result := resourceRequirementsForRemoteProxy(tt.proxy)
if tt.validate != nil {
tt.validate(t, result)
}
})
}
}

// TestMCPRemoteProxyDeploymentNeedsUpdate_Resources detects resource drift with defaults
func TestMCPRemoteProxyDeploymentNeedsUpdate_Resources(t *testing.T) {

@ChrisJBurns ChrisJBurns Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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).

t.Parallel()

scheme := testutil.NewScheme(t)
proxy := v1beta1test.NewMCPRemoteProxy("resources-drift-proxy", "default")
reconciler := &MCPRemoteProxyReconciler{
Scheme: scheme,
PlatformDetector: ctrlutil.NewSharedPlatformDetector(),
}

deployment := reconciler.deploymentForMCPRemoteProxy(t.Context(), proxy, "test-checksum")
require.NotNil(t, deployment)
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{}

@ChrisJBurns ChrisJBurns Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

assert.True(t, reconciler.deploymentNeedsUpdate(t.Context(), deployment, proxy, "test-checksum"))
}

// TestBuildHeaderForwardSecretEnvVars tests the buildHeaderForwardSecretEnvVars function
func TestBuildHeaderForwardSecretEnvVars(t *testing.T) {
t.Parallel()
Expand Down
55 changes: 55 additions & 0 deletions cmd/thv-operator/pkg/controllerutil/resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,15 @@
"github.com/stacklok/toolhive/pkg/secrets"
)

// Default proxy-runner resource values used when a CR omits resource requests/limits.
// These keep remote proxy containers bounded while remaining overrideable per CR.

Check failure on line 21 in cmd/thv-operator/pkg/controllerutil/resources.go

View workflow job for this annotation

GitHub Actions / Spellcheck / Codespell 

overrideable ==> overridable
const (
DefaultProxyRunnerCPURequest = "50m"
DefaultProxyRunnerCPULimit = "200m"
DefaultProxyRunnerMemoryRequest = "64Mi"
DefaultProxyRunnerMemoryLimit = "256Mi"
)

// BuildResourceRequirements builds Kubernetes resource requirements from CRD spec
// Shared between MCPServer and MCPRemoteProxy
func BuildResourceRequirements(resourceSpec mcpv1beta1.ResourceRequirements) corev1.ResourceRequirements {
Expand Down Expand Up @@ -45,6 +54,52 @@
return resources
}

// BuildDefaultProxyRunnerResourceRequirements returns the default resource
// requirements for proxy-runner containers (MCPRemoteProxy).
func BuildDefaultProxyRunnerResourceRequirements() corev1.ResourceRequirements {
return corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse(DefaultProxyRunnerCPURequest),
corev1.ResourceMemory: resource.MustParse(DefaultProxyRunnerMemoryRequest),
},
Limits: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse(DefaultProxyRunnerCPULimit),
corev1.ResourceMemory: resource.MustParse(DefaultProxyRunnerMemoryLimit),
},
}
}

// 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 {

@ChrisJBurns ChrisJBurns Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

merged := corev1.ResourceRequirements{
Requests: corev1.ResourceList{},
Limits: corev1.ResourceList{},
}

for resourceName, quantity := range defaults.Requests {
merged.Requests[resourceName] = quantity.DeepCopy()
}
for resourceName, quantity := range defaults.Limits {
merged.Limits[resourceName] = quantity.DeepCopy()
}
for resourceName, quantity := range user.Requests {
merged.Requests[resourceName] = quantity.DeepCopy()
}
for resourceName, quantity := range user.Limits {
merged.Limits[resourceName] = quantity.DeepCopy()
}

if len(merged.Requests) == 0 {
merged.Requests = nil
}
if len(merged.Limits) == 0 {
merged.Limits = nil
}

return merged
}

// BuildHealthProbe builds a Kubernetes health probe configuration
// Shared between MCPServer and MCPRemoteProxy
func BuildHealthProbe(
Expand Down
32 changes: 32 additions & 0 deletions cmd/thv-operator/pkg/controllerutil/resources_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

"github.com/stretchr/testify/assert"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"

"github.com/stacklok/toolhive/pkg/secrets"
)
Expand Down Expand Up @@ -353,3 +354,34 @@
})
}
}

func TestBuildDefaultProxyRunnerResourceRequirements(t *testing.T) {
t.Parallel()

res := BuildDefaultProxyRunnerResourceRequirements()
assert.Equal(t, resource.MustParse("50m"), res.Requests[corev1.ResourceCPU])
assert.Equal(t, resource.MustParse("64Mi"), res.Requests[corev1.ResourceMemory])
assert.Equal(t, resource.MustParse("200m"), res.Limits[corev1.ResourceCPU])
assert.Equal(t, resource.MustParse("256Mi"), res.Limits[corev1.ResourceMemory])
}

func TestMergeResourceRequirements(t *testing.T) {
t.Parallel()

defaults := BuildDefaultProxyRunnerResourceRequirements()
user := corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceMemory: resource.MustParse("128Mi"),
},
Limits: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("1"),
},
}

merged := MergeResourceRequirements(defaults, user)
assert.Equal(t, resource.MustParse("50m"), merged.Requests[corev1.ResourceCPU])
assert.Equal(t, resource.MustParse("128Mi"), merged.Requests[corev1.ResourceMemory])
assert.Equal(t, resource.MustParse("1"), merged.Limits[corev1.ResourceCPU])
assert.Equal(t, resource.MustParse("256Mi"), merged.Limits[corev1.ResourceMemory])
}

Check failure on line 387 in cmd/thv-operator/pkg/controllerutil/resources_test.go

View workflow job for this annotation

GitHub Actions / Linting / Lint Go Code 

File is not properly formatted (gci)
Loading