-
Notifications
You must be signed in to change notification settings - Fork 171
Simplify controller and add E2E tests for status reporting #3468
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
base: issue-3149-v2
Are you sure you want to change the base?
Conversation
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.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
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.
Pull request overview
This pull request simplifies the VirtualMCPServer controller by removing duplicate backend discovery logic (~130 lines) and delegating all backend discovery and health monitoring to the vMCP runtime via its StatusReporter mechanism. The change aligns with the architectural principle of separation of concerns: the controller manages infrastructure (RBAC, Deployment, Service, ConfigMap), while the vMCP runtime handles backend discovery, health monitoring, and status updates.
Changes:
- Removed
discoverBackends()method from controller (delegated to vMCP runtime with K8sReporter) - Removed controller-side backend discovery tests (virtualmcpserver_discover_backends_test.go)
- Removed integration test TestDiscoverBackendsWithExternalAuthConfigIntegration from virtualmcpserver_externalauth_test.go
- Added comprehensive E2E tests for status reporting (virtualmcp_status_reporting_test.go) with 2 test cases covering backend discovery, health monitoring, and failure recovery
- Added GetMCPServerDeployment helper function for E2E tests
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/thv-operator/virtualmcp/virtualmcp_status_reporting_test.go | New E2E test suite validating vMCP runtime status reporting for backend discovery and health monitoring |
| test/e2e/thv-operator/virtualmcp/helpers.go | Added GetMCPServerDeployment helper function to retrieve MCPServer deployments by name |
| cmd/thv-operator/controllers/virtualmcpserver_externalauth_test.go | Removed TestDiscoverBackendsWithExternalAuthConfigIntegration (170 lines) as backend discovery moved to runtime |
| cmd/thv-operator/controllers/virtualmcpserver_discover_backends_test.go | Deleted entire file (643 lines of controller-side backend discovery tests) |
| cmd/thv-operator/controllers/virtualmcpserver_controller.go | Removed discoverBackends method and unused imports (groups, aggregator); added comprehensive documentation explaining responsibility split |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Remove duplicate backend discovery from VirtualMCPServer controller, delegating all backend discovery and health monitoring to the vMCP runtime via StatusReporter. Controller simplification: - Remove discoverBackends() method (~130 lines) from controller - Backend discovery now handled by vMCP runtime with K8sReporter - Controller focuses on infrastructure: RBAC, Deployment, Service, ConfigMap - vMCP runtime handles: backend discovery, health monitoring, status updates Test cleanup: - Remove virtualmcpserver_discover_backends_test.go (controller-side tests) - Remove TestDiscoverBackendsWithExternalAuthConfigIntegration from virtualmcpserver_externalauth_test.go - Keep relevant auth conversion tests E2E test additions: - Add virtualmcp_status_reporting_test.go with comprehensive tests Related-to: #3149
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
Large PR justification has been provided. Thank you!
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.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## issue-3149-v2 #3468 +/- ##
=================================================
- Coverage 64.95% 64.91% -0.05%
=================================================
Files 396 395 -1
Lines 38577 38481 -96
=================================================
- Hits 25057 24979 -78
+ Misses 11575 11559 -16
+ Partials 1945 1943 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The vMCP runtime's K8sReporter updates VirtualMCPServer status in ALL modes
(discovered and inline), but the controller was only creating RBAC resources
for discovered mode. This caused status reporting to fail in inline mode with
permission errors.
Changes:
- Remove inline mode skip in ensureRBACResources() - create RBAC in all modes
- Simplify serviceAccountNameForVmcp() to always use dedicated service account
(unless custom SA is provided)
- Update RBAC rules comment to clarify usage in all modes
- Update test TestVirtualMCPServerEnsureRBACResources_StaticMode to expect
RBAC resources to be created in inline/static mode
The RBAC rules already include virtualmcpservers/status update permissions.
This change ensures those permissions are granted in all modes, allowing the
vMCP runtime to report backend discovery status, health monitoring, and phase
transitions regardless of the outgoing auth source configuration.
Remove duplicate backend discovery from VirtualMCPServer controller, delegating all backend discovery and health monitoring to the vMCP runtime via StatusReporter.
Controller simplification:
Test cleanup:
E2E test additions:
Related-to: #3149
Large PR Justification
This is a cleanup pr, that removes substantial lines of code to replace with newer ones. It also includes tests to validate the new behaviour.