Stop createOrUpdate from rewriting objects it watches on every reconcile

[aswinsuryan]

createOrUpdate did an unconditional Get-then-Update on every call, and both controllers watch the objects they write, so every write re-triggered their own reconcile - an infinite loop (already found and fixed once, in unmerged upstream PR #13, but never merged to main). It also fanned out further than that: CUDNBgpConfig's reconcile makes live AWS calls, and CUDNBgpRouting's shared RouteAdvertisements object turns one CR's rewrite into a reconcile storm across every CR.

createOrUpdate now skips the write when the spec and our own managed labels already match, while leaving any labels we don't own untouched.

Adds direct unit tests for createOrUpdate/specEqual/labelsSatisfied and regression tests in both controllers' reconcile tests. The envtest integration suite from the first pass of this work is left out for now and will follow separately.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 850a4c9c-6e64-46e0-a932-f978e134b808

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aswinsuryan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [aswinsuryan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[frobware]

I took this for a spin on a 4.22.9 AWS cluster with route servers up, and it does what it says for two of the three kinds -- CUDN and RouteAdvertisements both stop writing once they've converged. FRRConfiguration doesn't, and I think that's worth sorting before this lands.

FRRConfiguration still gets written on every pass

ensureSingleFRRConfiguration builds a neighbour as {address, asn, disableMP, toReceive}, but the CRD defaults dualStackAddressFamily, so what you read back always has one more field than what you sent:

{"address":"10.0.31.87","asn":65000,"disableMP":true,
 "dualStackAddressFamily":false,"toReceive":{"allowed":{"mode":"all"}}}

specEqual asks for exact equality, that extra key never goes away, so it's false every time and you fall straight through to the Update. Easy enough to see on a cluster:

oc get frrconfiguration cudn-bgp-1 -n openshift-frr-k8s \
  -o jsonpath='{.spec.bgp.routers[0].neighbors[0]}'

The unit tests can't catch it because the fake client doesn't apply CRD defaults.

It's less alarming than it sounds -- this isn't the old loop coming back. I watched it for five minutes: three write attempts, one reconcile, resourceVersion never budged. The API server discards an update that changes nothing, so you're paying one wasted Update per FRRConfiguration per reconcile rather than spinning.

The way out is to compare only the fields you actually set, rather than insisting the whole spec matches. This passes your tests as they stand, is lint clean, and still catches drift in asn, address, mode, a neighbour going missing, and so on:

func specEqual(existing, desired *unstructured.Unstructured) bool {
	existingSpec, _, _ := unstructured.NestedMap(existing.Object, "spec")
	desiredSpec, _, _ := unstructured.NestedMap(desired.Object, "spec")
	return specSatisfied(existingSpec, desiredSpec)
}

// specSatisfied reports whether everything we set is already present with the
// value we want. Fields we never set are ignored, because the API server adds
// its own: FRRConfiguration defaults neighbors[].dualStackAddressFamily, so an
// exact comparison is false on every pass and we rewrite forever.
func specSatisfied(existing, desired interface{}) bool {
	switch want := desired.(type) {
	case map[string]interface{}:
		have, ok := existing.(map[string]interface{})
		if !ok {
			return false
		}
		for k, v := range want {
			if !specSatisfied(have[k], v) {
				return false
			}
		}
		return true
	case []interface{}:
		have, ok := existing.([]interface{})
		if !ok || len(have) != len(want) {
			return false
		}
		for i := range want {
			if !specSatisfied(have[i], want[i]) {
				return false
			}
		}
		return true
	default:
		return apiequality.Semantic.DeepEqual(existing, desired)
	}
}

The downside is that a field you never set, which somebody then edits by hand, won't be reverted any more. Since you only ever send a partial spec I'd argue that's the behaviour you want regardless. Server-side apply with a field owner would do the same job more neatly, but that's a bigger change than belongs in this PR.

The update path takes other people's labels and annotations with it

c.Update(ctx, obj) replaces metadata wholesale, so anything you didn't put there disappears. The comment on labelsSatisfied says extra labels are left alone, which is only true while you're skipping the write.

Because of the problem above, that's happening today. I put a label and an annotation on cudn-bgp-1 and both were gone after a single reconcile:

before: labels={"app.kubernetes.io/managed-by":"...","foreign.io/owner":"someone-else"}
        annots={"foreign.io/note":"keep-me"}
after:  labels={"app.kubernetes.io/managed-by":"..."}
        annots=

Fixing the first problem hides this most of the time, but it doesn't go away -- any genuine spec change brings it back. And it isn't only labels: the CUDN carries k8s.ovn.org/user-defined-network-protection from ovn-kubernetes, so the first time that spec changes you'd strip ovn-k's finalizer off it.

mergeLabels is already sitting in the file, so just before the Update:

obj.SetLabels(mergeLabels(existing.GetLabels(), obj.GetLabels()))

Annotations want the same treatment.

On the tests

They're good and they earn their keep -- pull the guard out and all three regression tests go red, drop labelsSatisfied from the condition and TestCreateOrUpdate_UpdatesWhenManagedLabelMissing goes red. Worth keeping whichever way #13 ends up, since #13 has no direct coverage of createOrUpdate, specEqual or labelsSatisfied. The one gap is that nothing here can catch the FRRConfiguration problem, which is exactly what the envtest suite you've deferred would be for.

One aside while I was in there: the API server warns that disableMP is deprecated and that dualStackAddressFamily replaces it -- the very field causing the mismatch above. Probably deserves its own issue.

[openshift-ci]

@aswinsuryan: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[aswinsuryan]

I took this for a spin on a 4.22.9 AWS cluster with route servers up, and it does what it says for two of the three kinds -- CUDN and RouteAdvertisements both stop writing once they've converged. FRRConfiguration doesn't, and I think that's worth sorting before this lands.

FRRConfiguration still gets written on every pass

ensureSingleFRRConfiguration builds a neighbour as {address, asn, disableMP, toReceive}, but the CRD defaults dualStackAddressFamily, so what you read back always has one more field than what you sent:

{"address":"10.0.31.87","asn":65000,"disableMP":true,
 "dualStackAddressFamily":false,"toReceive":{"allowed":{"mode":"all"}}}

specEqual asks for exact equality, that extra key never goes away, so it's false every time and you fall straight through to the Update. Easy enough to see on a cluster:

oc get frrconfiguration cudn-bgp-1 -n openshift-frr-k8s \
  -o jsonpath='{.spec.bgp.routers[0].neighbors[0]}'

The unit tests can't catch it because the fake client doesn't apply CRD defaults.

It's less alarming than it sounds -- this isn't the old loop coming back. I watched it for five minutes: three write attempts, one reconcile, resourceVersion never budged. The API server discards an update that changes nothing, so you're paying one wasted Update per FRRConfiguration per reconcile rather than spinning.

The way out is to compare only the fields you actually set, rather than insisting the whole spec matches. This passes your tests as they stand, is lint clean, and still catches drift in asn, address, mode, a neighbour going missing, and so on:

func specEqual(existing, desired *unstructured.Unstructured) bool {
	existingSpec, _, _ := unstructured.NestedMap(existing.Object, "spec")
	desiredSpec, _, _ := unstructured.NestedMap(desired.Object, "spec")
	return specSatisfied(existingSpec, desiredSpec)
}

// specSatisfied reports whether everything we set is already present with the
// value we want. Fields we never set are ignored, because the API server adds
// its own: FRRConfiguration defaults neighbors[].dualStackAddressFamily, so an
// exact comparison is false on every pass and we rewrite forever.
func specSatisfied(existing, desired interface{}) bool {
	switch want := desired.(type) {
	case map[string]interface{}:
		have, ok := existing.(map[string]interface{})
		if !ok {
			return false
		}
		for k, v := range want {
			if !specSatisfied(have[k], v) {
				return false
			}
		}
		return true
	case []interface{}:
		have, ok := existing.([]interface{})
		if !ok || len(have) != len(want) {
			return false
		}
		for i := range want {
			if !specSatisfied(have[i], want[i]) {
				return false
			}
		}
		return true
	default:
		return apiequality.Semantic.DeepEqual(existing, desired)
	}
}

The downside is that a field you never set, which somebody then edits by hand, won't be reverted any more. Since you only ever send a partial spec I'd argue that's the behaviour you want regardless. Server-side apply with a field owner would do the same job more neatly, but that's a bigger change than belongs in this PR.

The update path takes other people's labels and annotations with it

c.Update(ctx, obj) replaces metadata wholesale, so anything you didn't put there disappears. The comment on labelsSatisfied says extra labels are left alone, which is only true while you're skipping the write.

Because of the problem above, that's happening today. I put a label and an annotation on cudn-bgp-1 and both were gone after a single reconcile:

before: labels={"app.kubernetes.io/managed-by":"...","foreign.io/owner":"someone-else"}
        annots={"foreign.io/note":"keep-me"}
after:  labels={"app.kubernetes.io/managed-by":"..."}
        annots=

Fixing the first problem hides this most of the time, but it doesn't go away -- any genuine spec change brings it back. And it isn't only labels: the CUDN carries k8s.ovn.org/user-defined-network-protection from ovn-kubernetes, so the first time that spec changes you'd strip ovn-k's finalizer off it.

mergeLabels is already sitting in the file, so just before the Update:

obj.SetLabels(mergeLabels(existing.GetLabels(), obj.GetLabels()))

Annotations want the same treatment.

On the tests

They're good and they earn their keep -- pull the guard out and all three regression tests go red, drop labelsSatisfied from the condition and TestCreateOrUpdate_UpdatesWhenManagedLabelMissing goes red. Worth keeping whichever way #13 ends up, since #13 has no direct coverage of createOrUpdate, specEqual or labelsSatisfied. The one gap is that nothing here can catch the FRRConfiguration problem, which is exactly what the envtest suite you've deferred would be for.

One aside while I was in there: the API server warns that disableMP is deprecated and that dualStackAddressFamily replaces it -- the very field causing the mismatch above. Probably deserves its own issue.

Thanks for the review, fixed both. specEqual now only checks that the fields we set are present with the value we want, instead of exact matching the whole spec, so FRRConfiguration's server defaulted dualStackAddressFamily no longer forces an update on every reconcile. Labels and annotations from the existing object are now merged in before Update, so anything we don't manage, like a foreign label or ovn-kubernetes finalizer or annotations, survives a real update instead of being wiped by the wholesale metadata replace.

[frobware]

Some measurements from a live cluster, in case they help move this along.

Reproduced on a 4.22.10 IPI AWS cluster in us-east-2, three availability zones, one CUDNBgpConfig and one CUDNBgpRouting, with the full e2e suite passing throughout:

• 827 reconciles of CUDNBgpRouting between 10:09:54 and 10:14:26, which is 2.45 per second, sustained
• every one ran both phases and logged reconciliation complete
• every condition stayed healthy the whole time, and the CR sat at Ready
• CUDNBgpConfig logged 52 lines over the same window, for comparison
• it stopped only when the CR was deleted

So this is not a slow drift. It is the operator writing at whatever rate the API server will serve, indefinitely, while reporting itself healthy. Nothing fails, which is presumably why it has gone unnoticed for so long.

On the fix: the subset comparison is the part that matters, and it is worth saying why, because an exact DeepEqual on spec looks equivalent and is not. The API server defaults fields the operator never sets, so an exact comparison never matches, and the object is rewritten every reconcile regardless. The loop survives. A fake client does no defaulting, so a unit test cannot tell the two implementations apart -- which makes this a fix that can look green and not work.

#76 targets the same loop with an exact comparison, so these two want deciding between rather than both merging. The one piece there that this PR does not cover is retry.RetryOnConflict around the Update. That is a separate failure: ovn-kubernetes modifying the same object concurrently, the Update losing on optimistic concurrency, and the reconciler marking itself Degraded as a result. Worth lifting onto this one.