Feat/rework dnszone reconcile loop

[fr6nco]

Reworks reconcile loop.

For details refer to:
docs/enhancements/dnsrecordset-reconcile-consolidation.md

https://github.com/datum-cloud/dns-operator/blob/ba4bca40fce83f4f635ffe24fed2b41ee7285407/docs/enhancements/dnsrecordset-reconcile-consolidation.md

Addresses #58 #59.

[ecv]

these must be cleaned before this merges #83

[ecv]

tomáš (@fr6nco) wrote a summary separately:

garbage collection is sort of called manually, instead of relying on kube. because if the zone is gone first, we can't determine purely from the dnsrecordset the zoneclass, thus we can't reliably determine, where to clean the records up. E.g. in future, once we run multiple DNS servers (PDNS, Hicktory-DNS), we have to know where the record was originally created, and that info is stored in the Zone, not Recordset. So Recordsets have to go first, then Zone

DNSZone is no longer Controller Owner of DNSRecordset, only Ownerreference (without controller), since the subresource lifecycle is managed by the user, not the DNSZone itself (e.g. DNSRecordsets are created by the user, not by a DNSZone CR).

DNSZone status is now updated, so Accepted/Programmed is actually up to date

Status is build based on what is in PDNS, and with observed generation and resoruce ns/name naming, we can track if there were any changes. If observed generation (stored in PDNS comment) matches the resource's generation, no patches are applied to PDNs (reducing unnecessary API calls).

if a record in a recordset is deleted, we can track exactly what resources were created in PDNS, we're not guessing it from the status, what is gone. As per kubebuilder docs status is an observation/cache of the system's current state, not durable authoritative state that reconciliation fundamentally depends on.  -> in case of DR, we're losing status (edited)

[ecv]

PR #82 closes half of #58. The finalizer now waits on a successful downstream delete. It makes #59 worse: workers drop from 4 to 1, per-reconcile PowerDNS calls rise from 1 to M+1 plus a cross-zone search, and the fan-out win it is built on is worth 1.6%, because production averages 1.02 record names per record set.

Two correctness bugs outrank both issues. The error branch reports Programmed=True over records never written, and the fast path's Comments == nil check misreads PowerDNS's [], freezing all 2,690 existing production record sets permanently.

Green CI proves nothing here. Four headline mechanisms can be deleted wholesale and the suite still passes, because the fake stubs EnsureRecordSet to nil, nil.

Don't merge.

[ecv]

Issues this branch touches, and where each stands against it:

• DNSRecordSet delete can orphan the PowerDNS RRset: no finalizer on PDNS controller + dead cleanup path #58 The finalizer now waits for the downstream delete, which closes the restart gap. Owners renamed, retyped, or moved to another zone still orphan, because the delete path never reads the new OWNER comment index.
• dnsrecordset-powerdns queue saturates (p95 wait ~79s, depth ~1555): raise concurrency, cut churn, fix fresh-zone backoff #59 Fan-out per record name goes away. Worker count drops from 4 to 1, per-reconcile PowerDNS calls rise, and the concurrency knob the issue names is no longer read.
• Duplicate RecordSets #83 Ownership arbitration between colliding record sets goes away. Six owner keys in production are claimed by two record sets each.
• DNSZones and DNSRecordSets seem to be patching status too often #21 Steady state gets quieter. A record that fails to program now rewrites its status on every pass with nothing to slow it.
• PowerDNS conflicts bundled with other errors are not recognised and retry forever #76 The conflict classifier stays in the tree but the binary no longer runs it.
• An accepted DNSRecordSet edit deletes the live RRset and reports success #81 The guard against writing an empty record payload no longer runs.
• Renaming a DNSRecordSet owner name deletes the record that was just written #74 and Add coverage for the aliased-owner delete guard when an owner yields no records #77 The aliased-owner delete guard and its tests now cover a controller the binary never starts.
• DNS Operator webhook for recordsets #24 and test: assert the manager actually serves the DNSRecordSet mutating webhook #68 Untouched.

Upstream, both of these measure a record set condition rather than a resolvable name, so they would report success on this branch whether or not the record reaches PowerDNS:

