Skip to content

Conversation

@siddharth-s31
Copy link

Fixes #3131

This PR adds default resource limits to MCPRemoteProxy to prevent containers from consuming unlimited resources, aligning with the pattern established in VirtualMCPServer (#2873).

Changes

  • Added helper functions BuildDefaultProxyRunnerResourceRequirements and MergeResourceRequirements to controllerutil.
  • Updated mcpremoteproxy_deployment.go to apply default resources (50m/200m CPU, 64Mi/256Mi Memory) when creating deployments.
  • Updated mcpremoteproxy_controller.go to correctly detect updates by comparing against merged resource requirements.
  • Added unit tests to mcpremoteproxy_deployment_test.go to verify:
    • Default resources are applied when no user input is provided.
    • User overrides take precedence over defaults.

@github-actions github-actions bot added the size/L Large PR: 600-999 lines changed label Dec 22, 2025
Copy link
Contributor

@yrobla yrobla left a comment

Choose a reason for hiding this comment

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

thanks for the contribution! generally it looks good but i don't like the way the defaults are hardcoded. Instead we can have 2 different options. Use env vars and modify operator to accept them:

In deploy/charts/operator/values.yaml:

  resources:
    defaults:
      mcpRemoteProxy:
        requests:
          cpu: "50m"
          memory: "64Mi"
        limits:
          cpu: "200m"
          memory: "256Mi"

In deploy/charts/operator/templates/deployment.yaml:

env:
- name: DEFAULT_MCPREMOTEPROXY_CPU_REQUEST
  value: {{ .Values.resources.defaults.mcpRemoteProxy.requests.cpu | quote }}
- name: DEFAULT_MCPREMOTEPROXY_MEMORY_REQUEST
  value: {{ .Values.resources.defaults.mcpRemoteProxy.requests.memory | quote }}
- name: DEFAULT_MCPREMOTEPROXY_CPU_LIMIT
  value: {{ .Values.resources.defaults.mcpRemoteProxy.limits.cpu | quote }}
- name: DEFAULT_MCPREMOTEPROXY_MEMORY_LIMIT
  value: {{ .Values.resources.defaults.mcpRemoteProxy.limits.memory | quote }}

In controllerutil/resources.go:

func BuildDefaultProxyRunnerResourceRequirements() corev1.ResourceRequirements {
    return corev1.ResourceRequirements{
        Limits: corev1.ResourceList{
            corev1.ResourceCPU:    resource.MustParse(getEnvOrDefault("DEFAULT_MCPREMOTEPROXY_CPU_LIMIT", "200m")),
            corev1.ResourceMemory: resource.MustParse(getEnvOrDefault("DEFAULT_MCPREMOTEPROXY_MEMORY_LIMIT", "256Mi")),
        },
        Requests: corev1.ResourceList{
            corev1.ResourceCPU:    resource.MustParse(getEnvOrDefault("DEFAULT_MCPREMOTEPROXY_CPU_REQUEST", "50m")),
            corev1.ResourceMemory: resource.MustParse(getEnvOrDefault("DEFAULT_MCPREMOTEPROXY_MEMORY_REQUEST", "64Mi")),
        },
    }
}

func getEnvOrDefault(key, defaultValue string) string {
    if val := os.Getenv(key); val != "" {
        return val
    }
    return defaultValue
}

Or use a configmap:

Create a well-known ConfigMap:

  apiVersion: v1
  kind: ConfigMap
  metadata:
    name: toolhive-operator-defaults
    namespace: toolhive-system
  data:
    mcpremoteproxy-resources.yaml: |
      requests:
        cpu: "50m"
        memory: "64Mi"
      limits:
        cpu: "200m"
        memory: "256Mi"

In controller, read it once at startup:

func (r *MCPRemoteProxyReconciler) loadDefaults(ctx context.Context) {
    cm := &corev1.ConfigMap{}
    err := r.Get(ctx, types.NamespacedName{
        Name: "toolhive-operator-defaults",
        Namespace: "toolhive-system",
    }, cm)
    if err != nil {
        // Use hardcoded defaults if ConfigMap doesn't exist
        return
    }
    // Parse YAML and cache defaults
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Large PR: 600-999 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add default resource limits to MCPRemoteProxy

2 participants