Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: TestUpdateResolvConf fails since https://github.com/golang/go/commit/5efbdd9d10908206d4e0351cb4724c5fefdfa2be #14437

Closed
mikioh opened this issue Feb 21, 2016 · 3 comments
Milestone

Comments

@mikioh
Copy link
Contributor

mikioh commented Feb 21, 2016

For example,

go test -v -run=TestUpdateResolvConf
=== RUN   TestUpdateResolvConf
--- FAIL: TestUpdateResolvConf (0.39s)
    dnsclient_unix_test.go:208: lookup golang.org on 192.168.0.1:53: server misbehaving
    dnsclient_unix_test.go:208: lookup golang.org on 192.168.0.1:53: server misbehaving
    dnsclient_unix_test.go:208: lookup golang.org on 192.168.0.1:53: server misbehaving
    dnsclient_unix_test.go:208: lookup golang.org on 192.168.0.1:53: server misbehaving
    dnsclient_unix_test.go:221: #0: got [192.168.0.1]; want [8.8.8.8]
    dnsclient_unix_test.go:208: lookup www.example.com on 192.168.0.1:53: server misbehaving
    dnsclient_unix_test.go:208: lookup www.example.com on 192.168.0.1:53: server misbehaving
    dnsclient_unix_test.go:221: #2: got [192.168.0.1]; want [8.8.4.4]
FAIL

/cc @mdempsky

@mikioh mikioh added this to the Go1.7 milestone Feb 21, 2016
@mikioh mikioh changed the title net: TestUpdateResolvConf fails since 5efbdd9d10908206d4e0351cb4724c5fefdfa2be net: TestUpdateResolvConf fails since https://github.com/golang/go/commit/5efbdd9d10908206d4e0351cb4724c5fefdfa2be Feb 21, 2016
@mdempsky mdempsky self-assigned this Feb 21, 2016
@mdempsky
Copy link
Member

The problem as far as I can tell it is TestUpdateResolvConf uses resolvConfTest to write and load a /tmp/foo/resolv.conf file like "nameserver 8.8.8.8\n". However, the actual DNS resolver code still checks /etc/resolv.conf.

The logic in forceUpdate intentionally zeros out lastChecked (which only limits os.Stat calls), but previously it left conf.mtime untouched. So the next DNS client call would call os.Stat("/etc/resolv.conf") and see it (most likely) still matches conf.mtime.

However, now that conf.mtime has been moved to conf.dnsConfig.mtime, the tryUpdate("/etc/resolv.conf") calls are noticing that /etc/resolv.conf's mtime does not match /tmp/foo/resolv.conf, and reloading the file.

The quick fix to restore the previous behavior would be changing forceUpdate to something like:

    conf.mu.Lock()
    dnsConf.mtime = conf.dnsConfig.mtime
    conf.dnsConfig = dnsConf
    conf.mu.Unlock()

However, that feels hacky to me. I'm thinking writeAndUpdate should actually set lastChecked to far in the future to avoid it being reloaded, and then only teardown needs to zero out lastChecked.

@mikioh
Copy link
Contributor Author

mikioh commented Feb 21, 2016

SGTM to modify the test helpers.

@gopherbot
Copy link

CL https://golang.org/cl/19777 mentions this issue.

@golang golang locked and limited conversation to collaborators Feb 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants