From 19618aa92d81869f9d94aa328517ea2ec48279c8 Mon Sep 17 00:00:00 2001 From: chandru-mck-2002 Date: Mon, 22 Dec 2025 12:40:56 +0530 Subject: [PATCH] updated resource limit for MCPRemoteProxy --- .../controllers/mcpremoteproxy_controller.go | 2 +- .../controllers/mcpremoteproxy_deployment.go | 11 +- .../mcpremoteproxy_deployment_test.go | 183 ++++++++++++++++++ .../pkg/controllerutil/resources.go | 57 ++++++ 4 files changed, 251 insertions(+), 2 deletions(-) diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go index e7d209d467..09226c1a00 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go @@ -688,7 +688,7 @@ func (r *MCPRemoteProxyReconciler) containerNeedsUpdate( } // Check if resources have changed - expectedResources := ctrlutil.BuildResourceRequirements(proxy.Spec.Resources) + 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 14edf325ee..c43d0cdc68 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go @@ -28,7 +28,7 @@ func (r *MCPRemoteProxyReconciler) deploymentForMCPRemoteProxy( args := r.buildContainerArgs() volumeMounts, volumes := r.buildVolumesForProxy(proxy) env := r.buildEnvVarsForProxy(ctx, proxy) - 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) @@ -79,6 +79,15 @@ func (r *MCPRemoteProxyReconciler) deploymentForMCPRemoteProxy( return dep } +// resourceRequirementsForRemoteProxy returns the resource requirements for the MCPRemoteProxy. +// It merges default proxy runner resources (50m/200m CPU, 64Mi/256Mi memory) with user-provided values. +// User-provided values take precedence over defaults. +func resourceRequirementsForRemoteProxy(proxy *mcpv1alpha1.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 5d954db84a..20ab1f6029 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" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -83,6 +84,12 @@ func TestDeploymentForMCPRemoteProxy(t *testing.T) { // Verify service account assert.Equal(t, proxyRunnerServiceAccountNameForRemoteProxy("basic-proxy"), dep.Spec.Template.Spec.ServiceAccountName) + + // Verify default resources are applied when none are specified + assert.Equal(t, "50m", container.Resources.Requests.Cpu().String()) + assert.Equal(t, "64Mi", container.Resources.Requests.Memory().String()) + assert.Equal(t, "200m", container.Resources.Limits.Cpu().String()) + assert.Equal(t, "256Mi", container.Resources.Limits.Memory().String()) }, }, { @@ -336,6 +343,182 @@ func TestBuildResourceRequirements(t *testing.T) { } } +// TestResourceRequirementsForRemoteProxy tests resource requirements for remote proxy +func TestResourceRequirementsForRemoteProxy(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + proxy *mcpv1alpha1.MCPRemoteProxy + validate func(*testing.T, corev1.ResourceRequirements) + }{ + { + name: "no user resources - uses defaults", + proxy: &mcpv1alpha1.MCPRemoteProxy{ + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + Resources: mcpv1alpha1.ResourceRequirements{}, + }, + }, + validate: func(t *testing.T, res corev1.ResourceRequirements) { + t.Helper() + assert.Equal(t, "50m", res.Requests.Cpu().String()) + assert.Equal(t, "64Mi", res.Requests.Memory().String()) + assert.Equal(t, "200m", res.Limits.Cpu().String()) + assert.Equal(t, "256Mi", res.Limits.Memory().String()) + }, + }, + { + name: "user resources override defaults", + proxy: &mcpv1alpha1.MCPRemoteProxy{ + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + Resources: mcpv1alpha1.ResourceRequirements{ + Limits: mcpv1alpha1.ResourceList{ + CPU: "1", + Memory: "512Mi", + }, + Requests: mcpv1alpha1.ResourceList{ + CPU: "100m", + Memory: "128Mi", + }, + }, + }, + }, + validate: func(t *testing.T, res corev1.ResourceRequirements) { + t.Helper() + // User values should override defaults + assert.Equal(t, "100m", res.Requests.Cpu().String()) + assert.Equal(t, "128Mi", res.Requests.Memory().String()) + assert.Equal(t, "1", res.Limits.Cpu().String()) + assert.Equal(t, "512Mi", res.Limits.Memory().String()) + }, + }, + { + name: "partial user resources - only CPU specified", + proxy: &mcpv1alpha1.MCPRemoteProxy{ + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + Resources: mcpv1alpha1.ResourceRequirements{ + Limits: mcpv1alpha1.ResourceList{ + CPU: "500m", + }, + Requests: mcpv1alpha1.ResourceList{ + CPU: "100m", + }, + }, + }, + }, + validate: func(t *testing.T, res corev1.ResourceRequirements) { + t.Helper() + // CPU should be user-provided + assert.Equal(t, "100m", res.Requests.Cpu().String()) + assert.Equal(t, "500m", res.Limits.Cpu().String()) + // Memory should use defaults + assert.Equal(t, "64Mi", res.Requests.Memory().String()) + assert.Equal(t, "256Mi", res.Limits.Memory().String()) + }, + }, + } + + 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) + } + }) + } +} + +// TestBuildDefaultProxyRunnerResourceRequirements tests default resource requirements +func TestBuildDefaultProxyRunnerResourceRequirements(t *testing.T) { + t.Parallel() + + res := ctrlutil.BuildDefaultProxyRunnerResourceRequirements() + + assert.Equal(t, "50m", res.Requests.Cpu().String()) + assert.Equal(t, "64Mi", res.Requests.Memory().String()) + assert.Equal(t, "200m", res.Limits.Cpu().String()) + assert.Equal(t, "256Mi", res.Limits.Memory().String()) +} + +// TestMergeResourceRequirements tests resource requirements merging +func TestMergeResourceRequirements(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + defaultRes corev1.ResourceRequirements + userRes corev1.ResourceRequirements + validate func(*testing.T, corev1.ResourceRequirements) + }{ + { + name: "user values override defaults", + defaultRes: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("50m"), + corev1.ResourceMemory: resource.MustParse("64Mi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("200m"), + corev1.ResourceMemory: resource.MustParse("256Mi"), + }, + }, + userRes: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("500m"), + }, + }, + validate: func(t *testing.T, res corev1.ResourceRequirements) { + t.Helper() + // CPU should be user-provided + assert.Equal(t, "100m", res.Requests.Cpu().String()) + assert.Equal(t, "500m", res.Limits.Cpu().String()) + // Memory should be from defaults + assert.Equal(t, "64Mi", res.Requests.Memory().String()) + assert.Equal(t, "256Mi", res.Limits.Memory().String()) + }, + }, + { + name: "empty user resources - returns defaults", + defaultRes: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("50m"), + corev1.ResourceMemory: resource.MustParse("64Mi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("200m"), + corev1.ResourceMemory: resource.MustParse("256Mi"), + }, + }, + userRes: corev1.ResourceRequirements{}, + validate: func(t *testing.T, res corev1.ResourceRequirements) { + t.Helper() + assert.Equal(t, "50m", res.Requests.Cpu().String()) + assert.Equal(t, "64Mi", res.Requests.Memory().String()) + assert.Equal(t, "200m", res.Limits.Cpu().String()) + assert.Equal(t, "256Mi", res.Limits.Memory().String()) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + result := ctrlutil.MergeResourceRequirements(tt.defaultRes, tt.userRes) + + if tt.validate != nil { + tt.validate(t, result) + } + }) + } +} + // TestBuildHealthProbe tests health probe building func TestBuildHealthProbe(t *testing.T) { t.Parallel() diff --git a/cmd/thv-operator/pkg/controllerutil/resources.go b/cmd/thv-operator/pkg/controllerutil/resources.go index 6dcbe784c2..728e3b880b 100644 --- a/cmd/thv-operator/pkg/controllerutil/resources.go +++ b/cmd/thv-operator/pkg/controllerutil/resources.go @@ -161,3 +161,60 @@ func CreateProxyServiceURL(resourceName, namespace string, port int32) string { func ProxyRunnerServiceAccountName(resourceName string) string { return fmt.Sprintf("%s-proxy-runner", resourceName) } + +// BuildDefaultProxyRunnerResourceRequirements returns default resource requirements for proxy runners. +// Defaults: 50m/200m CPU (requests/limits), 64Mi/256Mi memory (requests/limits). +// These defaults provide reasonable resource limits to prevent containers from monopolizing cluster resources. +func BuildDefaultProxyRunnerResourceRequirements() corev1.ResourceRequirements { + return corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("50m"), + corev1.ResourceMemory: resource.MustParse("64Mi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("200m"), + corev1.ResourceMemory: resource.MustParse("256Mi"), + }, + } +} + +// MergeResourceRequirements merges user-provided resource requirements with defaults. +// User-provided values take precedence over defaults. +// If a resource is specified in userResources, it will override the corresponding default. +func MergeResourceRequirements(defaultResources, userResources corev1.ResourceRequirements) corev1.ResourceRequirements { + result := corev1.ResourceRequirements{} + + // Start with defaults + if defaultResources.Requests != nil { + result.Requests = make(corev1.ResourceList) + for k, v := range defaultResources.Requests { + result.Requests[k] = v + } + } + if defaultResources.Limits != nil { + result.Limits = make(corev1.ResourceList) + for k, v := range defaultResources.Limits { + result.Limits[k] = v + } + } + + // Override with user-provided values + if userResources.Requests != nil { + if result.Requests == nil { + result.Requests = make(corev1.ResourceList) + } + for k, v := range userResources.Requests { + result.Requests[k] = v + } + } + if userResources.Limits != nil { + if result.Limits == nil { + result.Limits = make(corev1.ResourceList) + } + for k, v := range userResources.Limits { + result.Limits[k] = v + } + } + + return result +} \ No newline at end of file