fix(operator): skip no-op VirtualMCPServer Deployment updates - #6377
fix(operator): skip no-op VirtualMCPServer Deployment updates#6377RaviTharuma wants to merge 2 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6377 +/- ##
==========================================
- Coverage 78.31% 78.25% -0.07%
==========================================
Files 770 770
Lines 75456 75456
==========================================
- Hits 59095 59048 -47
- Misses 16356 16403 +47
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Nice fix for the root cause — the subset check + semantic env comparison directly address the drift false-positive from #6340, and the regression test follows the reconcile-twice/assert- One question on the "second line of defense" skip-write gate added in if equality.Semantic.DeepEqual(deployment.Spec.Template, newDeployment.Spec.Template) &&
maps.Equal(deployment.Labels, newDeployment.Labels) &&
maps.Equal(deployment.Annotations, mergedAnnotations) &&
replicasUnchanged {
return ctrl.Result{}, nil
}
Since the actual fix already lives in |
|
Hi @RaviTharuma , can you please solve the conflicts? |
will do it, this weekend 👍 |
9380995 to
fb3753a
Compare
|
@reyortiz3 Conflicts are resolved — the branch is MERGEABLE and CI is green. On the extra skip-write gate: dropped it, as you suggested. The production path is |
|
@reyortiz3 Friendly ping — conflicts are resolved, the skip-write gate is dropped as you suggested, and the branch is MERGEABLE with CI green. Happy to take any further review notes. |
fb3753a to
a012441
Compare
There was a problem hiding this comment.
Pull request overview
Fixes a VirtualMCPServer operator hot-loop where status-interval requeues were causing no-op Deployment Updates (generation bumps + DeploymentUpdated events) even when the rendered pod template was effectively unchanged.
Changes:
- Switch pod-template metadata comparison from full-map equality to a subset check so user-merged “extra” labels/annotations on the live template don’t look like drift.
- Compare container env vars using Kubernetes semantic equality to avoid false drift from defaulted pointer fields.
- Add/adjust regression coverage to ensure
ensureDeploymentis a no-op across repeated reconciles in the steady state.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/thv-operator/controllers/virtualmcpserver_controller.go | Adjusts drift detection (pod template metadata + env var comparison) to prevent no-op Deployment updates on status requeues. |
| cmd/thv-operator/controllers/virtualmcpserver_controller_test.go | Updates the pod-template-metadata drift test expectations to match subset-based behavior. |
| cmd/thv-operator/controllers/virtualmcpserver_podtemplatespec_reconcile_test.go | Adds a regression test covering steady-state reconciliation when the live template has extra merged metadata. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ChrisJBurns
left a comment
ChrisJBurns
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: kubernetes-expert, test-coverage-reviewer, code-reviewer, codex
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| 1 | PodTemplateSpec label drift is silently ignored | 8/10 | HIGH | Fix |
| 2 | Environment comparison does not implement the claimed normalization | 10/10 | HIGH | Fix |
Overall
This bug fix changes VirtualMCPServer Deployment drift detection and adds a steady-state regression test. Replacing exact label equality with a subset comparison addresses the reported write loop for merged metadata, and asserting an unchanged ResourceVersion is the right idempotency invariant.
Two correctness gaps remain. The label subset omits labels declared by the user in spec.podTemplateSpec, so deleting or changing those labels is no longer repaired. The environment comparison also does not normalize the pointer defaults named in its comment and compares against environment variables built before PodTemplateSpec overrides are merged, while the new tests never exercise a live-versus-expected environment difference. These should be resolved before merge.
Generated with the requested multi-agent pr-review workflow
statusReportingInterval requeues were treated as drift when a user PodTemplateSpec left extra labels on the live template. Compare pod-template metadata as a subset and use semantic env equality so generation stops bumping every interval. The extra full-template DeepEqual skip-write gate is omitted: API-server defaulting makes it unreachable, and the subset plus semantic-env checks already prevent the write. Fixes stacklok#6340 Co-authored-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>
Copilot asked to drop the apostrophe so the comment reads "updates the Deployment" instead of "Update's". Co-authored-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>
c61cc6b to
6f629de
Compare
|
@RaviTharuma Not sure if they latest commits where meant to address the review comments, but they are still outstanding to resolve |
Cross-links
Summary
VirtualMCPServer
statusReportingIntervalrequeues (including the CRD 30sdefault) called
Updateon the Deployment even when the pod template wasunchanged. That bumped
metadata.generationand emittedDeploymentUpdatedwith no new ReplicaSet — and only for vMCPs that had a unique
podTemplateSpec.Root cause:
podTemplateMetadataNeedsUpdateusedmaps.Equalon the fulllabel/annotation maps. User
PodTemplateSpecmerge leaves extra keys on thelive template, so every status tick looked like drift.
MapIsSubset(expected ⊆ live)equality.Semantic.DeepEqual(K8s defaulting)annotations, and replicas are unchanged
Fixes #6340
Type of change
Test plan
go test -ldflags=-extldflags=-Wl,-w ./cmd/thv-operator/controllers/ -run 'TestVirtualMCPServerEnsureDeployment_PodTemplateSpecSteadyState|TestVirtualMCPServerPodTemplateSpec'podTemplateSpecshouldnot emit
DeploymentUpdatedon everystatusReportingIntervaltickAPI Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.No CRD schema change.
Does this introduce a user-facing change?
Yes. VirtualMCPServer Deployments no longer get a no-op Update (and a
generation bump) on every status-interval reconcile.
Special notes for reviewers
Matches the suggested fix on #6340: do not treat the status requeue itself as
drift. The subset check is what stops
podTemplateSpecextras from lookinglike a spec change; the skip-write is a second line of defense if
deploymentNeedsUpdateis still true for a non-template reason.