fix: make the per-client DNS bound a share, and stop holding it across the ladder - #25
Merged
Merged
Conversation
…s the ladder A fixed per-client constant cannot describe a share. 64 against a global 512 means the gateway needs eight equally busy clients before the global bound can ever be the one that binds; below that the configured capacity is decorative. On the reported gateway there was effectively one DNS client, and it was refused 14,237 times against a per-client bound of 64 while 448 permits sat permanently idle and the global budget was never once reached. Derive each client's bound from the global budget instead: the budget divided by the clients currently using the forwarder, floored at 64 so a bridge full of quiet clients cannot squeeze a busy one down to nothing, and capped at the global budget, which stays the only bound on total load. `dns.max_concurrent_queries_per_client` becomes an optional fixed ceiling for operators who want one, unset by default; a config that sets it keeps exactly the behaviour it asks for. The second half of the same failure was scope. A permit was held for the whole retry ladder — two UDP attempts and a TCP fallback whose timeout applies per operation, so a failing query could occupy admission capacity for the better part of ten seconds. That made admission a measure of upstream latency rather than of concurrency, and closed the door in proportion to how slow the resolver was. A client's share is now released when the query leaves UDP for TCP, at which point it is waiting on the upstream rather than competing for admission; the global permit is held to completion and continues to bound in-flight work. Per-client accounting moves from a semaphore per client to a counter per client, since the bound now varies, with idle clients swept on an interval so they neither hold memory nor dilute the divisor. The example config also still documented the pre-#12 shape: it now shows the real global default of 512, and the per-client key it never mentioned. Refs #21
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.
Closes #21.
What was wrong
A fixed constant cannot describe a share. 64 against a global 512 means the
gateway needs eight equally busy clients before the global bound can ever be
the one that binds; below that the configured
max_concurrent_queriesisdecorative. The reported gateway had effectively one DNS client — 15,768 of
15,827 client log lines were the same container — and it was refused 14,237
times against the per-client bound while 448 permits sat permanently idle and
the global budget was never once reached:
A permit was held across the whole retry ladder. It was moved into the
worker and dropped only when the task ended — after two UDP attempts and a TCP
fallback whose timeout applies per operation, so a failing query could occupy
admission capacity for the better part of ten seconds. Admission was therefore
measuring upstream latency rather than concurrency, and closing the door in
proportion to how slow the resolver was.
What changed
currently using the forwarder, floored at 64 so a bridge full of quiet
clients cannot squeeze a busy one to nothing, and capped at the global
budget, which stays the only bound on total load. One busy client can now
reach the capacity the gateway is configured for.
dns.max_concurrent_queries_per_clientbecomes an optional fixedceiling, unset by default. A config that sets it keeps exactly the behaviour
it asks for.
past that point it is waiting on the upstream, not competing for admission.
The global permit is still held to completion and still bounds in-flight work.
client, since the bound now varies; idle clients are swept on an interval so
they neither hold memory nor dilute the divisor.
128). It now showsthe real default of 512 and documents the per-client key it never mentioned.
Operational impact
A single-tenant gateway stops refusing at an eighth of its capacity, and a slow
upstream costs latency instead of admission slots. Together with #20 this
removes the loop where refusals provoked retries that consumed the scarce
permits.
Config compatibility:
max_concurrent_queries_per_clientis still accepted andstill means a hard ceiling; omitting it (the default, and the shape every
existing config has, since the key was never in the example file) switches to
the derived share.
0remains rejected at validation.Rollback: set
dns.max_concurrent_queries_per_client: 64to pin the oldbehaviour without a redeploy of code, or revert the commit.
Validation
cargo fmt --all --check,cargo clippy --all-targets -- -D warnings,cargo test --all-targets— all clean. Five new tests cover a single clientreaching the whole global budget, the share narrowing as clients arrive and
holding at the floor, the divisor tracking clients actually using the
forwarder, a fixed ceiling overriding the share, and a query that leaves UDP
releasing its share while the global permit is still held. The existing
starvation, global-bound, and unbounded-tracking tests are unchanged in intent.