• Custom-hostname DNSRecordProgrammed stuck Pending >60s on fresh zone (breaches convergence SLO) network-services-operator#291
• Gateway *.datumproxy.net address resolves in 65–85s, exceeding the 60s convergence SLO network-services-operator#292

[ecv]

Rewritten against c7e7787. My earlier version was written against ba4bca4, before @0xmc's review and the fixes in that commit, and it also carried findings that belong to #79. Those have moved to #79 (comment). What follows is only what the two commits on top of #79 introduce, and only what is still open.

Closed by c7e7787

• needsReplace now defaults to true and clears only on an exact generation match. This also settles the case I had flagged as the worst one. No deployed build has ever written an OWNER or OBSERVED_GENERATION comment, since zoneRRset on main has no Comments field at all, so every existing RRset is comment-less. Whichever way PowerDNS serialises that, an inverted default replaces rather than skips, so pre-existing records are not frozen. The old default only reached the replace branch if the key was absent rather than an empty array.
• DeleteRecordSet queries by OWNER comment after the spec-derived deletes, and returns the error rather than logging it. That closes the orphan-on-rename path.
• Both ownership mutations use Patch with MergeFrom(base).

Blockers still open

client.go:227 and :239 build error statuses with metav1.ConditionTrue. The replace path at :356 correctly uses ConditionFalse. A missing zone, a PowerDNS 5xx, a refused connection, and the multiple-RRSets case all report the record as programmed. updateStatus clears the aggregate only when a per-record condition is not True, so the top-level condition inherits it.

EnsureRecordSet still ends return statusList, nil. No PowerDNS failure produces a reconcile error, so nothing requeues and the rate limiter never engages. Combined with the above, Programmed=True no longer carries information about whether anything reached PowerDNS.

The zone fetch at :58 and the class check at :68 both sit above the deletion split at :74. A DNSRecordSet holding the finalizer whose DNSZone is gone, or whose zone class was renamed, returns before the deletion branch and hangs in Terminating. Production record sets already carry this finalizer, so it lands on upgrade rather than on new objects. This is the same pattern as the zone-side one on #79, and one reorder fixes both.

Majors

• Concurrency fell from 4 to 1. SetupWithManager passes only a RateLimiter, so MaxConcurrentReconciles takes the runtime default of 1. serverConfig.Controllers.DNSRecordSetPowerDNS lost its only reader, so setting it in an overlay is now a silent no-op.
• A failing record set hot-loops with nothing pacing it. makeProgrammedStatus stamps metav1.Now() on every call, so the reflect.DeepEqual in updateStatus is always false on error paths. That patches status, which fires a watch event on a For() source with no predicate, which reconciles again. The reconcile returns nil, so AddRateLimited never fires. Each turn costs M+1 PowerDNS calls on the single worker. The retired controller guarded this by preserving LastTransitionTime and returning early on DeepEqual(base.Status, rs.Status).
• Backoff was the load-shedding mechanism. The old path returned the PowerDNS error and climbed 1s to a 30s ceiling. Removing the error return means constant offered load against a backend that is usually unresponsive because it is saturated.
• A slow PowerDNS blocks everything. The client timeout is 10s and the M+1 calls run serially in one goroutine, with one worker. At ten records that is up to 110 seconds of a blocked worker for a single record set.
• The sweep and the delete ignore cur.Zone and act against the current zone, and both filter on cur.Type. spec.dnsZoneRef and spec.recordType are mutable, so repointing a record set or changing its type leaves the old RRset matching the OWNER search but excluded from cleanup.
• applyRRSetPatch treats 404 as fatal where DeleteZone tolerates it. If the PowerDNS zone is gone, the record set can never finalize.
• search-data runs on every reconcile, with max=9999 and no zone scope. The backend runs it as a LIKE across the whole comments table, so cost grows with total records everywhere.
• Drift is still never repaired when the generation matches. The fast path returns success without comparing record contents, so a hand edit or a restore is permanent.
• Ownership arbitration is gone. The retired reconciler aggregated by (zone, type, owner) and picked the oldest. Six keys in production are claimed by two record sets each, confirmed by parsing all 4,789 production record entries and independently corroborated by reason: NotOwner appearing on exactly those same six objects. Five are apex ALIAS, one is apex TXT on datum.net, and every pair is one zone-controller-owned object against one anchor-projected object. Staging has zero. This is Duplicate RecordSets #83.
• client.go:221 discards the ok return with ownerRRSet, _ :=. A type-mismatched entry yields an empty payload, a REPLACE with empty records[] deletes the RRset, and the code reports "Record set successfully created". This is An accepted DNSRecordSet edit deletes the live RRset and reports success #81.
• BuildOwnerRRSet no longer skips rec.Disabled. Both origin/main and feat(): support dnszone classes. DNS Operations abstraction #79 have that check.
• Kubernetes GC no longer cascades. SetControllerReference became a plain SetOwnerReference, and the zone deletion path replaces the guarantee with a list-and-delete loop that logs delete errors and returns Requeue: true.
• The zone controller reuses a stale merge base. base := zone.DeepCopy() is hoisted above the Accepted block and then used for two later status patches.

