Skip to content

Make embedding server optional for vMCP optimizer #6106

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
meenacodes wants to merge 3 commits into
base: main
Choose a base branch
from
+8 −15
Open

Make embedding server optional for vMCP optimizer #6106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7e57142
Update virtualmcpserver_types.go
meenacodes Jul 28, 2026
a2662cd
Update virtualmcpserver_types_test.go
meenacodes Jul 28, 2026
f766d6c
Merge branch 'main' into main
meenacodes Jul 28, 2026
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
18 changes: 6 additions & 12 deletions cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,10 @@
TelemetryConfigRef *MCPTelemetryConfigReference `json:"telemetryConfigRef,omitempty"`

// EmbeddingServerRef references an existing EmbeddingServer resource by name.
// When the optimizer is enabled, this field is required to point to a ready EmbeddingServer
// that provides embedding capabilities.
// It is optional even when the optimizer is enabled: without it (and without
// spec.config.optimizer.embeddingService), find_tool falls back to FTS5
// keyword-only search with no semantic ranking. Set this field to point to a
// ready EmbeddingServer when semantic ranking is desired.
// The referenced EmbeddingServer must exist in the same namespace and be ready.
// +optional
EmbeddingServerRef *EmbeddingServerRef `json:"embeddingServerRef,omitempty"`
Expand Down Expand Up @@ -598,8 +600,9 @@

// validateEmbeddingServer validates EmbeddingServerRef and Optimizer configuration.
// Rules:
// - embeddingServerRef.name must be non-empty when ref is provided

Check failure on line 603 in cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go

View workflow job for this annotation

GitHub Actions / Linting / Lint Go Code 

File is not properly formatted (gci)
// - optimizer requires either embeddingServerRef or a manually set embeddingService
// - optimizer does not require an embedding source: with neither embeddingServerRef
// nor optimizer.embeddingService set, find_tool runs FTS5 keyword-only search

@aponcedeleonch aponcedeleonch Jul 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gofmt rejects this. Go 1.19+ gofmt reformats doc comments, and a column-0 // - bullet with an indented continuation line isn't recognized as a list, so it gets rewritten to the canonical // - form:

//   - embeddingServerRef.name must be non-empty when ref is provided
//   - optimizer does not require an embedding source: with neither embeddingServerRef
//     nor optimizer.embeddingService set, find_tool runs FTS5 keyword-only search
//   - if embeddingServerRef is set without optimizer, auto-populate optimizer with defaults

gofmt -l flags the file as it stands, so linting will fail. task lint-fix sorts it.

// - if embeddingServerRef is set without optimizer, auto-populate optimizer with defaults
//
// The controller handles the remaining cases at runtime (event emission, URL population).
Expand All @@ -611,15 +614,6 @@

hasOptimizer := r.Spec.Config.Optimizer != nil
hasRef := r.Spec.EmbeddingServerRef != nil

@aponcedeleonch aponcedeleonch Jul 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth thinking about what happens to embeddingProvider and embeddingModel once this check is gone.

With no embedding source those two fields stop meaning anything, so I agree there's no reason to reject the spec over them. What we should do is tell the user they're inert, so nobody sets embeddingProvider: openai, sees keyword-only results, and spends an afternoon wondering why their gateway is never called.

One wrinkle to sort out first: today they aren't actually ignored. resolveEmbeddingProvider (pkg/vmcp/optimizer/optimizer.go:113-117) hard-errors for the openai provider when embeddingService is empty, so this spec doesn't degrade, it CrashLoopBackOffs:

config:
  optimizer:
    embeddingProvider: openai
    embeddingModel: text-embedding-3-small
    # no embeddingService, no embeddingServerRef

The CEL rule at line 22 of this file doesn't catch it, since that one only blocks embeddingServerRef combined with openai and there's no ref here. Before this PR Validate() caught it with a clear message. Now it reaches the pod and dies at startup with the reason only in pod logs, no condition or event pointing at why.

So "these fields are ignored" needs to become true before we can warn that it's true. That means short-circuiting on the absence of an embedding source before the provider switch:

func resolveEmbeddingProvider(optCfg *Config) error {
    if optCfg.EmbeddingService == "" {
        // No embedding source: semantic search is disabled entirely, so the
        // provider and model are inert. Skip provider validation rather than
        // failing startup over fields that will never be read.
        return nil
    }
    ...
}

