-
Notifications
You must be signed in to change notification settings - Fork 153
fix vmcp crd don't reconcile after podtemplatespec changes #2817
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 #2817 +/- ##
==========================================
- Coverage 56.59% 56.51% -0.08%
==========================================
Files 322 322
Lines 31247 31333 +86
==========================================
+ Hits 17685 17709 +24
- Misses 12042 12096 +54
- Partials 1520 1528 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 PR fixes a bug where VirtualMCPServer CRD resources were not being reconciled when PodTemplateSpec changes were made (#2784). The fix adds detection logic for PodTemplateSpec changes and implements strategic merge patching to apply user-provided pod customizations.
Key Changes:
- Added PodTemplateSpec validation early in the reconciliation flow to catch invalid specs before deployment creation
- Implemented
podTemplateSpecNeedsUpdatemethod to detect when user-provided PodTemplateSpec has changed - Created
VirtualMCPServerPodTemplateSpecBuilderto validate and build PodTemplateSpec customizations - Applied strategic merge patch in
applyPodTemplateSpecToDeploymentto merge user customizations with controller defaults
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
virtualmcpserver_controller.go |
Added PodTemplateSpec validation function and change detection logic in deployment update checks |
virtualmcpserver_deployment.go |
Implemented strategic merge patch application for PodTemplateSpec and removed obsolete TODO comment |
virtualmcpserver_podtemplatespec_builder.go |
New builder pattern for validating and constructing PodTemplateSpec customizations |
virtualmcpserver_podtemplatespec_test.go |
Unit tests for PodTemplateSpec builder validation and parsing |
virtualmcpserver_podtemplatespec_reconcile_test.go |
Tests for deterministic generation and change detection of PodTemplateSpec updates |
virtualmcpserver_controller_test.go |
Refactored to use test constants (testVmcpName, testGroupName) for consistency |
mcpgroup_controller_test.go |
Added testGroupName constant and updated test cases to use it consistently |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cmd/thv-operator/controllers/virtualmcpserver_podtemplatespec_reconcile_test.go
Show resolved
Hide resolved
cmd/thv-operator/controllers/virtualmcpserver_controller_test.go
Outdated
Show resolved
Hide resolved
cmd/thv-operator/controllers/virtualmcpserver_controller_test.go
Outdated
Show resolved
Hide resolved
cmd/thv-operator/controllers/virtualmcpserver_controller_test.go
Outdated
Show resolved
Hide resolved
cmd/thv-operator/controllers/virtualmcpserver_controller_test.go
Outdated
Show resolved
Hide resolved
|
I think there is quite a lot of code duplication with this PR because the podTemplateSpec in this PR is nearly identical to the one used by MCPServer. I will give claude a shot at unifying those. |
|
I split out the pod template spec in #2840 - let's see if it gets accepted and if yes, we should rebase this PR atop it. |
cmd/thv-operator/controllers/virtualmcpserver_podtemplatespec_builder.go
Outdated
Show resolved
Hide resolved
Follow-up Suggestion: Status Update Pattern ConsistencyIn Consider refactoring in a follow-up to use StatusManager consistently for:
This is a minor consistency issue and doesn't block the PR. |
Closes: #2784