Throughput, measured

Over 7 days in production, increase(workqueue_adds_total[7d]) is 174,822 on dnsrecordset against 177,589 on dnsrecordset-powerdns, a ratio of 1.016. Mean record names per record set is 1.02, so collapsing per-name fan-out to per-object removes about 1.6% of queue items.

	drain rate	2,934-item burst
Before	4 workers at 115 ms p95, 34.8 items/s	~84 s
After	1 worker at 150-250 ms, 4-7 items/s	~7-12 min

The before-model reproduces #59's observed p95 of 78.8 s, which is what makes the projection credible.

Separately, workqueue_*{name="dnsrecordset-powerdns"} stops existing, and that is the series #59's acceptance criteria are written against.

What the green CI proves

Four mechanisms were deleted or disabled one at a time in a scratch copy. The suite stayed green on all four.

Mutation	Result
Deleted the ordered-teardown block from the zone controller	ok
Disabled the generation fast path	12/12 pdns tests pass
Stripped OWNER and OBSERVED_GENERATION from the REPLACE payload	12/12 pass
Neutered the queryDNSByComment sweep loop	12/12 pass

internal/dns/fake/client.go:81 returns nil, nil from EnsureRecordSet and records nothing, so every record set test counts calls into a spy and never checks the far side. OBSERVED_GENERATION, search-data, and Comments appear in no test file, which is also why @0xmc's request for a unit test on queryDNSByComment matters. The one testcontainers test calls none of the methods the new path uses. About 59% of the PowerDNS-path controller test lines exercise the controller cmd/main.go no longer wires.

Worth keeping: finalizer-before-programming ordering, the non-controller ownerReference, Accepted persisted before EnsureRecordSet runs, zone-delete-failure retaining the finalizer, and class filtering are all genuinely covered.

Refuted

Checked and found not to be problems:

• A REPLACE that omits comments does not wipe them. applyReplace only touches comments when the key is an array, and the Go field is omitempty.
• The comment search is exact-match. pattern2SQLPattern translates only * and ? and escapes literal _ and %, and Kubernetes names cannot contain a wildcard.
• TTL lives at spec.records[].ttl, so generation covers it.
• spec.records cannot be empty on delete, since MinItems=1.
• Zone deletion does not leave RRset orphans, because DeleteZone removes the whole zone.

Residue

• client.go logs the full request body and every fetched RRset at Info, once per write and once per owner per reconcile.
• internal/dns/handler.go accepts --controller-class-type=fake in production with no guard.
• The 204 path in applyRRSetPatch returns without closing the response body.
• dnsrecordset_downstream_controller.go ends with an orphaned doc comment for cleanupPDNSForRecordSet.
• The retired dnsrecordset_powerdns_controller.go and its 764-line test are refactored rather than deleted.
• The enhancement doc says "Does not requeue on delete errors"; the zone controller returns Requeue: true.

Measured baseline

controller_runtime_max_concurrent_reconciles reads dnsrecordset-powerdns=4 and dnsrecordset=1 in both environments today, which is the before-and-after of the concurrency change.

