fix: avoid stale resourceVersion errors in ServiceL2Status and ServiceBGPStatus reconcile - #3079

Rebased onto latest main to pick up the go 1.25.11 / x/net v0.55.0 bump (5be638f0) — that clears the static-security-analysis govulncheck failures, which were stale-toolchain findings unrelated to this PR's changes. The bgp_status_controller.go case @yahlifried mentioned is already handled in 3b7e3a9f with a deterministic reproducer test. PTAL — thanks!

@yahlifried @oribon — could I ask for a re-run of the two failing e2e legs (e2e (ipv6, frr, manifests) and e2e (ipv6, frr-k8s, manifests)) on run 30073923459? I believe both are flakes rather than regressions from this PR:

• The failing specs do not touch what this PR changes. grep -niE "ServiceBGPStatus|ServiceL2Status" over unnummbered.go, service_selector.go and validate.go returns zero hits, while every spec that does exercise those status CRs (l2tests/l2.go, l2tests/interface_selector.go, bgptests/bgp.go) passed.
• The twin legs of the same modes passed in the same run: e2e (ipv6, frr, helm) and e2e (ipv6, frr-k8s, helm) are both green.
• Across this branch's three CI runs a different spec failed each time, and one run had e2e fully green — not the shape of a deterministic regression.
• Both failures are timeouts (timed out after 30s waiting for a route; timed out after 240s with 1 of 3 nodes not converging into VRF red), i.e. convergence timing.

Separately, is static-security-analysis a required check here? It looks repo-wide rather than PR-specific — main has been red on it consistently, the workflow installs the scanner unpinned (go install golang.org/x/vuln/cmd/govulncheck@latest), and the reported vulns are all in indirect deps this PR does not touch. This same branch was green on that leg on 2026-07-05 with essentially the same diff.

Happy to rebase if you would like, but since the one commit this is behind (5fb56ec8) only addresses 1 of the 3 reported vulns, I do not think it would clear the scan on its own. Let me know if you would prefer a different approach.

Thanks for taking a look, @oribon — happy to explain.

Short version: the race was always there. What the old code did was make it unrecoverable, so every retry reproduced the identical failure and no progress was ever made.

Why the old code failed

In the leader branch the reconciler picked its CreateOrPatch target straight out of the List result:

