[CORE-149029] Fix broker restart loop: trigger on CR intent change, not live-pod drift - #269
Open
dobrerazvan wants to merge 9 commits into
Open
[CORE-149029] Fix broker restart loop: trigger on CR intent change, not live-pod drift#269dobrerazvan wants to merge 9 commits into
dobrerazvan wants to merge 9 commits into
Conversation
dobrerazvan
force-pushed
the
CORE-149029-fix
branch
from
July 10, 2026 09:55
c78708b to
5321a04
Compare
amuraru
approved these changes
Jul 11, 2026
dobrerazvan
force-pushed
the
CORE-149029-fix
branch
from
August 18, 2026 09:04
20d3f2d to
f94ba9b
Compare
PreferredDuringSchedulingIgnoredDuringExecution has no patchMergeKey in the Kubernetes API types, so the three-way strategic merge patch treats it as an atomic list. Any term injected by an admission controller (e.g. ScaleOps) is removed on every reconcile because the list in the running pod no longer matches the desired spec. Replace the previous sync-copy approach (which corrupted the last-applied annotation and silently suppressed intentional CR changes) with a CalculateOption that strips preferred affinities from both sides of the diff before the merge is computed. The diff becomes blind to those terms, so admission-controller additions are never fought. Also adds annotation backfill for broker pods that pre-date the banzaicloud.com/last-applied annotation: without it the three-way merge degenerates to a two-way diff, producing false resource diffs. Both behaviours are gated on the new spec.admissionWebhooksEnabled field. 12 unit/integration tests added covering deletePreferredAffinities and the full diff path via patch.DefaultPatchMaker.Calculate. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…itionally ignorePreferredAffinities is a reconciler correctness fix, not a feature toggle. The atomic-list issue affects any external actor that touches preferredDuringSchedulingIgnoredDuringExecution, not just ScaleOps. The toleration merge already follows this precedent unconditionally. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…y current code Every pod Koperator creates goes through the creation path which calls SetLastAppliedAnnotation before r.Create. The annotation is therefore always present on pods managed by a current build; the backfill is dead code. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… param - Extract affinityKeyPod/AntiPod/Node constants to satisfy goconst - Change baseKafkaPod() to take no arguments to satisfy unparam Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d drift The preferred-affinity strip fixed one class of admission-controller churn but not the loop seen in practice: an autoscaler (ScaleOps VPA) rewrites the broker pod's resource requests after admission, koperator's diff reverts them, and the pod is deleted/recreated every reconcile. The strategic three-way merge reverts any value change to a field koperator declares (deltaMap = diff(current, modified)), so resources — and any other webhook-touched field — keep looping. The strip also silently swallowed intentional soft-affinity edits made through the CR. Replace the affinity strip with a generic, intent-aware comparison: handleRollingUpgrade now triggers a rolling upgrade iff koperator's own desired spec differs from what it last applied — the last-applied annotation vs the freshly generated desired pod (two-way strategic merge). The live pod is not consulted, so mutations by any admission controller (autoscalers, webhooks, node lifecycle controller) never cause a restart, while intentional CR edits (resources and soft affinities alike) always do, settling after one restart. - pkg/resources/kafka/util.go: add podSpecIntentChanged(); remove ignorePreferredAffinities()/deletePreferredAffinities() (subsumed) - pkg/resources/kafka/kafka.go: switch on !intentChanged instead of patchResult.IsEmpty(); isPodTainted() case unchanged and still ordered first, so the shredder park->restart flow (TaintedBrokersSelector on the live pod) is unaffected - pkg/resources/kafka/util_test.go: TestPodSpecIntentChanged (resource/affinity drift ignored; CR edits honored) and TestParkedBrokerRestartsIndependentOfIntent Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dobrerazvan
force-pushed
the
CORE-149029-fix
branch
from
August 18, 2026 09:17
f94ba9b to
f190c0b
Compare
dobrerazvan
changed the title
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
2 participants
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.
Description
Problem: Kafka broker pods restart in a loop (~8s cycle) whenever an admission controller mutates the live pod — most concretely ScaleOps VPA rewriting container requests (e.g. kafka cpu 1→392m), but also any webhook appending preferred affinity terms.
Root cause: handleRollingUpgrade used patch.DefaultPatchMaker.Calculate(currentPod, desiredPod) — a three-way strategic merge that includes the live pod in the change-detection half. Per apimachinery strategicpatch/patch.go, deltaMap = diffMaps(current, modified, {IgnoreDeletions: true}): a value change to a field koperator declares (like requests.cpu) is always reverted → non-empty patch → rolling upgrade → pod deleted → admission controller re-mutates → loop. The last-applied annotation only governs deletions; it cannot protect value changes to fields koperator still declares.
Fix — intent-aware two-way merge (podSpecIntentChanged):
Compare original (last-applied annotation on the current pod) vs modified (freshly generated desired pod). The live pod is deliberately not consulted.
This subsumes the earlier preferred-affinity strip approach (ignorePreferredAffinities) and is strictly better: the old approach also silently swallowed intentional soft-affinity edits made through the CR.
This matches how built-in controllers work — ReplicaSet, Deployment, and StatefulSet all decide from recorded intent (replica count / pod-template-hash / controller-revision-hash), never from a live-pod spec diff. koperator is a hand-rolled StatefulSet for Kafka; the bug was that it diffed the live pod instead of recorded intent.
Cleanup included:
Security note: The old live-pod diff incidentally reverted out-of-band tampering (e.g. kubectl patch image swap). The intent diff does not — consistent with RS/Deployment/StatefulSet, none of which revert live-pod drift. Tamper detection should be an explicit feature (RBAC, admission policy, image digest pinning), not coupled to the reconcile diff.
Type of Change
Checklist