fix(operator): skip no-op VirtualMCPServer Deployment updates

[RaviTharuma]

Cross-links

• Issue: bug(operator): VirtualMCPServer statusReportingInterval requeue Updates the Deployment every interval (no rollout) #6340
• Maintainer direction: bug(operator): VirtualMCPServer statusReportingInterval requeue Updates the Deployment every interval (no rollout) #6340 (comment)
• Follow-up (CRD default 30s): bug(operator): VirtualMCPServer statusReportingInterval requeue Updates the Deployment every interval (no rollout) #6340 (comment)
• PR announcement: bug(operator): VirtualMCPServer statusReportingInterval requeue Updates the Deployment every interval (no rollout) #6340 (comment)

Summary

VirtualMCPServer statusReportingInterval requeues (including the CRD 30s
default) called Update on the Deployment even when the pod template was
unchanged. That bumped metadata.generation and emitted DeploymentUpdated
with no new ReplicaSet — and only for vMCPs that had a unique
podTemplateSpec.

Root cause: podTemplateMetadataNeedsUpdate used maps.Equal on the full
label/annotation maps. User PodTemplateSpec merge leaves extra keys on the
live template, so every status tick looked like drift.

• Compare pod-template metadata with MapIsSubset (expected ⊆ live)
• Compare container env with equality.Semantic.DeepEqual (K8s defaulting)
• Skip the Deployment write when the computed template, labels, merged
  annotations, and replicas are unchanged

Fixes #6340

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

• go test -ldflags=-extldflags=-Wl,-w ./cmd/thv-operator/controllers/ -run 'TestVirtualMCPServerEnsureDeployment_PodTemplateSpecSteadyState|TestVirtualMCPServerPodTemplateSpec'
• CI unit / operator tests
• After merge: a VirtualMCPServer with a unique podTemplateSpec should
  not emit DeploymentUpdated on every statusReportingInterval tick

API Compatibility

• This PR does not break the v1beta1 API, OR the api-break-allowed label 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 podTemplateSpec extras from looking
like a spec change; the skip-write is a second line of defense if
deploymentNeedsUpdate is still true for a non-template reason.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[RaviTharuma]

Implements #6340 per maintainer direction.

Cross-links:

• Issue: bug(operator): VirtualMCPServer statusReportingInterval requeue Updates the Deployment every interval (no rollout) #6340
• Maintainer comment: bug(operator): VirtualMCPServer statusReportingInterval requeue Updates the Deployment every interval (no rollout) #6340 (comment)
• Follow-up (CRD default 30s): bug(operator): VirtualMCPServer statusReportingInterval requeue Updates the Deployment every interval (no rollout) #6340 (comment)
• Issue PR note: bug(operator): VirtualMCPServer statusReportingInterval requeue Updates the Deployment every interval (no rollout) #6340 (comment)

[codecov]

Codecov Report

❌ Patch coverage is 85.48387% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.90%. Comparing base (c25e508) to head (d3f16fb).

Files with missing lines	Patch %	Lines
...perator/controllers/virtualmcpserver_controller.go	85.48%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6377      +/-   ##
==========================================
+ Coverage   78.88%   78.90%   +0.01%     
==========================================
  Files         781      781              
  Lines       77784    77840      +56     
==========================================
+ Hits        61362    61419      +57     
+ Misses      16417    16416       -1     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[reyortiz3]

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-ResourceVersion-unchanged pattern well.

One question on the "second line of defense" skip-write gate added in syncExistingVirtualMCPServerDeployment:

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
}

newDeployment comes from deploymentForVirtualMCPServer, which never sets RestartPolicy, DNSPolicy, SchedulerName, etc. on the PodSpec. On a real cluster the live deployment.Spec.Template.Spec will have those API-server-defaulted, so this full-template DeepEqual will differ from newDeployment's essentially every time — meaning this gate won't actually fire in production once deploymentNeedsUpdate is true for any reason. The fake client used in TestVirtualMCPServerEnsureDeployment_PodTemplateSpecSteadyState doesn't apply that defaulting, so the test can pass without this path ever proving itself against real API-server behavior.

