Skip the Installation finalizer write when nothing changed

[eastagiletracker]

This PR proposes skipping the Installation finalizer write when the finalizer is already present, so a steady-state reconcile stops issuing an optimistic-lock patch that has nothing to change and can only fail (Fixes #4999). We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/402. You can sign in with your GitHub ID to claim ownership of the project.

Description

This is a bug fix in pkg/controller/utils, affecting every controller that maintains a finalizer on the Installation: whisker, goldmane, apiserver, gatewayapi and clusterconnection.

MaintainInstallationFinalizer reads the Installation, captures a patch base with client.MergeFromWithOptimisticLock{}, and then patches unconditionally — including the ordinary case where the controller's finalizer is already in the list and nothing needs writing. The optimistic lock means that patch is never an empty no-op: mergeFromPatch.Data always writes metadata.resourceVersion into the body, so the request goes to the apiserver every time. That happens on every reconcile of each of those controllers — at minimum once per PeriodicReconcileTime (5 minutes) each, and again on every watch event.

Because each of those writes carries a resourceVersion precondition, any other writer touching the Installation between the read and the write turns a request that had nothing to change into a 409. The callers do not treat that as benign: it becomes SetDegraded(ResourceReadError, "Error setting finalizer on Installation", ...), which is the error reported in #4999 for a cluster where Argo CD also writes the object.

The removal path already guards against precisely this — it returns early when the finalizer is not present, rather than writing to discover there was nothing to do. This change adds the mirror-image guard on the add path, so the write only happens when the finalizer list actually changes.

Reproducing on master

The five specs added here run against pkg/controller/utils. On master at 5615c1a, two of them fail:

$ go test ./pkg/controller/utils/ -args -ginkgo.focus="MaintainInstallationFinalizer"

Summarizing 2 Failures:
  [FAIL] MaintainInstallationFinalizer [It] does not write the Installation when the finalizer is already present
  [FAIL] MaintainInstallationFinalizer [It] does not fail when another writer updates the Installation and no finalizer changed

Ran 5 of 446 Specs in 0.107 seconds
FAIL! -- 3 Passed | 2 Failed | 0 Pending | 441 Skipped

The second failure reproduces the reported symptom exactly, with the apiserver's own wording:

[FAILED] Unexpected error:
    Operation cannot be fulfilled on installations.operator.tigera.io "default": object was modified
    Reason: "Conflict",
    Code: 409,

The three specs that pass on both trees are the controls: the finalizer is still added when it is missing, finalizers owned by other controllers are still preserved alongside it, and the finalizer is still removed once the main resource is gone. They are there so the guard cannot pass by simply doing less work.

Testing

Automated only; I have no cluster reproduction of the Argo CD setup in #4999, so the concurrent writer is modelled with an interceptor that updates the Installation between the read and the write.

With the change applied, all five specs pass. The wider suite was run before and after the change with no new failures — go test ./pkg/... is green across 98 packages, using envtest assets from setup-envtest use 1.34.x to match the ut target (without them the Installation CEL validation specs fail in BeforeEach on a missing control plane, before and after alike).

One thing worth being explicit about: this does not make finalizer writes conflict-proof. A genuine first-time add can still lose the optimistic lock if another writer lands at the same instant, and that path is deliberately unchanged — it has something real to write, and it retries on the next reconcile. What goes away is the steady-state write that recurs for the life of the cluster, which is the one behind a recurring error.

Release Note

Stop rewriting the Installation resource on every reconcile when the controller's finalizer is already present, which could surface as a spurious "Error setting finalizer on Installation" degraded status on clusters where another controller also writes the Installation.

For PR author

• Tests for change.
• If changing pkg/apis/, run make gen-files — not changed.
• If changing versions, run make gen-versions — not changed.

How this was managed

This work was tracked on a board imported from this repository's own issues and pull requests — 5,202 stories, with release milestones as epics. The story for this fix is Error setting finalizer on Installation errors in operator logs, on the board at https://eastagiletracker.com/projects/402.

If you'd rather not receive contributions like this, reply no-more-prs on this pull request and we won't open any further ones on your repositories.

---

Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds an optimization to MaintainInstallationFinalizer to avoid unnecessary patches (and resulting optimistic-lock conflicts) when the desired finalizer state is already satisfied, and introduces targeted tests to validate patch behavior under concurrent writes.

Changes:

• Skip patching the Installation when the finalizer is already present (main resource exists), avoiding empty-patch conflict errors.
• Add new Ginkgo specs using controller-runtime client interceptors to assert when Patch is (and isn’t) called.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
pkg/controller/utils/utils.go	Adds an early-return optimization when the finalizer is already present to avoid needless optimistic-lock patch conflicts.
pkg/controller/utils/utils_test.go	Adds tests for MaintainInstallationFinalizer, including verifying no patch occurs when nothing changes and simulating another writer updating the Installation.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[caseydavenport]

Let's revert this comment change - I don't think it's adding anything that isn't obvious from the code.

[caseydavenport]

@eastagiletracker You'll need to sign the CLA if we want to merge this. Not sure if you're capable of doing that, since you're a bot. I'll give you a bit and then probably close this.