With that in place the behavior matches the mental model (no embedding source means keyword-only, full stop, regardless of provider) and the warning is honest. Pair it with a line alongside the keyword-only event in the controller:

if config.Optimizer.EmbeddingProvider != "" || config.Optimizer.EmbeddingModel != "" {
    ctxLogger.Info("optimizer.embeddingProvider and optimizer.embeddingModel are ignored "+
        "when no embedding source is configured; set embeddingServerRef or "+
        "optimizer.embeddingService to enable semantic ranking",
        "embeddingProvider", config.Optimizer.EmbeddingProvider,
        "embeddingModel", config.Optimizer.EmbeddingModel)
}

Note embeddingProvider defaults to tei via kubebuilder, so on the operator path that check fires for essentially everyone in keyword-only mode. Probably worth folding the "provider and model ignored" sentence into the single keyword-only event rather than emitting a second one.

hasManualService := hasOptimizer && r.Spec.Config.Optimizer.EmbeddingService != ""

// Optimizer configured without any embedding source is an error.
// The user must either set embeddingServerRef or manually set optimizer.embeddingService.
if hasOptimizer && !hasRef && !hasManualService {
return fmt.Errorf(
"spec.config.optimizer requires an embedding service: " +
"set spec.embeddingServerRef (recommended) or spec.config.optimizer.embeddingService")
}

// EmbeddingServerRef is set but optimizer is not configured: auto-populate
// optimizer with default values so the embedding server is actually used.
Expand Down
5 changes: 2 additions & 3 deletions cmd/thv-operator/api/v1beta1/virtualmcpserver_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ func TestValidateEmbeddingServer(t *testing.T) {
expectOptimizer: true,
},
{
name: "optimizer_without_ref_or_service_errors",
name: "optimizer_without_ref_or_service_succeeds_keyword_only",
server: &VirtualMCPServer{
Spec: VirtualMCPServerSpec{
GroupRef: &MCPGroupRef{Name: "test-group"},
Expand All @@ -431,8 +431,7 @@ func TestValidateEmbeddingServer(t *testing.T) {
},
},
},
expectError: true,
errContains: "spec.config.optimizer requires an embedding service",
expectOptimizer: true,

@aponcedeleonch aponcedeleonch Jul 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This asserts the check is gone, which is right, but nothing yet asserts the newly-legal config produces a working deployment.

The keyword-only search path is well covered at the store layer (TestSQLiteToolStore_Search has 9 FTS5-only cases, plus _ResultsCapped, _Close, _Concurrent), but all of that predates this PR. The gaps are at the seams this PR opens:

  1. NewOptimizerFactory has no tests at all. Zero references to it in any test file. Coverage stops at NewSQLiteToolStore and NewEmbeddingClient separately, so nothing asserts that NewOptimizerFactory(&Config{}) with no service succeeds and yields a store that returns FTS5 results. That's precisely the composition this PR makes reachable from the operator.

  2. No operator-side test for populateOptimizerEmbeddingService in the ref-nil, svc-empty case. Worth asserting the ConfigMap still carries a non-nil config.Optimizer with an empty embeddingService, so a later refactor can't quietly drop the optimizer block. One trap: TestOptimizerEmbeddingServiceURL (controllers/virtualmcpserver_vmcpconfig_test.go:2149) looks like the natural home and already accepts esName == "", but its assertion block is wrapped in if tt.expectedURL != "" at line 2262, so a new case with an empty expectedURL would pass while asserting nothing.

  3. No envtest case for spec.config.optimizer: {} with no ref being admitted. cmd/thv-operator/test-integration/virtualmcp/virtualmcpserver_embedding_cel_test.go is the place, and it's cheap to add next to the existing provider cases.

  4. If resolveEmbeddingProvider gains the empty-service short-circuit, it wants a case asserting embeddingProvider: openai with no service returns cleanly instead of erroring. That's the regression pin for the whole "provider is inert without a source" contract.

Items 1 and 2 feel like they belong in this PR. 3 and 4 could be follow-ups, though 4 should land with whatever change introduces it.

},
{
name: "empty_ref_name_errors",
Expand Down
Loading