p99 workqueue_queue_duration_seconds on dnsrecordset-powerdns is 98.1s in production and 97.6s in staging. Near-identical across environments, so it is a standing property of the current design rather than anything #79 introduced.

Reconcile error ratios track within 0.4 points between environments on every controller, and both clusters are looping identically on status 422: RRset ... Conflicts with pre-existing RRset with a 10m requeue. That churn source predates this stack and is the live shape of #76.

Two facts that bear on the stranded-finalizer finding: every one of the 4,254 DNSZone and DNSRecordSet objects across both clusters carries the downstream finalizer, so every delete must traverse that path. And five production record sets plus three in staging currently reference DNSZones that do not exist, sitting with no Programmed condition and a message of waiting for DNSZone. Under the new early-return order those are exactly the objects that cannot finalize.

Note on evidence

The two commits on top of #79 have never run in any environment. Staging serves e5c1f94, which is #79's head, and production runs v0.6.8 from main, confirmed from the running pod images.

So the failure modes above are inferred from the code, not observed. What is measured: the object counts, the six duplicate keys, the concurrency settings, the queue and error metrics, and the universal finalizer coverage. Production currently holds 115 DNSZones and 2,707 DNSRecordSets; staging holds 38 and 1,394. No object is in Terminating in either cluster right now, and no status anywhere carries a PDNSError reason, which is consistent with the code not being deployed rather than with the findings being wrong.

One check remains unrun and would settle the comment-shape question independently of the fix in c7e7787: a read-only GET /api/v1/servers/localhost/zones/<zone>. against pdns-auth, to see whether a comment-less RRset serialises as "comments": [] or omits the key.

[ecv]

i think my claudes inferred a lot here and didnt test it

[ecv]

filing followups for observation/confirmation

[ecv]

Approving. The findings in my earlier comment are accepted risk, not resolved, and the two that fire on upgrade rather than at some later edge are worth restating here.

The error branch in EnsureRecordSet builds its status with ConditionTrue, so a PowerDNS read failure reports the record as programmed. And the fast path tests zones[0].Comments == nil, which never matches, because PowerDNS emits "comments": [] and Go decodes that to a non-nil empty slice. Every record set written before this change carries no comments, so all of them take the skip branch and stay there, including after a later spec edit.

Both are small changes. Passing ConditionFalse at the two error call sites, and defaulting needsReplace to true unless an OBSERVED_GENERATION comment matches, close them without touching the design.

Nothing here destroys data. The desired state stays in the specs and PowerDNS holds the actual state, so any orphan or stale record is recoverable by diffing the two. What is missing is anything that runs that diff: there is no sweeper in the binary and no DNS alert rule in production, so residue persists until someone looks.

[fr6nco]

Full review behind the summary above. Line references are on ba4bca4, compared against origin/main. Production figures come from the live clusters and the prod Victoria Metrics instance.

Correctness blockers

Programmed=True over a record that was never written. internal/dns/pdns/client.go:224 and :236 build the error status with metav1.ConditionTrue. The replace branch at :353 correctly uses ConditionFalse. A missing zone, a PowerDNS 5xx, a refused connection, and the "multiple RRSets" case all report the record as programmed. updateStatus only clears the aggregate when a per-record condition is not True, so the top-level condition inherits the lie.

Yes this is an issue. Will be updated to ConditionFalse.

No failure ever requeues. EnsureRecordSet returns nil unconditionally at client.go:298, and Reconcile returns ctrl.Result{}. A failed PATCH produces no error, no RequeueAfter, and no rate-limited retry. PowerDNS goes down for a minute, the write fails, and nothing re-drives it when PowerDNS comes back.

unfortunately this is left like this on purpose, as adding all the RRSets is happening in a loop. We don't want other requests to fail if one of the requests failed. It also happens on delete, so it remains orphaned until the next reconcile loop, where orphaned resources are cleaned up.

Together these two convert a latency bug into a silent correctness bug, and they take ControllerReconcileErrorRatioCritical quiet with them.

