-
Notifications
You must be signed in to change notification settings - Fork 160
Add default resource limits and separate proxy runner resource config #3067
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3067 +/- ##
==========================================
+ Coverage 56.82% 57.12% +0.29%
==========================================
Files 335 335
Lines 33474 33548 +74
==========================================
+ Hits 19022 19164 +142
+ Misses 12868 12793 -75
- Partials 1584 1591 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
8283172 to
2820b32
Compare
5b123c3 to
a586144
Compare
a586144 to
9932871
Compare
9932871 to
f355ff6
Compare
…uration Implements default resource limits for MCP server and VirtualMCP server containers to prevent resource monopolization, with intelligent merging of user-provided values and separate resource configuration for proxy runner containers. Per issue stacklok#2873, vmcp and MCP server containers lacked default resource limits, creating security and operational risks where misbehaving containers could exhaust node resources and affect other workloads. Additionally, MCPServer deployments have two containers (MCP server + proxy runner) but only had one resource field (`spec.resources`), creating ambiguity about which container the resources applied to. - MCP server container: 500m/100m CPU, 512Mi/128Mi memory - Proxy runner container: 200m/50m CPU, 256Mi/64Mi memory (lighter weight) - VirtualMCP server container: 500m/100m CPU, 512Mi/128Mi memory - Defaults are intelligently merged with user-provided values - Add `spec.proxyRunnerResources` field for proxy runner container configuration - Existing `spec.resources` applies to MCP server container - Clear separation eliminates ambiguity between the two containers MCP server resources are composed from three sources: - Defaults → `spec.resources` → `spec.podTemplateSpec` (highest precedence) Proxy runner resources are composed from: - Defaults → `spec.proxyRunnerResources` - Add `resourceRequirementsForMCPServer()`: Merges defaults with spec.resources - Add `resourceRequirementsForProxyRunner()`: Merges defaults with spec.proxyRunnerResources - Update `deploymentNeedsUpdate()`: Checks both MCP and proxy resources separately - Add `PodTemplateSpecBuilder.WithResources()`: Merges resources intelligently - Add shared utility functions in `pkg/controllerutil/resources.go` - Add regression tests for `deploymentNeedsUpdate()` resource changes (4 tests) - Add tests for resource merging in PodTemplateSpecBuilder (2 tests) - Add tests for default resources and merging (3 tests) - Add VirtualMCPServer deployment tests (10 tests) - Total: 19 new tests ensuring no reconciliation loops - Add `proxyRunnerResources` field to MCPServer CRD - Regenerate deepcopy methods and CRD manifests - Bump Helm chart version: 0.0.75 → 0.0.76 - Update example YAML with clear comments - Document default values and composition precedence **MCPServer (2 containers):** - `spec.resources` → MCP server container (via pod template patch) - `spec.proxyRunnerResources` → Proxy runner container (in deployment) **VirtualMCPServer (1 container):** - `spec.resources` → VirtualMCP server container - Prevents resource exhaustion by providing sensible defaults - Clear separation: separate fields for separate containers in MCPServer - User can customize each container independently - Intelligent merging allows partial overrides - Prevents reconciliation loops through proper update detection Fixes stacklok#2873 Signed-off-by: 4t8dd <wanger.xyz@gmail.com>
f355ff6 to
48387c9
Compare
Add default resource limits and separate proxy runner resource configuration
Implements default resource limits for MCP server and VirtualMCP server containers to prevent resource monopolization, with intelligent merging of user-provided values and separate resource configuration for proxy runner containers.
Per issue #2873, vmcp and MCP server containers lacked default resource limits, creating security and operational risks where misbehaving containers could exhaust node resources and affect other workloads.
Additionally, MCPServer deployments have two containers (MCP server + proxy runner) but only had one resource field (
spec.resources), creating ambiguity about which container the resources applied to.MCP server container: 500m/100m CPU, 512Mi/128Mi memory
Proxy runner container: 200m/50m CPU, 256Mi/64Mi memory (lighter weight)
VirtualMCP server container: 500m/100m CPU, 512Mi/128Mi memory
Defaults are intelligently merged with user-provided values
Add
spec.proxyRunnerResourcesfield for proxy runner container configurationExisting
spec.resourcesapplies to MCP server containerClear separation eliminates ambiguity between the two containers
MCP server resources are composed from three sources:
spec.resources→spec.podTemplateSpec(highest precedence)Proxy runner resources are composed from:
Defaults →
spec.proxyRunnerResourcesAdd
resourceRequirementsForMCPServer(): Merges defaults with spec.resourcesAdd
resourceRequirementsForProxyRunner(): Merges defaults with spec.proxyRunnerResourcesUpdate
deploymentNeedsUpdate(): Checks both MCP and proxy resources separatelyAdd
PodTemplateSpecBuilder.WithResources(): Merges resources intelligentlyAdd shared utility functions in
pkg/controllerutil/resources.goAdd regression tests for
deploymentNeedsUpdate()resource changes (4 tests)Add tests for resource merging in PodTemplateSpecBuilder (2 tests)
Add tests for default resources and merging (3 tests)
Add VirtualMCPServer deployment tests (10 tests)
Total: 19 new tests ensuring no reconciliation loops
Add
proxyRunnerResourcesfield to MCPServer CRDRegenerate deepcopy methods and CRD manifests
Bump Helm chart version: 0.0.75 → 0.0.76
Update example YAML with clear comments
Document default values and composition precedence
MCPServer (2 containers):
spec.resources→ MCP server container (via pod template patch)spec.proxyRunnerResources→ Proxy runner container (in deployment)VirtualMCPServer (1 container):
spec.resources→ VirtualMCP server containerPrevents resource exhaustion by providing sensible defaults
Clear separation: separate fields for separate containers in MCPServer
User can customize each container independently
Intelligent merging allows partial overrides
Prevents reconciliation loops through proper update detection
Fixes #2873