fix: add GenerationChangedPredicate to stop reconcile churn from status patches#33
Open
zachsmith1 wants to merge 3 commits intomainfrom
Open
fix: add GenerationChangedPredicate to stop reconcile churn from status patches#33zachsmith1 wants to merge 3 commits intomainfrom
zachsmith1 wants to merge 3 commits intomainfrom
Conversation
…us patches
Status patches on the upstream object were re-enqueuing the reconciler
via the For() watch because there was no predicate filtering out
status-only updates (which don't bump metadata.generation). This caused
an unnecessary reconcile cycle after every status mirror from downstream.
On the zone replicator, Owns(&DNSRecordSet{}) without a predicate also
triggered zone reconciliation whenever an owned recordset's status was
mirrored — the zone only needs to react to recordset creates, deletes,
and spec changes.
Changes:
- Add GenerationChangedPredicate to For() on all three replicator
controllers and to Owns() on the zone replicator
- Use explicit Requeue: true after metadata mutations (finalizer, owner
ref) since the predicate now filters those watch events
- Rename dnszonediscovery-replicator to dnszonediscovery (not a
replicator)
- Add status idempotency tests proving updateStatus and
ensureDownstream methods produce zero writes in steady state
…ators The zone and recordset replicators rely on the For() watch re-enqueuing after status patches to drive convergence — the downstream watch alone does not reliably deliver events in time (confirmed by e2e failures). Keep GenerationChangedPredicate only on dnszonediscovery which is one-shot and has no downstream watch dependency. The status update methods are proven idempotent by tests, so the re-enqueue from the For() watch produces a read-only no-op reconcile.
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.
Summary
GenerationChangedPredicatetoFor()watches on all three replicator controllers and toOwns()on the zone replicator, preventing status-only updates from re-enqueuing the reconcilerRequeue: trueafter metadata mutations (finalizer, owner ref) since those don't bump generation and the predicate now filters themdnszonediscovery-replicatorcontroller todnszonediscoveryupdateStatusandensureDownstream*produce zero writes in steady stateContext
Status patches on the upstream object were bouncing back through the
For()watch and re-enqueuing the reconciler, since there was no predicate to filter out status-only updates (which don't bumpmetadata.generation). Every downstream status mirror caused at least one wasted reconcile cycle. On the zone replicator,Owns(&DNSRecordSet{})without a predicate compounded this by triggering zone reconciliation on every owned recordset status mirror.Test plan
status_idempotency_test.gocovers:updateStatusno-op when upstream already matches downstream (recordset + zone)ensureDownstream*second-call idempotency (no patch on second call)SetStatusConditionstability when re-applied with differentLastTransitionTime