Skip to content

Implement random resolver option - #467

Open
dontcrash wants to merge 5 commits into
AdguardTeam:masterfrom
dontcrash:master
Open

Implement random resolver option#467
dontcrash wants to merge 5 commits into
AdguardTeam:masterfrom
dontcrash:master

Conversation

@dontcrash

Copy link
Copy Markdown

Implements the option for a random resolver to go alongside this PR AdguardTeam/AdGuardHome#7950

Hopefully resolves AdguardTeam/AdGuardHome#4756

@windsurf-bot windsurf-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other comments (1)
  • proxy/upstreammode.go (27-27) The comment for `UpstreamModeRandom` is missing a verb. Consider changing to "makes server to use a random upstream" or "makes server to select a random upstream".

💡 To request another review, post a new comment with "/windsurf-review".

Comment thread proxy/config.go Outdated
UpstreamModeLoadBalance,
UpstreamModeParallel:
UpstreamModeParallel,
UpstreamModeRandom:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR adds UpstreamModeRandom to the switch statement, but this constant isn't defined anywhere in the visible code. You need to define this constant alongside the other UpstreamMode constants (like UpstreamModeFastestAddr, UpstreamModeLoadBalance, etc.).

@dontcrash dontcrash Jul 25, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one I believe is fine as it is inherited from AdGuardHome?
Although I realised the switch case is wrong, fixed it up

UpstreamModeFastestAddr,
UpstreamModeLoadBalance,
UpstreamModeRandom,
UpstreamModeParallel:
// Go on.

Comment thread proxy/upstreammode.go Outdated
case
UpstreamModeLoadBalance,
UpstreamModeParallel,
UpstreamModeRandomm,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a typo in the variable name: UpstreamModeRandomm should be UpstreamModeRandom.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Facepalm.

Comment thread proxy/exchange.go Outdated
dontcrash and others added 3 commits July 25, 2025 20:39
Co-authored-by: windsurf-bot[bot] <189301087+windsurf-bot[bot]@users.noreply.github.com>
@ainar-g
ainar-g self-requested a review July 29, 2025 15:35
@ainar-g ainar-g self-assigned this Jul 29, 2025
@ainar-g

ainar-g commented Jul 29, 2025

Copy link
Copy Markdown
Collaborator

Thanks for the contribution! We'll review it shortly.

Comment thread internal/cmd/args.go
},
upstreamModeIdx: {
description: "Defines the upstreams logic mode, possible values: load_balance, parallel, " +
description: "Defines the upstreams logic mode, possible values: load_balance, parallel, random, " +

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update README.md as well.

Comment thread proxy/exchange.go
// Simply set ups to a random entry in ups
// Falls back to default behaviour as if ups <= 1 it will run UpstreamModeLoadBalance as intended
if len(ups) > 1 {
ups = []upstream.Upstream{ups[p.randSrc.Intn(len(ups))]}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't build:

proxy/exchange.go:28:44: p.randSrc.Intn undefined (type "math/rand/v2".Source has no field or method Intn)

See go doc math/rand/v2.

Comment thread proxy/upstreammode.go
// response time or TCP connection time.
UpstreamModeFastestAddr UpstreamMode = "fastest_addr"

// UpstreamModeRandom makes server to a random upstream

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, consider using gofumpt to format the code in this repository:

  1. make go-tools
  2. ./bin/gofumpt --extra -w .

@Sil3ntVip3r

Copy link
Copy Markdown

I ported this feature onto current dnsproxy master (acf2b30e05202e171bbbaf4a3176ea07edb964b3) in an isolated worktree and completed the missing implementation and test coverage. No commit or push was made.

The smallest current-master implementation is:

  • add UpstreamModeRandom = "random" to parsing and configuration validation;
  • reuse the existing sampleuv.Weighted sampler with equal weights in random mode; this produces a uniform random permutation and retries failed upstreams without replacement;
  • retain RTT-derived weights unchanged for load_balance;
  • preserve fallback and query-statistics accounting;
  • document the new CLI value in both internal/cmd/args.go and generated README.md.

Using the existing sampler is important here: it avoids the reported rand/v2.Source.Intn build failure and preserves the established retry path rather than selecting one upstream and discarding the rest.

I added deterministic tests for enum encoding, configuration validation, uniform choice despite six-orders-of-magnitude RTT differences, retrying every failed upstream exactly once, fallback, and statistics. Verification on the patched current-master tree passed focused and repeated race tests, the full race suite, go vet, make go-check, build, Markdown/text lint, formatting/diff checks, and Darwin, FreeBSD, OpenBSD, Linux, and Windows Go checks. An independent adversarial review found no remaining source blocker.

This also addresses the existing review requests to update README.md, make the code build with math/rand/v2, and format it with the repository toolchain. The companion AdGuard Home port still needs an accepted dnsproxy commit or release before its go.mod/go.sum can be finalized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a "Random" option to the DNS settings

4 participants