Since the actual fix already lives in podTemplateMetadataNeedsUpdate (subset check) and containerNeedsUpdate (semantic env compare) — both of which gate the call into this function in the first place — is this extra check pulling its weight? If it's meant as genuine defense-in-depth, could we validate it with an envtest (real API server, real defaulting) instead of/in addition to the fake-client test? Otherwise I'd lean toward dropping it to keep syncExistingVirtualMCPServerDeployment simpler, since as written it's effectively unreachable.

[reyortiz3]

Hi @RaviTharuma , can you please solve the conflicts?

[RaviTharuma]

Hi @RaviTharuma , can you please solve the conflicts?

will do it, this weekend 👍

[RaviTharuma]

@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 deploymentNeedsUpdate → subset metadata checks + equality.Semantic.DeepEqual on env. A full-template DeepEqual against deploymentForVirtualMCPServer would keep missing API-server defaults, so it was not earning its keep as defense-in-depth. Happy to add an envtest later if you still want that coverage.

[RaviTharuma]

@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.

[Copilot]

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 ensureDeployment is 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]

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

[ChrisJBurns]

@RaviTharuma Not sure if they latest commits where meant to address the review comments, but they are still outstanding to resolve

[ChrisJBurns]

@RaviTharuma Not sure if they latest commits where meant to address the review comments, but they are still outstanding to resolve

[RaviTharuma]

Thank you for the ping — the two HIGH findings are now addressed on this branch (commits e04a2533 and 9ba7146e).

podTemplateMetadataNeedsUpdate now compares the fully rendered desired label/annotation set (controller metadata plus merged spec.podTemplateSpec) against live metadata, still with subset semantics so extra live keys such as pod-template-hash and restartedAt are not treated as drift. Mutation and removal of the configured tenant label now trigger an update.

containerNeedsUpdate now compares the fully rendered desired main-container env after PodTemplateSpec merge, with explicit normalization for known API defaults (SecretKeyRef/ConfigMapKeyRef Optional nil vs false, and FieldRef.APIVersion empty vs v1). Coverage includes PodTemplateSpec env overrides and defaulted/persisted env representations.

The #6340 steady-state behavior is unchanged: a status-interval requeue still does not Update when the pod template is unchanged.

Focused VirtualMCPServer controller tests were run and passed.

[ChrisJBurns]

Multi-Agent Consensus Review

Agents consulted: kubernetes-expert, test-coverage-reviewer, code-reviewer, codex, Kubernetes-defaulting adjudicator

Consensus Summary

#	Finding	Consensus	Severity	Action
1	Live vmcp container is selected by position	8/10	MEDIUM	Fix
2	FileKeyRef's Kubernetes 1.35 default is not normalized	8/10	MEDIUM	Fix

Overall

This PR makes VirtualMCPServer Deployment drift detection render the desired PodTemplate metadata and main-container environment before comparing them with live state. That approach is sound, and the configured-label and environment cases are now covered by load-bearing tests.

Two supported configurations can still recreate the repeated no-op update symptom: strategic merge may put a PodTemplateSpec sidecar before the vmcp container, while the live comparison assumes index zero; and Kubernetes 1.35 defaults FileKeyRef.Optional to false, which the new environment normalization does not yet collapse. Both can be addressed within the current comparison helpers.

---

Generated with the requested multi-agent pr-review workflow

[RaviTharuma]

@ChrisJBurns the Sep 7 MEDIUM findings are addressed in cf0513d1 (find vmcp by name; normalize FileKeyRef.Optional). Inline replies are on the threads. CI is green. Please re-review when you can.

[RaviTharuma]

@ChrisJBurns the Sep 7 MEDIUM findings are addressed in cf0513d1 (find vmcp by name; normalize FileKeyRef.Optional). Inline replies are on the threads. CI is green. Please re-review when you can.

[ChrisJBurns]

Multi-Agent Consensus Review

Agents consulted: kubernetes-expert, test-coverage-reviewer, code-reviewer, codex

Consensus Summary

No blocking findings.

Overall

The VirtualMCPServer drift detection consistently renders and compares desired metadata and environment state, locates the vmcp container by name when PodTemplateSpec customizations add sidecars, and normalizes Kubernetes 1.35's FileKeyRef.Optional default. The approach is coherent, the operator test suite passes, and required CI is green.

Dedicated regression coverage for the sidecar-ordering and FileKeyRef cases would still be useful, but those are non-blocking test-coverage improvements.

---

Generated with the requested multi-agent pr-review workflow