Skip to content

Commit 0a6ec35

Browse files
allisoneerallisoneer
committed
make better
1 parent 2b43e06 commit 0a6ec35

File tree

2 files changed

+108
-47
lines changed

2 files changed

+108
-47
lines changed

‎acp/internal/controller/mcpserver/mcpserver_controller.go‎

Lines changed: 29 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -47,29 +47,41 @@ type MCPServerReconciler struct {
4747
}
4848

4949
// updateStatus updates the status of the MCPServer resource with the latest version
50+
// This method handles conflicts by retrying the status update up to 3 times
5051
func (r *MCPServerReconciler) updateStatus(ctx context.Context, req ctrl.Request, statusUpdate *acp.MCPServer) error {
5152
logger := log.FromContext(ctx)
53+
const maxRetries = 3
54+
55+
var updateErr error
56+
for i := 0; i < maxRetries; i++ {
57+
// Get the latest version of the MCPServer
58+
var latestMCPServer acp.MCPServer
59+
if err := r.Get(ctx, req.NamespacedName, &latestMCPServer); err != nil {
60+
logger.Error(err, "Failed to get latest MCPServer before status update")
61+
return err
62+
}
5263

53-
// Get the latest version of the MCPServer
54-
var latestMCPServer acp.MCPServer
55-
if err := r.Get(ctx, req.NamespacedName, &latestMCPServer); err != nil {
56-
logger.Error(err, "Failed to get latest MCPServer before status update")
57-
return err
58-
}
59-
60-
// Apply status updates to the latest version
61-
latestMCPServer.Status.Connected = statusUpdate.Status.Connected
62-
latestMCPServer.Status.Status = statusUpdate.Status.Status
63-
latestMCPServer.Status.StatusDetail = statusUpdate.Status.StatusDetail
64-
latestMCPServer.Status.Tools = statusUpdate.Status.Tools
64+
// Apply status updates to the latest version
65+
latestMCPServer.Status.Connected = statusUpdate.Status.Connected
66+
latestMCPServer.Status.Status = statusUpdate.Status.Status
67+
latestMCPServer.Status.StatusDetail = statusUpdate.Status.StatusDetail
68+
latestMCPServer.Status.Tools = statusUpdate.Status.Tools
69+
70+
// Update the status
71+
updateErr = r.Status().Update(ctx, &latestMCPServer)
72+
if updateErr == nil {
73+
// Success - no need for more retries
74+
return nil
75+
}
6576

66-
// Update the status
67-
if err := r.Status().Update(ctx, &latestMCPServer); err != nil {
68-
logger.Error(err, "Failed to update MCPServer status")
69-
return err
77+
// If conflict, wait briefly and retry
78+
logger.Info("Status update conflict, retrying", "attempt", i+1, "error", updateErr)
79+
time.Sleep(time.Millisecond * 100)
7080
}
7181

72-
return nil
82+
// If we got here, we failed all retries
83+
logger.Error(updateErr, "Failed to update MCPServer status after retries")
84+
return updateErr
7385
}
7486

7587
// Reconcile processes the MCPServer resource and establishes a connection to the MCP server

‎acp/internal/controller/mcpserver/mcpserver_controller_test.go‎

Lines changed: 79 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -78,69 +78,118 @@ var _ = Describe("MCPServer Controller", func() {
7878
It("Should validate and connect to the MCP server", func() {
7979
ctx := context.Background()
8080

81-
// Use a unique name for the MCPServer
82-
testName := "test-mcpserver-fix"
81+
// Create a test with a command that exists to avoid the command-not-found error
82+
testName := "test-mcpserver-echo"
8383

84-
By("Creating a new MCPServer")
84+
By("Creating a new MCPServer with a valid command")
8585
mcpServer := &acp.MCPServer{
8686
ObjectMeta: metav1.ObjectMeta{
8787
Name: testName,
8888
Namespace: MCPServerNamespace,
89+
// Add labels to identify this as our test server
90+
Labels: map[string]string{
91+
"test": "true",
92+
},
8993
},
9094
Spec: acp.MCPServerSpec{
9195
Transport: "stdio",
92-
Command: "echo", // Use a command that exists to avoid system errors
93-
Args: []string{"--arg1", "value1"},
94-
Env: []acp.EnvVar{
95-
{
96-
Name: "TEST_ENV",
97-
Value: "test-value",
98-
},
99-
},
96+
Command: "sh", // This command exists
97+
Args: []string{"-c", "echo test"},
10098
},
10199
}
102100

103101
Expect(k8sClient.Create(ctx, mcpServer)).To(Succeed())
104102
defer teardownMCPServer(ctx, mcpServer)
105103

106104
lookupKey := types.NamespacedName{Name: testName, Namespace: MCPServerNamespace}
107-
createdServer := &acp.MCPServer{}
108105

109-
By("Verifying the MCPServer was created")
110-
Eventually(func() bool {
111-
err := k8sClient.Get(ctx, lookupKey, createdServer)
112-
return err == nil
113-
}, time.Second*10, time.Millisecond*250).Should(BeTrue())
106+
By("Setting up a mock MCPServer manager")
107+
mockManager := &MockMCPServerManager{
108+
ConnectServerFunc: func(ctx context.Context, mcpServer *acp.MCPServer) error {
109+
return nil // Return success
110+
},
111+
GetToolsFunc: func(serverName string) ([]acp.MCPTool, bool) {
112+
return []acp.MCPTool{
113+
{
114+
Name: "test-tool",
115+
Description: "A test tool for validation",
116+
},
117+
}, true
118+
},
119+
}
114120

115-
// Wait for the background controller to process
116-
time.Sleep(time.Second * 2)
121+
By("Creating a new test reconciler with our mock manager")
122+
reconciler := &MCPServerReconciler{
123+
Client: k8sClient,
124+
Scheme: k8sClient.Scheme(),
125+
recorder: record.NewFakeRecorder(10),
126+
MCPManager: mockManager,
127+
}
117128

118-
// Skip creating a mock manager since we're directly updating the status
129+
By("Performing reconciliation with our mock manager")
130+
result, err := reconciler.Reconcile(ctx, ctrl.Request{
131+
NamespacedName: lookupKey,
132+
})
119133

120-
By("Manually updating the status to verify the test")
121-
err := k8sClient.Get(ctx, lookupKey, createdServer)
134+
// This should succeed since our mock returns success
122135
Expect(err).NotTo(HaveOccurred())
136+
Expect(result.RequeueAfter).To(Equal(time.Minute * 10)) // Successful requeue after 10 minutes
137+
138+
// Create a validation test that uses the simpler approach - directly updating status
139+
testName2 := "test-mcpserver-direct"
123140

141+
By("Creating a second MCPServer to test direct status updates")
142+
mcpServer2 := &acp.MCPServer{
143+
ObjectMeta: metav1.ObjectMeta{
144+
Name: testName2,
145+
Namespace: MCPServerNamespace,
146+
},
147+
Spec: acp.MCPServerSpec{
148+
Transport: "stdio",
149+
Command: "sh",
150+
Args: []string{"-c", "echo test"},
151+
},
152+
}
153+
154+
Expect(k8sClient.Create(ctx, mcpServer2)).To(Succeed())
155+
defer teardownMCPServer(ctx, mcpServer2)
156+
157+
lookupKey2 := types.NamespacedName{Name: testName2, Namespace: MCPServerNamespace}
158+
159+
// Wait for it to be created
160+
createdServer := &acp.MCPServer{}
161+
Eventually(func() bool {
162+
err := k8sClient.Get(ctx, lookupKey2, createdServer)
163+
return err == nil
164+
}, time.Second*10, time.Millisecond*250).Should(BeTrue())
165+
166+
By("Directly updating the status")
124167
createdServer.Status.Connected = true
125168
createdServer.Status.Status = "Ready"
169+
createdServer.Status.StatusDetail = "Manually set status"
126170
createdServer.Status.Tools = []acp.MCPTool{
127171
{
128-
Name: "test-tool",
129-
Description: "A test tool",
172+
Name: "manual-tool",
173+
Description: "A tool for testing",
130174
},
131175
}
132176

133177
err = k8sClient.Status().Update(ctx, createdServer)
134178
Expect(err).NotTo(HaveOccurred())
135179

136-
By("Checking that the status was updated correctly")
180+
// Verify the direct update worked
181+
By("Verifying the status was properly updated")
137182
updatedServer := &acp.MCPServer{}
138-
err = k8sClient.Get(ctx, lookupKey, updatedServer)
139-
Expect(err).NotTo(HaveOccurred())
183+
Eventually(func() bool {
184+
if err := k8sClient.Get(ctx, lookupKey2, updatedServer); err != nil {
185+
return false
186+
}
187+
return updatedServer.Status.Connected &&
188+
updatedServer.Status.Status == "Ready" &&
189+
len(updatedServer.Status.Tools) == 1
190+
}, time.Second*10, time.Millisecond*250).Should(BeTrue())
140191

141-
Expect(updatedServer.Status.Connected).To(BeTrue())
142-
Expect(updatedServer.Status.Status).To(Equal("Ready"))
143-
Expect(updatedServer.Status.Tools).To(HaveLen(1))
192+
Expect(updatedServer.Status.Tools[0].Name).To(Equal("manual-tool"))
144193
})
145194

146195
It("Should handle invalid MCP server specs", func() {

0 commit comments

Comments
 (0)