for _, item := range serviceL2statuses.Items {
    if item.Labels[LabelAnnounceNode] == r.NodeName && state == nil {
        state = &item   // carries Name, Namespace, UID *and* ResourceVersion

controllerutil.CreateOrPatch begins with a Get on that object, and both outcomes are broken here:

• Get returns NotFound — another speaker briefly became leader and deleted our status between the List and this call. CreateOrPatch then takes the create path and calls Create(state). But state is still the listed object, so metadata.resourceVersion is populated and the API server rejects it with resourceVersion should not be set on objects to be created. A failed Get does not clear the struct, so nothing along that path ever strips the field.
• Get succeeds from a lagging informer cache while the object is already gone in etcd — it takes the patch path, and the patch fails with servicel2statuses.metallb.io "l2-<rand>" not found.

Those are exactly the two errors #3063 reports alternating: one race, observed from either side of the cache lag.

Why it doesn't stop

This is the part worth being precise about — it is a livelock rather than a literal for {}, which I think is where the "infinite loop" wording is causing confusion.

The reconcile returns an error, so controller-runtime requeues it. On the requeue the reconciler lists again and — as long as the informer cache still serves the deleted status — selects the same object with the same dead resourceVersion and fails at exactly the same line.

The decisive property is that the failing attempt is completely side-effect-free: nothing is created, nothing is patched, and neither the cluster nor the cached object changes. So the next attempt runs with byte-identical inputs and produces a byte-identical failure. There is no state transition anywhere in the cycle that could let it exit — it can only escape if the cache happens to catch up. Under multi-speaker leader churn the window keeps reopening, so in practice it doesn't: hence the sustained ~5–6 errors/sec per service in the reporter's 3-node repro, and 859 errors in ~7 minutes in the downstream report (spatiumnorth/spatiumddi#284).

That is also why it is HA-specific — with a single speaker there is no concurrent deleter, the window never opens, and the same code path is fine.

Why the change fixes it

The fix stops reusing the listed object as the write target and builds one carrying identity only:

state = &v1beta1.ServiceL2Status{
    ObjectMeta: metav1.ObjectMeta{Name: item.Name, Namespace: item.Namespace},
}

The same race now resolves on the first pass: Get returns NotFound, the create path runs with no resourceVersion set, Create succeeds, and the reconcile returns nil. The status exists again, and the following reconcile short-circuits on DeepEqual and writes nothing. The race isn't removed — it's made recoverable in a single pass instead of unrecoverable.

CreateOrPatch re-populates the object via Get before patching, so the non-racy path is unaffected. The only thing dropped is item.Status, which the no-op short-circuit needs — that's why the change keeps it separately as observedStatus instead of reading state.Status. Without that, the comparison would always be false and we'd trade one source of churn for another.

What the test pins down

layer2_status_controller_test.go encodes precisely this sequence: a status is created (so it carries a real resourceVersion), the client is armed to delete it immediately after the List — standing in for the speaker that briefly became leader — and the reconcile must return without error and converge to exactly one owned status for the node. Against the pre-fix code that first Reconcile returns the resourceVersion should not be set on objects to be created error. bgp_status_controller_test.go does the same for the BGP side, which is the case @yahlifried asked about earlier.

Happy to fold any of this into the commit message or an in-tree comment if you'd like it recorded there.

I understand why the 2 scenarios you described are solved by the fix, what I don't understand is why we would get so many logs like mentioned in the issue (and repeatedly), other than "churn" and "cache lag" (which sounds a bit odd, because the issue mentions that this is easily reproducible).
To be clear, I'm fine with the code changes and the scenarios you're describing are good to fix, but I'm not sure I understand the root cause.

Fair question, and I think my earlier answer was wrong to lean on "churn". You're right that a race doesn't square with mzac reporting it as 100% reproducible from startup.

I dug into the v0.15.3 -> v0.16.0 boundary. The only commit touching this file in that range is c501ad7 ("L2Status controller: refactor to leader node manages resources"), and it changed the object's lifecycle in a way I think explains the volume better than a race does:

v0.15.3 adopted whatever status existed for the service, with no label check:

if len(serviceL2statuses.Items) > 0 {
    state = &serviceL2statuses.Items[0]
}

so one object per service was reused and patched in place across leader changes. Stable identity.

v0.16.0 only adopts when Labels[announceNode] == r.NodeName, deletes anything else, and when nothing was adopted creates a replacement under a fresh GenerateName. So on any label/leader mismatch the object is destroyed and recreated with a new name rather than relabelled. The non-leader path deletes too, and since the controller watches ServiceL2Status, each delete and each create feeds another reconcile.

That reframes this PR: the stale resourceVersion isn't what generates the churn, it's what makes the churn unrecoverable. state carries the resourceVersion of the object the reconcile just observed; if that object is already gone when CreateOrPatch does its Get, the create path runs with a stale resourceVersion and is rejected outright, so that pass can never re-create the object and the cycle repeats instead of settling. That's the alternation in the issue (resourceVersion should not be set... <-> l2-<rand> not found).

So I'd say there are two things here, and this PR only fixes the second:

1. delete-and-recreate-under-a-new-name on leader/label mismatch (introduced by c501ad7)
2. stale resourceVersion turning that into a hard, non-recovering failure (this PR)

I haven't been able to prove from the code alone that (1) never converges with a single stable leader, so I don't want to overstate it. mzac's "starts the instant the speakers come up" suggests the leader view is still settling at that point. mzac offered to run a #3079 build against the 3-node repro, which would tell us whether the errors stop entirely or just stop being fatal. That seems like the cheapest way to settle it, and it also tells you whether (1) needs its own fix.

Happy to narrow this PR to (2) and open a separate issue for (1) if you'd prefer them split.

it's what makes the churn unrecoverable

I'm not sure that's correct, but I do agree it turns each churn event into several unnecessary errors instead of recovering in one pass, and that's what I think this PR should be about.
let's reframe the PR around being what it is (not mentioning infinite loops or whatever), and clean the code to only be around it (and no need for the abundance of comments inside). I'm not entirely sure it would fix what's mentioned in the issue (because the rca isn't convincing), but it's at least a bug deserving a fix (handling stale rvs).

Agreed, and thanks for pushing back on it — reframed the PR to be only about the stale resourceVersion handling.

• Dropped the root-cause narrative and the loop/flood framing from the title, description and release note. It now describes what you agreed it is: each such event produces several unnecessary errors instead of recovering in one pass.
• Changed Fixed #3063 to Related to #3063, so it no longer auto-closes an issue this may not fully explain.
• Trimmed the code to just the fix. The loop restructure in layer2_status_controller.go is reverted, so the only change there is the one branch that builds the write target. Source diff is down to +15/-2 and +14/-2.
• Cut the comment blocks down to three lines each, and removed the same narrative from the test comments.

The tests are unchanged in substance. Reverting only the two controller .go files still makes both specs fail with exactly resourceVersion should not be set on objects to be created, so they pin the behaviour rather than passing vacuously.

Also rebased onto current main.

The code changes look good. The comments are way too verbose though, both in the controllers and the tests. The production code comments (8-10 lines each) should be a single line like // avoid carrying a stale resourceVersion into CreateOrPatch. The test comments (struct docs, inline explanations) should be similarly trimmed, the code is self-explanatory enough - let the test name and PR description carry the context.
Also, drop the "churn" and "looping forever" language throughout, this PR is about avoiding stale resourceVersions on concurrent deletes, not about fixing a reconcile loop.

Trimmed as suggested. The production comments are now the one-liner you proposed, and the test comments are down to the two non-obvious ones (the missing field index, and why a second reconcile is needed) — the rest now rides on the spec name and the PR description.

Also dropped the "churn"/"loop" wording everywhere, including the two commit messages and the test identifiers, so nothing in the code claims this is a reconcile-loop fix.

No logic changed — the diff against the previous push is comments and naming only. 10/10 specs still pass on envtest 1.34.1, gofmt/go vet clean, golangci-lint 0 issues.