refactor: stop no-op reconcile writes that churn resourceVersion at scale - #32
Merged
Merged
Conversation
…cale
Every ensure* helper did `existing.Spec = desired.Spec; Update()` unconditionally,
so an unchanged object was rewritten on EVERY reconcile — bumping resourceVersion
and emitting an audit event each pass, and amplifying reconciles back through the
informer. That is real API-server load at 500+ instances (design-review Q3), the
same churn the upstream community operator hit in production (valkey-io #315/#272).
Gate each Update, picking the tool that fits the object's shape:
- Service ports, NetworkPolicy spec: exact DeepEqual. The builder fully specifies
these (Protocol/PolicyTypes included) and the API server defaults nothing inside
them, so an exact compare skips no-ops AND catches a shrink (turning metrics off
drops a port).
- StatefulSet, backup CronJob: a desired-hash annotation (appliedSpecHash). Their
pod templates carry BOTH server defaults (DeepEqual would churn) AND
operator-owned lists that can shrink — a metrics sidecar or TLS volume removed
(DeepDerivative would prefix-match and silently skip the removal). Hashing the
desired value ignores defaults and detects removals.
- PodDisruptionBudget: DeepDerivative (resourceSettled) — no shrinkable list.
- ServiceMonitor: left unconditional (documented): its desired spec is
map[string]string inside an unstructured map[string]interface{}, so any
structural compare is a type mismatch; it is one object per cluster.
Cross-model review (Grok) drove the shape of this: it caught the Service/NP
prefix-match shrink bug and that the first StatefulSet gate was unsafe on
sidecar removal, which pushed the STS/CronJob paths to the hash approach.
Verified live on a dedicated k3d cluster: across five reconcile intervals a
steady-state cluster's Service/NetworkPolicy resourceVersion and StatefulSet
generation do not move (no operator writes), while unit tests prove each gate
still updates on a real change and, crucially, on a list shrink.
Follow-up (out of scope): ensureConfigMap still updates unconditionally.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every
ensure*helper reconciled withexisting.Spec = desired.Spec; Update()unconditionally, so an unchanged object got rewritten on every reconcile. At fleet scale that is constant resourceVersion churn and audit-log spam, plus reconcile amplification back through the informer. The upstream community operator hit the same thing in production (valkey-io/valkey-operator #315).Each Update is now gated by the tool that fits the object's shape.
DeepEqual. The builder fully specifies these (Protocol, PolicyTypes) and the API server defaults nothing inside them, so DeepEqual both skips a no-op and catches a shrink (turning metrics off drops a port).appliedSpecHash). Their pod templates carry both API-server defaults (DeepEqual would churn on them) and operator-owned lists that can shrink, like a metrics sidecar or a TLS volume being removed. Hashing the desired value ignores defaults and still detects a removal;DeepDerivativewould treat the shorter list as an already-satisfied prefix and silently skip it.DeepDerivative. It has no operator-owned list that can shrink.map[string]stringinside an unstructuredmap[string]interface{}, so a structural compare is a type mismatch. It is one object per cluster, so the write is cheap.Why the mix rather than one strategy
No single check works everywhere.
DeepEqualchurns on server-defaulted fields,DeepDerivativesilently skips a list shrink, and the pod-template resources have both problems at once. The hash annotation is the one gate that handles defaults and removals together.Verification
make lintare clean.A cross-model review (Grok) shaped this. It caught the Service/NetworkPolicy shrink bug and that the first StatefulSet gate was unsafe on sidecar removal, which is what pushed the StatefulSet and CronJob paths onto the hash approach.
Out of scope, tracked separately:
ensureConfigMapstill updates unconditionally.