The generation fast path misreads every pre-existing record. client.go:240 gates on zones[0].Comments == nil. PowerDNS always emits "comments": [] (ws-auth.cc:693 sets the key unconditionally), and Go decodes [] into a non-nil empty slice. Every RRset written by the current production client, which writes no comments at all, takes the else branch, loops over zero comments, leaves needsReplace false, and reports success without writing.

I have to take a second look on this

This is not a one-time miss. There is no OBSERVED_GENERATION comment to ever mismatch against, so a later spec edit bumps the generation and still skips. All 2,690 production record sets freeze at their pre-upgrade values, permanently, while reporting Programmed=True. The strconv.ParseInt failure branch at :250 continues into the same trap.

Four early returns sit above their deletion branches. In dnsrecordset_downstream_controller.go, the zone fetch and its NotFound return (:57-66) and the class check (:68-71) both precede the deletion block at :85. In dnszone_downstream_controller.go, the DNSZoneClass fetch swallows NotFound at :53 and returns before the deletion block at :74.

infortunately intentional. Mentioned in my comments, that we need the zoneClass information to delete the zone.

So a missing DNSZone, a missing DNSZoneClass, or a class rename each strand a finalizer permanently. All 2,690 production record sets already carry finalize-dnsrecordset-downstream and all 112 zones carry finalize-dnszone-downstream, added by the current controller, which removed them unconditionally. These jams arrive on upgrade, not on new objects. A stuck record set blocks its zone, which blocks project teardown.

class rename should not be allowed. so intentional. Yes if a class is gone, we stuck the system. We do not reconcile on dnsclasses, but we dont touch them either, so should be fine. Eventually we can set finalizers and immutable fields to be sure. but for the current status this is ok

A 404 during delete is fatal. DeleteZone tolerates 404 as "already gone" (client.go:184); applyRRSetPatch does not (:711). If the PowerDNS zone is removed out of band, the record set can never finalize even though there is provably nothing left to clean up.

perhaps we should not delete out of zone? we should not delete zone via API outside of the controller, so I think this is fine.

The orphan story does not close

The PR adds an OWNER comment index and a search-data query (client.go:389), then wires it into EnsureRecordSet only. DeleteRecordSet (:421-444) derives owners exclusively from the current spec.Records.

So the index has no reader once the object leaves etcd. Rename an owner at 10:00, miss the sweep, delete the record set at 10:05, and the old RRset keeps resolving with nothing in Kubernetes that references it. That is #58's exact shape, reproduced through the new code path.

The sweep itself only runs while reconciling a record set that still exists, of the same type, in the same zone. If the object is gone from Kubernetes while the operator is down, which is the literal scenario in #58, no reconcile fires and the sweep never runs. Nothing scans PowerDNS for RRsets whose OWNER names a CR that no longer exists.

this was fixed in an update yesterday evening

Three further holes in the sweep:

• It ignores cur.Zone (:279-296) and deletes every search hit against the current zone. spec.dnsZoneRef is mutable, so repointing a record set leaves the old RRset matching the comment search, drawing a 422 that gets logged and dropped at :292, forever.

this was fixed in an update yesterday evening

• It filters on cur.Type (:280), and spec.recordType is also mutable. Change A to CNAME and the stale A survives both the update and the eventual delete, which is an invalid DNS state.

Agreed, in next development cycle we should make these fields immutable.

• A failed queryDNSByComment is logged and dropped at :273-277. PowerDNS's own docs note that not all backends support comment search. The single mechanism for orphan cleanup fails open, with a green status.

Aware, we have to use a backend where comments or other metadata is supported. which we do.

The enhancement doc's claim that orphans "are expected to be cleared by later reconcile activity" is not true as written. There is no sweeper in the binary.

there's already one

Convergence regressions

Out-of-band drift is never repaired. The old path issued a ReplaceRRSet on every reconcile, so drift self-healed on the next event. Now a matching generation returns recordSetSuccessStatus without reading or comparing the actual record contents (client.go:244-267). A hand edit in PowerDNS, or a database restore, is permanent, because the generation never changes.

The generation comment should gate the write, not the comparison.

