From 0bf3842b210912c2107dafb898d9f578d23435db Mon Sep 17 00:00:00 2001 From: Jeremy Drouillard Date: Mon, 26 Jan 2026 12:52:06 -0800 Subject: [PATCH 1/2] fix(#3448) - vMCP operator deterministically orders servers Signed-off-by: Jeremy Drouillard --- pkg/vmcp/aggregator/aggregator.go | 1 + pkg/vmcp/aggregator/discoverer.go | 17 ++- pkg/vmcp/aggregator/discoverer_test.go | 150 +++++++++++++++++++++++-- 3 files changed, 158 insertions(+), 10 deletions(-) diff --git a/pkg/vmcp/aggregator/aggregator.go b/pkg/vmcp/aggregator/aggregator.go index d86526d1d6..7383ec9867 100644 --- a/pkg/vmcp/aggregator/aggregator.go +++ b/pkg/vmcp/aggregator/aggregator.go @@ -21,6 +21,7 @@ import ( type BackendDiscoverer interface { // Discover finds all backend workloads in the specified group. // Returns only healthy/running backends. + // Results are always sorted alphabetically by backend name to ensure deterministic ordering. // The groupRef format is platform-specific (group name for CLI, MCPGroup name for K8s). Discover(ctx context.Context, groupRef string) ([]vmcp.Backend, error) } diff --git a/pkg/vmcp/aggregator/discoverer.go b/pkg/vmcp/aggregator/discoverer.go index f8d7afa491..8421811561 100644 --- a/pkg/vmcp/aggregator/discoverer.go +++ b/pkg/vmcp/aggregator/discoverer.go @@ -14,6 +14,7 @@ package aggregator import ( "context" "fmt" + "sort" rt "github.com/stacklok/toolhive/pkg/container/runtime" "github.com/stacklok/toolhive/pkg/groups" @@ -120,7 +121,20 @@ func NewBackendDiscovererWithManager( // // In static mode (when staticBackends are configured), this returns pre-configured backends // without any K8s API access. In dynamic mode, it discovers backends at runtime. -func (d *backendDiscoverer) Discover(ctx context.Context, groupRef string) ([]vmcp.Backend, error) { +// +// Results are always sorted alphabetically by backend name to ensure deterministic ordering. +// This prevents non-deterministic ConfigMap content that would cause unnecessary +// deployment rollouts (pod cycling). See: https://github.com/stacklok/toolhive/issues/3448 +func (d *backendDiscoverer) Discover(ctx context.Context, groupRef string) (backends []vmcp.Backend, err error) { + // Sort backends by name before returning to ensure deterministic ordering + defer func() { + if len(backends) > 1 { + sort.Slice(backends, func(i, j int) bool { + return backends[i].Name < backends[j].Name + }) + } + }() + logger.Infof("Discovering backends in group %s", groupRef) // Static mode: Use pre-configured backends if available @@ -163,7 +177,6 @@ func (d *backendDiscoverer) Discover(ctx context.Context, groupRef string) ([]vm logger.Debugf("Found %d workloads in group %s, discovering backends", len(typedWorkloads), groupRef) // Query each workload and convert to backend - var backends []vmcp.Backend for _, workload := range typedWorkloads { backend, err := d.workloadsManager.GetWorkloadAsVMCPBackend(ctx, workload) if err != nil { diff --git a/pkg/vmcp/aggregator/discoverer_test.go b/pkg/vmcp/aggregator/discoverer_test.go index 876e51aba4..af324d76fc 100644 --- a/pkg/vmcp/aggregator/discoverer_test.go +++ b/pkg/vmcp/aggregator/discoverer_test.go @@ -552,15 +552,15 @@ func TestBackendDiscoverer_Discover(t *testing.T) { require.NoError(t, err) require.Len(t, backends, 2) - // Verify MCPServer backend - assert.Equal(t, "server1", backends[0].ID) - assert.Equal(t, "streamable-http", backends[0].TransportType) - assert.Equal(t, "github", backends[0].Metadata["tool_type"]) + // Backends are sorted alphabetically by name + // proxy1 comes before server1 alphabetically + assert.Equal(t, "proxy1", backends[0].ID) + assert.Equal(t, "sse", backends[0].TransportType) + assert.Equal(t, "mcp", backends[0].Metadata["tool_type"]) - // Verify MCPRemoteProxy backend - assert.Equal(t, "proxy1", backends[1].ID) - assert.Equal(t, "sse", backends[1].TransportType) - assert.Equal(t, "mcp", backends[1].Metadata["tool_type"]) + assert.Equal(t, "server1", backends[1].ID) + assert.Equal(t, "streamable-http", backends[1].TransportType) + assert.Equal(t, "github", backends[1].Metadata["tool_type"]) }) t.Run("applies authentication to MCPRemoteProxy backends", func(t *testing.T) { @@ -1308,3 +1308,137 @@ func TestStaticBackendDiscoverer_MetadataGroupOverride(t *testing.T) { }) } } + +// TestBackendDiscoverer_Discover_DeterministicOrdering tests that Discover returns backends +// in a deterministic order (sorted alphabetically by name) regardless of input order. +// This prevents non-deterministic ConfigMap content that would cause unnecessary +// deployment rollouts (pod cycling). See: https://github.com/stacklok/toolhive/issues/3448 +func TestBackendDiscoverer_Discover_DeterministicOrdering(t *testing.T) { + t.Parallel() + + // Test with multiple different input orders to ensure output is always sorted + testCases := []struct { + name string + staticBackends []config.StaticBackendConfig + }{ + { + name: "reverse alphabetical order (zebra, middle, alpha)", + staticBackends: []config.StaticBackendConfig{ + {Name: "zebra-backend", URL: "http://zebra:8080", Transport: "sse"}, + {Name: "middle-backend", URL: "http://middle:8080", Transport: "streamable-http"}, + {Name: "alpha-backend", URL: "http://alpha:8080", Transport: "sse"}, + }, + }, + { + name: "alphabetical order (alpha, middle, zebra)", + staticBackends: []config.StaticBackendConfig{ + {Name: "alpha-backend", URL: "http://alpha:8080", Transport: "sse"}, + {Name: "middle-backend", URL: "http://middle:8080", Transport: "streamable-http"}, + {Name: "zebra-backend", URL: "http://zebra:8080", Transport: "sse"}, + }, + }, + { + name: "random order (middle, zebra, alpha)", + staticBackends: []config.StaticBackendConfig{ + {Name: "middle-backend", URL: "http://middle:8080", Transport: "streamable-http"}, + {Name: "zebra-backend", URL: "http://zebra:8080", Transport: "sse"}, + {Name: "alpha-backend", URL: "http://alpha:8080", Transport: "sse"}, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + + discoverer := NewUnifiedBackendDiscovererWithStaticBackends( + tc.staticBackends, + nil, // No auth config needed for this test + "test-group", + ) + + backends, err := discoverer.Discover(ctx, "test-group") + require.NoError(t, err) + + // Output should ALWAYS be alphabetically sorted regardless of input order + require.Len(t, backends, 3, "should include all valid backends") + assert.Equal(t, "alpha-backend", backends[0].Name, + "first backend should be alpha-backend (alphabetically first)") + assert.Equal(t, "middle-backend", backends[1].Name, + "second backend should be middle-backend (alphabetically second)") + assert.Equal(t, "zebra-backend", backends[2].Name, + "third backend should be zebra-backend (alphabetically third)") + }) + } +} + +// TestBackendDiscoverer_Discover_DeterministicOrdering_DynamicMode tests that Discover +// returns backends in deterministic order when using dynamic mode (K8s API discovery). +func TestBackendDiscoverer_Discover_DeterministicOrdering_DynamicMode(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockWorkloadDiscoverer := discoverermocks.NewMockDiscoverer(ctrl) + mockGroups := mocks.NewMockManager(ctrl) + + // Create backends in non-alphabetical order to test sorting + backend1 := &vmcp.Backend{ + ID: "zebra-backend", + Name: "zebra-backend", + BaseURL: "http://zebra:8080/mcp", + TransportType: "sse", + HealthStatus: vmcp.BackendHealthy, + } + backend2 := &vmcp.Backend{ + ID: "alpha-backend", + Name: "alpha-backend", + BaseURL: "http://alpha:8080/mcp", + TransportType: "streamable-http", + HealthStatus: vmcp.BackendHealthy, + } + backend3 := &vmcp.Backend{ + ID: "middle-backend", + Name: "middle-backend", + BaseURL: "http://middle:8080/mcp", + TransportType: "sse", + HealthStatus: vmcp.BackendHealthy, + } + + mockGroups.EXPECT().Exists(gomock.Any(), testGroupName).Return(true, nil) + // Return workloads in non-alphabetical order (zebra, alpha, middle) + mockWorkloadDiscoverer.EXPECT().ListWorkloadsInGroup(gomock.Any(), testGroupName). + Return([]workloads.TypedWorkload{ + {Name: "zebra-backend", Type: workloads.WorkloadTypeMCPServer}, + {Name: "alpha-backend", Type: workloads.WorkloadTypeMCPServer}, + {Name: "middle-backend", Type: workloads.WorkloadTypeMCPServer}, + }, nil) + mockWorkloadDiscoverer.EXPECT().GetWorkloadAsVMCPBackend( + gomock.Any(), + workloads.TypedWorkload{Name: "zebra-backend", Type: workloads.WorkloadTypeMCPServer}, + ).Return(backend1, nil) + mockWorkloadDiscoverer.EXPECT().GetWorkloadAsVMCPBackend( + gomock.Any(), + workloads.TypedWorkload{Name: "alpha-backend", Type: workloads.WorkloadTypeMCPServer}, + ).Return(backend2, nil) + mockWorkloadDiscoverer.EXPECT().GetWorkloadAsVMCPBackend( + gomock.Any(), + workloads.TypedWorkload{Name: "middle-backend", Type: workloads.WorkloadTypeMCPServer}, + ).Return(backend3, nil) + + discoverer := NewUnifiedBackendDiscoverer(mockWorkloadDiscoverer, mockGroups, nil) + backends, err := discoverer.Discover(context.Background(), testGroupName) + + require.NoError(t, err) + require.Len(t, backends, 3) + + // Backends should be sorted alphabetically by name + assert.Equal(t, "alpha-backend", backends[0].Name, + "first backend should be alpha-backend (alphabetically first)") + assert.Equal(t, "middle-backend", backends[1].Name, + "second backend should be middle-backend (alphabetically second)") + assert.Equal(t, "zebra-backend", backends[2].Name, + "third backend should be zebra-backend (alphabetically third)") +} From f23b26b1b5089eb8de10aabb6cde41bcf60dbf42 Mon Sep 17 00:00:00 2001 From: Jeremy Drouillard Date: Mon, 26 Jan 2026 13:13:30 -0800 Subject: [PATCH 2/2] fix lint Signed-off-by: Jeremy Drouillard --- pkg/vmcp/aggregator/discoverer.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/vmcp/aggregator/discoverer.go b/pkg/vmcp/aggregator/discoverer.go index 8421811561..985d5f3a0d 100644 --- a/pkg/vmcp/aggregator/discoverer.go +++ b/pkg/vmcp/aggregator/discoverer.go @@ -140,7 +140,7 @@ func (d *backendDiscoverer) Discover(ctx context.Context, groupRef string) (back // Static mode: Use pre-configured backends if available if len(d.staticBackends) > 0 { logger.Infof("Using %d pre-configured static backends (no K8s API access)", len(d.staticBackends)) - return d.discoverFromStaticConfig() + return d.discoverFromStaticConfig(), nil } // If staticBackends was explicitly set (even if empty), but groupsManager is nil, @@ -260,7 +260,7 @@ func (d *backendDiscoverer) applyAuthConfigToBackend(backend *vmcp.Backend, back // discoverFromStaticConfig converts pre-configured static backends into vmcp.Backend objects // for use in static mode where no K8s API access is available. -func (d *backendDiscoverer) discoverFromStaticConfig() ([]vmcp.Backend, error) { +func (d *backendDiscoverer) discoverFromStaticConfig() []vmcp.Backend { backends := make([]vmcp.Backend, 0, len(d.staticBackends)) for _, staticBackend := range d.staticBackends { @@ -292,5 +292,5 @@ func (d *backendDiscoverer) discoverFromStaticConfig() ([]vmcp.Backend, error) { staticBackend.Name, staticBackend.URL, staticBackend.Transport) } - return backends, nil + return backends }