feat: apply K8sSettings resources and enable sync for SystemMCPServer#6038
feat: apply K8sSettings resources and enable sync for SystemMCPServer#6038wklee610 wants to merge 9 commits intoobot-platform:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds K8s-settings drift handling and resource defaults to improve Kubernetes deployments for MCP servers, including SystemMCPServer.
Changes:
- Add
NeedsK8sUpdatestatus flag toSystemMCPServerStatusand trigger redeploy logic in the system server controller. - Apply
K8sSettings.Resources(or defaults) to additional Kubernetes containers during object creation. - Extend the deployment-status copier to handle
SystemMCPServerobjects in addition toMCPServer.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| pkg/storage/apis/obot.obot.ai/v1/systemmcpserver.go | Adds NeedsK8sUpdate to SystemMCPServer status. |
| pkg/mcp/kubernetes.go | Sets container Resources from K8s settings (with defaults) for additional containers. |
| pkg/controller/handlers/systemmcpserver/systemmcpserver.go | Shuts down system server deployments when NeedsK8sUpdate is set to force redeploy. |
| pkg/controller/handlers/deployment/deployment.go | Updates deployment-to-status syncing to support SystemMCPServer and propagate K8s drift status. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| systemServer.Status.NeedsK8sUpdate = false | ||
| return req.Client.Status().Update(req.Ctx, systemServer) |
There was a problem hiding this comment.
Clearing NeedsK8sUpdate without also updating Status.K8sSettingsHash to the current expected hash can cause the deployment watcher to immediately set NeedsK8sUpdate back to true while the old Deployment (with the old hash annotation) still exists, leading to repeated shutdown/redeploy cycles. Consider fetching current K8sSettings here, computing the current hash, and updating Status.K8sSettingsHash alongside clearing the flag (similar to the MCPServer redeploy path in pkg/api/handlers/mcp.go).
| systemServer.Status.NeedsK8sUpdate = false | |
| return req.Client.Status().Update(req.Ctx, systemServer) | |
| // Leave NeedsK8sUpdate and any associated hash fields unchanged here. | |
| // The component responsible for reconciling the K8s deployment should | |
| // update the hash and clear the flag together once the deployment is in sync. | |
| return nil |
pkg/mcp/kubernetes.go
Outdated
| Resources: func() corev1.ResourceRequirements { | ||
| if k8sSettings.Resources != nil { | ||
| return *k8sSettings.Resources | ||
| } | ||
| return corev1.ResourceRequirements{ | ||
| Requests: corev1.ResourceList{ | ||
| corev1.ResourceMemory: resource.MustParse("400Mi"), | ||
| }, | ||
| } | ||
| }(), |
There was a problem hiding this comment.
Now that resources can be set on this non-"mcp" container at creation time, note that RestartServerDeployment/patchDeploymentWithK8sSettings currently only patches resources for the "mcp" container. That means a restart used to apply new K8sSettings.Resources won’t update resources for this container, and the hash may still be updated after verification. Consider extending the patch + match logic to cover resources for all relevant containers, or ensure redeploy uses delete/recreate when resources drift.
| Resources: func() corev1.ResourceRequirements { | |
| if k8sSettings.Resources != nil { | |
| return *k8sSettings.Resources | |
| } | |
| return corev1.ResourceRequirements{ | |
| Requests: corev1.ResourceList{ | |
| corev1.ResourceMemory: resource.MustParse("400Mi"), | |
| }, | |
| } | |
| }(), | |
| Resources: corev1.ResourceRequirements{ | |
| Requests: corev1.ResourceList{ | |
| corev1.ResourceMemory: resource.MustParse("400Mi"), | |
| }, | |
| }, |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
This PR ensures that SystemMCPServer pods respect Kubernetes resource configurations and automatically redeploy when those settings change.
Key Changes
Apply resources to all containers
mcp-shim and other related containers now correctly inherit requests/limits from K8sSettings.Resources.
Track Kubernetes settings changes
Added NeedsK8sUpdate and K8sSettingsHash to SystemMCPServerStatus to detect configuration updates.
Watch SystemMCPServer deployments
Updated deployment handlers to monitor SystemMCPServer (sms*) objects, not just MCPServer.
Automatic redeploy on config change
During reconciliation, if NeedsK8sUpdate is true, the controller gracefully shuts down the existing pod and redeploys it with the updated settings.