From 9cead90ac4008543a48c3c96d93d8ac412adbeff Mon Sep 17 00:00:00 2001 From: xing Date: Sun, 26 Jul 2026 12:47:13 +0800 Subject: [PATCH] fix(operator): add default resource limits for MCPRemoteProxy 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 #3131 --- .../controllers/mcpremoteproxy_controller.go | 5 +- .../controllers/mcpremoteproxy_deployment.go | 10 +- .../mcpremoteproxy_deployment_test.go | 109 ++++++++++++++++++ .../pkg/controllerutil/resources.go | 55 +++++++++ .../pkg/controllerutil/resources_test.go | 32 +++++ 5 files changed, 208 insertions(+), 3 deletions(-) diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go index 2dc32672bc..ebb6852ca2 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go @@ -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) { return true } diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go b/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go index a1fa5da8dc..8cf793d4c2 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go @@ -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) @@ -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 diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_deployment_test.go b/cmd/thv-operator/controllers/mcpremoteproxy_deployment_test.go index 6b6ad56a03..808f5a8dc4 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_deployment_test.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_deployment_test.go @@ -24,6 +24,7 @@ import ( 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" @@ -79,6 +80,12 @@ func TestDeploymentForMCPRemoteProxy(t *testing.T) { // 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]) }, }, { @@ -106,6 +113,30 @@ func TestDeploymentForMCPRemoteProxy(t *testing.T) { 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", @@ -333,6 +364,84 @@ func TestBuildResourceRequirements(t *testing.T) { } } + +// 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) { + 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{} + assert.True(t, reconciler.deploymentNeedsUpdate(t.Context(), deployment, proxy, "test-checksum")) +} + // TestBuildHeaderForwardSecretEnvVars tests the buildHeaderForwardSecretEnvVars function func TestBuildHeaderForwardSecretEnvVars(t *testing.T) { t.Parallel() diff --git a/cmd/thv-operator/pkg/controllerutil/resources.go b/cmd/thv-operator/pkg/controllerutil/resources.go index abedfad873..23adc81c69 100644 --- a/cmd/thv-operator/pkg/controllerutil/resources.go +++ b/cmd/thv-operator/pkg/controllerutil/resources.go @@ -17,6 +17,15 @@ import ( "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. +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 { @@ -45,6 +54,52 @@ func BuildResourceRequirements(resourceSpec mcpv1beta1.ResourceRequirements) cor 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 { + 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( diff --git a/cmd/thv-operator/pkg/controllerutil/resources_test.go b/cmd/thv-operator/pkg/controllerutil/resources_test.go index 6405014336..3e053016bf 100644 --- a/cmd/thv-operator/pkg/controllerutil/resources_test.go +++ b/cmd/thv-operator/pkg/controllerutil/resources_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" "github.com/stacklok/toolhive/pkg/secrets" ) @@ -353,3 +354,34 @@ func TestMergeStringMaps(t *testing.T) { }) } } + +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]) +} +