-
Notifications
You must be signed in to change notification settings - Fork 149
Unified Workload Discovery for vmcp #2487
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2487 +/- ##
==========================================
- Coverage 55.84% 55.58% -0.27%
==========================================
Files 312 312
Lines 29541 29658 +117
==========================================
- Hits 16498 16485 -13
- Misses 11601 11733 +132
+ Partials 1442 1440 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dmjb
left a comment
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.
Marking this as "request changes" until we figure out how to split up this interface.
Summary
This PR implements a unified workload discovery interface for vmcp's backend discovery, enabling seamless discovery of MCP server workloads from both CLI (Docker/Podman) and Kubernetes environments. The implementation uses a minimal interface pattern to avoid coupling vmcp to platform-specific workload management implementations.
Motivation
The vmcp aggregator needs to discover backend MCP servers from workloads, but the existing workload managers (
workloads.Managerfor CLI andk8s.Managerfor Kubernetes) expose many methods that vmcp doesn't need. This PR introduces a minimalWorkloadDiscovererinterface that:Minimal Interface: Only exposes methods needed for backend discovery (
ListWorkloadsInGroup,GetWorkload)Platform Agnostic: Works with both CLI and Kubernetes workloads through a single interface
No Import Cycles: Interface and implementations are in
pkg/vmcp/workloads, separate fromaggregatorDirect Implementations: Uses
statuses.StatusManagerfor CLI and direct Kubernetes client for K8s (no dependency on workload managers)Clean Architecture: Follows interface segregation principle - vmcp only depends on what it needs
Implementation
WorkloadDiscoverer Interface
Location:
pkg/vmcp/workloads/discoverer.goA minimal interface that exposes only the methods needed for backend discovery:
Direct Implementations
CLI Implementation (
pkg/vmcp/workloads/cli.go):Uses
statuses.StatusManagerdirectly (notworkloads.Manager)Converts
core.Workloadtovmcp.BackendMaps
runtime.WorkloadStatustovmcp.BackendHealthStatusFilters workloads without URLs (not accessible)
Kubernetes Implementation (
pkg/vmcp/workloads/k8s.go):Uses Kubernetes controller-runtime client directly (not
k8s.Manager)Queries
MCPServerCRDs directlyConverts
MCPServerCRD tovmcp.BackendMaps
mcpv1alpha1.MCPServerPhasetovmcp.BackendHealthStatusFilters workloads without URLs (not accessible)
Unified Backend Discoverer
Location:
pkg/vmcp/aggregator/discoverer.goA single
backendDiscovererthat works with both CLI and Kubernetes workloads through theWorkloadDiscovererinterface:Factory Function:
NewBackendDiscoverer()automatically detects runtime and creates the appropriate discovererManual Creation:
NewUnifiedBackendDiscoverer()accepts anyWorkloadDiscovererimplementationTesting Support:
NewBackendDiscovererWithManager()for injecting test implementationsPackage Structure
pkg/vmcp/workloads/:discoverer.go- Interface definitioncli.go- CLI implementation (NewCLIDiscoverer)k8s.go- Kubernetes implementation (NewK8SDiscoverer)mocks/mock_discoverer.go- Generated mock (no import cycles)pkg/vmcp/aggregator/:discoverer.go- Unified backend discoverer implementationdiscoverer_test.go- Tests using generated mocksKey Features
No Import Cycles
pkg/vmcp/workloads(separate fromaggregator)pkg/vmcp/workloads/mockswithout issuesDirect Platform Access
statuses.StatusManagerdirectly, bypassingworkloads.Managerk8s.ManagerUnified Discovery
BackendDiscovererinterface for both platformsComprehensive Testing
discoverermocks.NewMockDiscoverer)discoverer_test.go) with unified test casesFiles Added
Workload Discoverer Package:
pkg/vmcp/workloads/discoverer.go- Interface definitionpkg/vmcp/workloads/cli.go- CLI implementationpkg/vmcp/workloads/k8s.go- Kubernetes implementationpkg/vmcp/workloads/mocks/mock_discoverer.go- Generated mockAggregator:
pkg/vmcp/aggregator/discoverer.go- Unified backend discovererpkg/vmcp/aggregator/discoverer_test.go- Consolidated testsFiles Modified
pkg/vmcp/aggregator/discoverer.go- Updated to useworkloads.Discovererinterfacecmd/vmcp/app/commands.go- Updated to useaggregator.NewBackendDiscoverer()Files Removed
pkg/vmcp/aggregator/workload_discoverer.go- Moved topkg/vmcp/workloads/discoverer.gopkg/vmcp/aggregator/cli_workload_discoverer.go- Moved topkg/vmcp/workloads/cli.gopkg/vmcp/aggregator/k8s_workload_discoverer.go- Moved topkg/vmcp/workloads/k8s.gopkg/vmcp/aggregator/cli_discoverer_test.go- Consolidated intodiscoverer_test.gopkg/vmcp/aggregator/k8s_discoverer_test.go- Consolidated intodiscoverer_test.gopkg/vmcp/aggregator/mocks/mock_workload_discoverer.go- Replaced bypkg/vmcp/workloads/mocks/mock_discoverer.goBenefits
WorkloadDiscovererinterface can be used by other componentsTesting
Design Decisions
Why a Separate Package?
pkg/vmcp/workloadsprevents cycles withaggregatorpackageWorkloadDiscovererwithout importingaggregatorWhy Direct Implementations?
workloads.Managerork8s.Managerinterfacesstatuses.StatusManager, Kubernetes client)Why Minimal Interface?
Why Consolidated Tests?
Related
Example Usage
Automatic Runtime Detection:
Manual Creation (Testing):
Direct Implementation Usage:
implements #169