Ownership arbitration is gone. The retired reconciler aggregated by (zone, type, owner) and picked a deterministic winner, oldest first. The new per-object reconciler REPLACEs the whole RRset from its own spec, and DeleteRecordSet issues an unconditional DELETE per owner, so deleting one object wipes records another still claims.

validating webhook should be added, so no duplicates can be in the system. This will solve way too many problems in the future.

Six (namespace, zone, type, owner) keys in production are claimed by two DNSRecordSets each today: five apex ALIAS records and one apex TXT on datum.net in the datum-cloud project namespace. Those need reconciling by hand or the arbitration restoring, before the new writer touches them. This is #83.

to be fixed manually

The empty-payload guard is gone. client.go:218 discards the ok return with ownerRRSet, _ :=. A type-mismatched entry still yields an empty payload, which now goes straight into ReplaceRRSet. A REPLACE with empty records[] deletes the RRset, and the code then reports "Record set successfully created". This is #81, now unguarded.

probably a use case which should never happen, but will be addressed in the update.

Throughput, measured

The fan-out fix is real and small. Over 7 days in production, increase(workqueue_adds_total[7d]) is 174,822 on dnsrecordset against 177,589 on dnsrecordset-powerdns, a ratio of 1.016. Mean record names per record set is 1.02. The N times M collapse to N is the PR's central throughput argument, and production M is essentially 1.

Everything else moves the other way:

drain rate 2,934-item burst
Before 4 workers at 115 ms p95 = 34.8 items/s ~84 s
After 1 worker at 150-250 ms = 4-7 items/s ~7-12 min
The before-model reproduces #59's observed number, 2,934 divided by 34.8 is 84 s against a reported p95 of 78.8 s, which is what makes the projection credible.

this need to be tested, I dont think the numbers will be that dramatic. I can run the reconciler with the fake client, without writes against staging/prod to see how long it will reconcile.

Contributing factors:

• SetupWithManager passes only a RateLimiter, so MaxConcurrentReconciles defaults to 1 (controller-runtime v0.22.1, pkg/controller/controller.go:244). The old path read 4 from config. dnsrecordset-powerdns queue saturates (p95 wait ~79s, depth ~1555): raise concurrency, cut churn, fix fresh-zone backoff #59 asked for roughly 32.

to be tested as well

• serverConfig.Controllers.DNSRecordSetPowerDNS is no longer read by anything. An operator following dnsrecordset-powerdns queue saturates (p95 wait ~79s, depth ~1555): raise concurrency, cut churn, fix fresh-zone backoff #59's own proposed overlay gets a silent no-op.

yes, will be deprecated if PR is stable

• Per reconcile: M GETs, up to M PATCHes, and one unconditional search-data GET with max=9999 across all zones, run even on no-op passes. That last one is a backend-wide scan whose cost grows with total records everywhere, not with the object being reconciled. Roughly 1,050 of them per hour at today's rate.

yes perhaps agreed, with this scale it should be fine, Long term plan...we should use a different backend imo, will discuss about the options later.

• The limiter is (1s, 30s). Stock is MaxOf(exponential(5ms, 1000s), bucket(10qps/100)), so the first retry is 200 times slower, and the global bucket that throttled simultaneous error bursts is gone.

Unbounded hot loop. makeProgrammedStatus stamps LastTransitionTime: metav1.Now() on every call (client.go:309). On the error paths that value changes every pass, so the reflect.DeepEqual at dnsrecordset_downstream_controller.go:209 is always false, which patches status, which produces an update event on a For() source with no predicate, which reconciles again. The reconcile returns nil, so AddRateLimited never fires and nothing throttles it. Each turn burns M+1 PowerDNS calls on the single worker, so one broken record set starves every other one. main guarded exactly this at dnsrecordset_powerdns_controller.go:281-287 and :293-295; the PR drops the guard.

needs further analysis

Smaller: statuses is sorted only in the else branch of updateStatus (:202-213), so the first write persists an unsorted list built from map iteration, and the next reconcile finds a spurious diff. One wasted write per object created.

ok good catch, fixed

