Repository navigation
Conversation
## Problem The provider used diff2.ByRecordSet and turned every change into its own correction, so each correction called CreateManyRecords, UpdateManyRecords or DeleteManyRecords with the records of a single recordset (usually one record). A zone with N changed recordsets cost N requests, all serialized through the 4 req/s rate limiter. ## Fix GetZoneRecordsCorrections now collects the changes by kind and emits one correction per bulk request: all deletes, then all updates, then all creates. Keeping deletes ahead of creates preserves diff2's guarantee that a label's old records are removed before a conflicting new record (A -> CNAME) is created. - At most 100 RRs per request (maxBatchRRs). LuaDNS documents no limit; this is a conservative guess. - A recordset is never split across requests, because UpdateManyRecords replaces everything stored for each (name, type) it receives. An oversized recordset gets a request of its own. - Updates and deletes keep the 500 retry from DNSControl#5042. Creates are still not retried, but a failed create request now fails up to 100 records instead of one recordset. - The per-request Msg is the joined per-change messages, so preview output and report counts are unchanged. ## Testing - `go test ./providers/luadns ./pkg/diff2 ./commands` passes. New tests run GetZoneRecordsCorrections against a fake LuaDNS server and check request count, endpoints, bodies, kind order, chunking, and the retry policy. - Live LuaDNS integration groups 11, 12, 15, 16, 17, 31, 32 and 35 pass. Group 38 is skipped for LUADNS by its only() filter. No live group sends more than 100 RRs in one request, so chunk splitting is covered only by the fake-server test.
TomOnTime
reviewed
Oct 28, 2026
TomOnTime
left a comment
TomOnTime
left a comment
Collaborator
There was a problem hiding this comment.
I love the performance improvement from batching requests but I do have a concern.
Diff2 very carefully orders the instructions using a much more sophisticated algorithm than simply doing deletes before creates.
I wonder if it would be better to batch runs of the same type of instruction.
For example, if the instructions are info add add add delete add add add... it could batch the two groups of adds.
Does that sound viable?
(Writing in my phone. I'm hope this all makes sense)
Review feedback on DNSControl#5047: diff2 orders changes with a dependency graph (pkg/dnssort), not just deletes before creates, so collecting all deletes, then updates, then creates could reorder them. Repointing a CNAME at a new host is the clearest case: diff2 creates the host, changes the CNAME, then deletes the old host, but the grouped version sent delete, update, create. Only consecutive changes of the same kind are now merged into one request, and a REPORT ends a run. The order that reaches the API is exactly diff2's. The 100-RR limit, recordset-never-split rule and the 500 retry policy are unchanged. Tests: the kind-grouping test is replaced by one that expects adjacent runs (create, update, delete, create, delete, create), and a new test covers the CNAME repoint order. Both failed against the grouped implementation.
Member
Author
|
I pushed the fix for that but forgot to mention it, my bad |
Collaborator
|
This makes the pager101 test a lot faster (5 seconds instead of 50 seconds). However if I enable the pager601 test, I get Please enable pager601, pager1201, batchRecordswithOthers in integrationTest/integration_test.go and make sure they all pass. |
This branch has not been deployed
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
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.
LuaDNS provider called CreateManyRecords, UpdateManyRecords, DeleteManyRecords, but didn't batch RRset changes. This PR fixes that.
I ran the normal tests, integration tests, and tried it on my real zones. I can confirm it is working as intended.