-
Notifications
You must be signed in to change notification settings - Fork 119
feat: add k8s settings update notification in deployment status #5396
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?
Changes from 5 commits
2572a8a
3dd2afa
33e56ee
4326639
ae49a25
607b266
a5741f5
e8a54a3
6ce01ce
eb40233
1b0dafb
a50fe90
6024e21
fc357d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -156,6 +156,7 @@ func ConvertMCPServerCatalogEntryWithWorkspace(entry v1.MCPServerCatalogEntry, p | |
| PowerUserWorkspaceID: powerUserWorkspaceID, | ||
| PowerUserID: powerUserID, | ||
| NeedsUpdate: entry.Status.NeedsUpdate, | ||
| NeedsK8sUpdate: entry.Status.NeedsK8sUpdate, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -2463,6 +2464,7 @@ func ConvertMCPServer(server v1.MCPServer, credEnv map[string]string, serverURL, | |
| MCPCatalogID: server.Spec.MCPCatalogID, | ||
| ConnectURL: connectURL, | ||
| NeedsUpdate: server.Status.NeedsUpdate, | ||
| NeedsK8sUpdate: server.Status.NeedsK8sUpdate, | ||
| NeedsURL: server.Spec.NeedsURL, | ||
| PreviousURL: server.Spec.PreviousURL, | ||
| MCPServerInstanceUserCount: server.Status.MCPServerInstanceUserCount, | ||
|
|
@@ -3046,6 +3048,14 @@ func (m *MCPHandler) RedeployWithK8sSettings(req api.Context) error { | |
| return fmt.Errorf("failed to redeploy server: %w", err) | ||
| } | ||
|
|
||
| // Clear the NeedsK8sUpdate flag since user explicitly redeployed | ||
| if server.Status.NeedsK8sUpdate { | ||
| server.Status.NeedsK8sUpdate = false | ||
| if err := req.Storage.Status().Update(req.Context(), &server); err != nil { | ||
| return fmt.Errorf("failed to update server status: %w", err) | ||
| } | ||
|
||
| } | ||
|
|
||
| // Get credential for server | ||
| var credCtxs []string | ||
| if server.Spec.MCPCatalogID != "" { | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file should be deleted. The way our controller framework works, the two controllers you wrote (one for servers and one for catalog entries) will do the correct thing when the k8s settings change. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| package k8ssettings | ||
|
|
||
| import ( | ||
| "github.com/obot-platform/nah/pkg/router" | ||
| "github.com/obot-platform/obot/pkg/mcp" | ||
| v1 "github.com/obot-platform/obot/pkg/storage/apis/obot.obot.ai/v1" | ||
| "github.com/obot-platform/obot/pkg/system" | ||
| "k8s.io/apimachinery/pkg/fields" | ||
| kclient "sigs.k8s.io/controller-runtime/pkg/client" | ||
| ) | ||
|
|
||
| // UpdateAllServerK8sSettingsDrift updates the NeedsK8sUpdate status on all MCP servers when K8s settings change | ||
| func UpdateAllServerK8sSettingsDrift(req router.Request, _ router.Response) error { | ||
| k8sSettings := req.Object.(*v1.K8sSettings) | ||
|
|
||
| // Compute the new hash | ||
| currentHash := mcp.ComputeK8sSettingsHash(k8sSettings.Spec) | ||
|
|
||
| // List all MCP servers | ||
| var servers v1.MCPServerList | ||
| if err := req.List(&servers, &kclient.ListOptions{ | ||
| Namespace: req.Object.GetNamespace(), | ||
| }); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Update each server's NeedsK8sUpdate status | ||
| for i := range servers.Items { | ||
| server := &servers.Items[i] | ||
| // Skip servers without K8s settings hash (not deployed or non-K8s runtimes) | ||
| if server.Status.K8sSettingsHash == "" { | ||
| if server.Status.NeedsK8sUpdate { | ||
| server.Status.NeedsK8sUpdate = false | ||
| if err := req.Client.Status().Update(req.Ctx, server); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| // Check if the server needs update | ||
| needsK8sUpdate := server.Status.K8sSettingsHash != currentHash | ||
|
|
||
| if server.Status.NeedsK8sUpdate != needsK8sUpdate { | ||
| server.Status.NeedsK8sUpdate = needsK8sUpdate | ||
| if err := req.Client.Status().Update(req.Ctx, server); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // UpdateAllCatalogEntryK8sSettingsDrift updates the NeedsK8sUpdate status on all catalog entries when K8s settings change | ||
| func UpdateAllCatalogEntryK8sSettingsDrift(req router.Request, _ router.Response) error { | ||
| k8sSettings := req.Object.(*v1.K8sSettings) | ||
|
|
||
| // Compute the new hash | ||
| currentHash := mcp.ComputeK8sSettingsHash(k8sSettings.Spec) | ||
|
|
||
| // List all catalog entries | ||
| var entries v1.MCPServerCatalogEntryList | ||
| if err := req.List(&entries, &kclient.ListOptions{ | ||
| Namespace: req.Object.GetNamespace(), | ||
| }); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Update each catalog entry's NeedsK8sUpdate status | ||
| for i := range entries.Items { | ||
| entry := &entries.Items[i] | ||
| // List all servers created from this catalog entry | ||
| var mcpServers v1.MCPServerList | ||
| if err := req.List(&mcpServers, &kclient.ListOptions{ | ||
| FieldSelector: fields.OneTermEqualSelector("spec.mcpServerCatalogEntryName", entry.Name), | ||
| Namespace: system.DefaultNamespace, | ||
| }); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Check if any server has outdated K8s settings | ||
| var needsK8sUpdate bool | ||
| for _, server := range mcpServers.Items { | ||
| // Skip servers being deleted or without K8s settings hash | ||
| if !server.DeletionTimestamp.IsZero() || server.Status.K8sSettingsHash == "" { | ||
| continue | ||
| } | ||
|
|
||
| // Check if hash differs from current settings | ||
| if server.Status.K8sSettingsHash != currentHash { | ||
| needsK8sUpdate = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if entry.Status.NeedsK8sUpdate != needsK8sUpdate { | ||
| entry.Status.NeedsK8sUpdate = needsK8sUpdate | ||
| if err := req.Client.Status().Update(req.Ctx, entry); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ import ( | |
| "github.com/gptscript-ai/gptscript/pkg/hash" | ||
| "github.com/obot-platform/nah/pkg/router" | ||
| "github.com/obot-platform/obot/apiclient/types" | ||
| "github.com/obot-platform/obot/pkg/mcp" | ||
| v1 "github.com/obot-platform/obot/pkg/storage/apis/obot.obot.ai/v1" | ||
| "github.com/obot-platform/obot/pkg/system" | ||
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
|
|
@@ -135,6 +136,76 @@ func DetectCompositeDrift(req router.Request, _ router.Response) error { | |
| return nil | ||
| } | ||
|
|
||
| // DetectK8sSettingsDrift detects when servers created from this catalog entry need redeployment with new K8s settings | ||
| func DetectK8sSettingsDrift(req router.Request, _ router.Response) error { | ||
|
||
| entry := req.Object.(*v1.MCPServerCatalogEntry) | ||
|
|
||
| // Only check for containerized or uvx/npx runtimes that might run in K8s | ||
| if entry.Spec.Manifest.Runtime != types.RuntimeContainerized && | ||
| entry.Spec.Manifest.Runtime != types.RuntimeUVX && | ||
| entry.Spec.Manifest.Runtime != types.RuntimeNPX { | ||
| // Remote and composite servers don't have K8s deployments | ||
| if entry.Status.NeedsK8sUpdate { | ||
| entry.Status.NeedsK8sUpdate = false | ||
| return req.Client.Status().Update(req.Ctx, entry) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // Get current K8s settings | ||
| var k8sSettings v1.K8sSettings | ||
| if err := req.Get(&k8sSettings, req.Object.GetNamespace(), system.K8sSettingsName); err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| // K8s settings not found, mark as not needing update | ||
| if entry.Status.NeedsK8sUpdate { | ||
| entry.Status.NeedsK8sUpdate = false | ||
| return req.Client.Status().Update(req.Ctx, entry) | ||
| } | ||
| return nil | ||
| } | ||
| return fmt.Errorf("failed to get K8s settings: %w", err) | ||
| } | ||
|
|
||
| // Compute current K8s settings hash | ||
| currentHash := mcp.ComputeK8sSettingsHash(k8sSettings.Spec) | ||
|
|
||
| // List all servers created from this catalog entry | ||
| var mcpServers v1.MCPServerList | ||
| if err := req.List(&mcpServers, &kclient.ListOptions{ | ||
| FieldSelector: fields.OneTermEqualSelector("spec.mcpServerCatalogEntryName", entry.Name), | ||
| Namespace: req.Object.GetNamespace(), | ||
| }); err != nil { | ||
| return fmt.Errorf("failed to list MCP servers: %w", err) | ||
| } | ||
|
|
||
| // Check if any server has outdated K8s settings | ||
| var needsK8sUpdate bool | ||
| for _, server := range mcpServers.Items { | ||
| // Skip servers being deleted | ||
| if !server.DeletionTimestamp.IsZero() { | ||
| continue | ||
| } | ||
|
|
||
| // Skip servers without K8s settings hash (non-K8s runtimes or not yet deployed) | ||
| if server.Status.K8sSettingsHash == "" { | ||
| continue | ||
| } | ||
|
|
||
| // Check if hash differs from current settings | ||
| if server.Status.K8sSettingsHash != currentHash { | ||
| needsK8sUpdate = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if entry.Status.NeedsK8sUpdate != needsK8sUpdate { | ||
| entry.Status.NeedsK8sUpdate = needsK8sUpdate | ||
| return req.Client.Status().Update(req.Ctx, entry) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // CleanupNestedCompositeServers removes component servers with composite runtimes from composite catalog entries. | ||
| // This handler cleans up entries that were created before API validation to prevent nested composite servers. | ||
| func CleanupNestedCompositeEntries(req router.Request, _ router.Response) error { | ||
|
|
||
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.
I believe this can also be removed.
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.
If I delete this I do get the following error in
mcp.goline159There 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.
We don't need to track this for catalog entries. There are no deployments for catalog entries, so this field and all of its references should be removed.