Verification note. workqueue_*{name="dnsrecordset-powerdns"} stops existing, and that is the series #59's acceptance criteria are written against. No alert breaks, but any before-and-after has to be re-based onto name="dnsrecordset", which already exists with different semantics.

yeah seems like metrics , reporting will have to be updated

What the green CI proves

Four of the PR's headline mechanisms were deleted or disabled in a scratch copy. The suite stayed green on all four:

Mutation Result
Deleted the ordered-teardown block from the zone controller ok
Disabled the generation fast path (if false && ...) 12/12 pdns tests pass
Stripped OWNER and OBSERVED_GENERATION from the REPLACE payload 12/12 pass
Neutered the queryDNSByComment sweep loop 12/12 pass
Three reasons:

The fake absorbs the assertion. internal/dns/fake/client.go:81 returns nil, nil from EnsureRecordSet and records nothing; DeleteRecordSet likewise. Every record set test counts calls into a spy and never checks the far side, because on the far side there is nothing.

will run AI on that, to finish all the details

The PowerDNS wire shapes are untested. OBSERVED_GENERATION, search-data, and Comments appear in zero test files. The one real-PowerDNS test, under testcontainers, calls CreateZone, GetZone, GetZoneRRSets, and ApplyRecordSetAuthoritative, none of which the new production path uses.

Roughly 59% of the PowerDNS-path controller test lines exercise a controller cmd/main.go no longer wires. dnsrecordset_powerdns_controller_test.go is 764 lines and 9 functions against the retired reconciler, and its 191 changed lines are a mechanical fake swap with no new assertions.

The e2e suite cannot catch an orphan either. The two absence checks use [ -z "$out" ] && echo "EMPTY" || echo "NONEMPTY" (test/e2e/zones-and-records/chainsaw-test.yaml:305, test/e2e/federation/chainsaw-test.yaml:217), which exits 0 whether the record is gone or still live, so set -eu never fires. That is pre-existing, not introduced here.

The concurrent: false now added to all six suites follows from a real behaviour change: one operator instance serves exactly one class name, so the suites must share powerdns-e2e and each teardown deletes it. Those six suites were the only place concurrent multi-zone pressure on a single operator and a single PowerDNS was exercised, which is #58's bug class. That pressure is now gone.

e2e tests need work, the reconciler now requires zoneclass to be specified, so can't run it in parallel as each test cleans up the zoneclass resulting in conflicts.

Genuine coverage does exist, and is worth keeping: finalizer-before-programming ordering, the non-controller ownerReference, Accepted persisted before EnsureRecordSet runs (which asserts EnsureRecordSetCalls == 0 at each step, the strongest test in the PR), zone-delete-failure retaining the finalizer, and class filtering.

Rollout

The class gate is correct today and is not the problem. Infra builds from config/agent with no --controller-class, so the default datum-external-global-dns applies; all 112 production zones and all staging zones carry that class name; the replicator copies the spec wholesale and refuses to replicate a zone with an empty class. Nothing goes dark. The concern is that the gate is silent, and the operator's own overlays now default the e2e path to powerdns-e2e, so the two configurations have started to drift. Separately, --controller-class-type is never compared against DNSZoneClass.spec.controllerName, a check the retired controller did perform.

The ownership migration is also unthrottled. 2,685 of 2,690 records carry controller: true, and the migration block strips the controllerRef in one Update and adds the plain ownerRef in a second, each with an early return. That is roughly 5,400 full-object writes plus status patches, through one worker, with the bucket limiter removed.

ok might take a few seconds yeah

Rollback recovers ownership, since the old code's SetControllerReference upserts over the plain ref. It does not recover the comments: the old REPLACE writes no comments field, so a roll-back-then-forward wipes the OWNER and OBSERVED_GENERATION metadata and re-enters the same untested branch. Fixing the fast path to treat "no OBSERVED_GENERATION comment" as replace-required makes the round trip idempotent.

needs to be tested again and again

Observability

There is no metric, event, or condition for "skipped: class mismatch", only an unstructured logger.Info. Production vmalert carries no DNS rule group at all. Every failure mode above degrades silently until a customer reports stale DNS.

Minimum before this ships: a counter for the class-skip and for the replace-versus-fast-path decision, and rules on DNSRecordSet Programmed=False persisting and on controller_runtime_reconcile_errors_total{controller="dnsrecordset"}.

Refuted

Checked and found not to be problems, recorded so nobody chases them:

• A REPLACE that omits comments does not wipe them. applyReplace only touches comments when the key is an array, and the Go field is omitempty.
• The comment search is exact-match, not substring. pattern2SQLPattern translates only * and ?, and escapes literal _ and %, so default:foo does not match default:foobar. Kubernetes names cannot contain a wildcard, so none can be injected.

that's fine and expected

• TTL lives at spec.records[].ttl, so generation covers it. Every field the RRset is built from is under spec.
• spec.records cannot be empty on delete. MinItems=1.
• Zone deletion does not leave RRset orphans. DeleteZone removes the whole PowerDNS zone.

Residue

• client.go:705 logs the full request body at Info on every write, and :221 dumps every fetched RRset at Info once per owner per reconcile.

ok cleanups might be necessary, good for testing now

• client.go:164: EnsureZone silently no-ops on an existing zone, so nameserver changes never propagate.

yes, TODO added, never implemented before

• internal/dns/handler.go:29: --controller-class-type=fake is reachable in production and silently makes the operator a no-op against real DNS, with no guard and no warning.
• client.go:707 returns on the 204 path without closing the response body. Every REPLACE and DELETE returns 204.
• dnszone_downstream_controller.go:60-63 and :112: a nested ContainsFinalizer guard inside the !ContainsFinalizer branch, unreachable both times.

oh yes, block removed

• client.go:256: the comment reads "Observed generation matches, no need to replace" directly under needsReplace = true.
• dnsrecordset_downstream_controller.go ends with an orphaned doc comment for cleanupPDNSForRecordSet, a function that no longer exists.

clean up docs later

• BuildOwnerRRSet no longer skips rec.Disabled. Unrelated behaviour change, unmentioned.

never used yes

• config/overlays/agent-powerdns-chainsaw/ is referenced by nothing.
• The enhancement doc says "Does not requeue on delete errors"; dnszone_downstream_controller.go:96 returns Requeue: true. The doc also carries a branch-scoped status line and a ?utm_source=chatgpt.com on the kubebuilder link.
• The retired dnsrecordset_powerdns_controller.go and its 764-line test were refactored rather than deleted. If they are staying as a fallback, that intent needs writing down.

Suggested gates before merge

1. Confirm what PowerDNS actually returns for a comment-less RRset in production. If the payload carries "comments": [], the fast path freezes the estate and client.go:240 must change first. Nothing else matters until this is settled.

WILL BE TESTED!!!

2. Repoint the staging downstream semverFilter at this branch and let it reconcile all 1,394 staging record sets. Then confirm the ownership migration completed, the comments landed, and an edit to a pre-existing record set actually reaches PowerDNS.

yes, right now branch is deployed on staging in infra

3. Exercise both stranding paths against a live cluster: delete a DNSZoneClass out from under a zone, and delete a zone out from under its record sets. Both finalizers must still clear.

no...do not delete a DNSZoneClass in prod or staging PLS, do not pls do not.

4. Resolve the six colliding owner keys before the new writer touches them.

yes needs to be cleaned up

5. Land the alert rules in the same change.

[ecv]

Re-approving at f207b7b. The previous approval was dismissed by the push, and that push is an improvement: error paths now report ConditionFalse, the BuildOwnerRRSet result is checked so an empty payload cannot delete a live RRset, and the status list is sorted before the first write.

Four things are knowingly left open, tracked so they are not lost if this merges first:

• #92, the remaining half. EnsureRecordSet still returns nil regardless of outcome, so a failed write now reports honestly but nothing retries it.
• #91. The zone fetch and the class check still sit above the deletion split, in this reconciler and in the zone one.
• The status timestamp is minted on every call, so an error path still re-triggers itself through a watch with no predicate and nothing pacing it.
• MaxConcurrentReconciles is unset on the new controller, so it runs one worker where the retired one ran four.