migrate snooper tests to use new local DNS server #20615
Conversation
Most of our DNS tests were hitting google DNS (8.8.8.8). This PR runs a singleton DNS server which responds with canned responses to domains used in our tests. The test server creates a 'dummy' interface on 10.10.10.10 and binds on TCP/UDP port 53. The server code is all in the testutil/testdns package. This should avoid any packet loss causing test flakiness. The one exception is TestTracerSuite/TestDNSStatsWithNAT which still uses NAT. I couldn't figure out how to get NAT working to a dummy interface.
This reverts commit cefcfa2.
Bloop Bleep... Dogbot HereRegression Detector ResultsRun ID: 992741b1-4e7d-408f-9c1a-60324bc6475c ExplanationA regression test is an integrated performance test for Because a target's optimization goal performance in each experiment will vary somewhat each time it is run, we can only estimate mean differences in optimization goal relative to the baseline target. We express these differences as a percentage change relative to the baseline target, denoted "Δ mean %". These estimates are made to a precision that balances accuracy and cost control. We represent this precision as a 90.00% confidence interval denoted "Δ mean % CI": there is a 90.00% chance that the true value of "Δ mean %" is in that interval. We decide whether a change in performance is a "regression" -- a change worth investigating further -- if both of the following two criteria are true:
The table below, if present, lists those experiments that have experienced a statistically significant change in mean optimization goal performance between baseline and comparison SHAs with 90.00% confidence OR have been detected as newly erratic. Negative values of "Δ mean %" mean that baseline is faster, whereas positive values of "Δ mean %" mean that comparison is faster. Results that do not exhibit more than a ±5.00% change in their mean optimization goal are discarded. An experiment is erratic if its coefficient of variation is greater than 0.1. The abbreviated table will be omitted if no interesting change is observed. No interesting changes in experiment optimization goals with confidence ≥ 90.00% and |Δ mean %| ≥ 5.00%. Fine details of change detection per experiment.
|
| func GetServerIP(t *testing.T, port int) net.IP { | ||
| if port != 53 { | ||
| non53ServerOnce.Do(func() { | ||
| globalServer.Start("tcp", port) | ||
| globalServer.Start("udp", port) | ||
| }) | ||
| } | ||
| serverOnce.Do(func() { | ||
| globalServer, globalServerError = newServer() | ||
| globalServer.Start("tcp") | ||
| globalServer.Start("udp") | ||
| globalServer.Start("tcp", port) | ||
| globalServer.Start("udp", port) | ||
| }) | ||
|
|
||
| require.NoError(t, globalServerError) | ||
| return net.ParseIP("127.0.0.1") | ||
| return net.ParseIP(localhostAddr) | ||
| } |
There was a problem hiding this comment.
I think this could be cleaner if GetServerIP had it's original signature (taking a *testing.T) and started both the non-port-53 server and the port-53 server.
There was a problem hiding this comment.
yeah that's fine... I think the only thing there is that the non-53 server will be started always, when it's only used in one test
akarpz
added
changelog/no-changelog
team/networks
[deprecated] qa/skip-qa - use other qa/ labels
pkg/network/dns/snooper_test.go
Outdated
| h := &handler{} | ||
| shutdown, port := newTestServer(t, localhost, 0, "udp", h.ServeDNS) | ||
| defer shutdown() | ||
| ln, _ := net.Listen("tcp", "127.0.0.1:0") |
There was a problem hiding this comment.
For the cases where we need a random port, I think we should just spin up a server and shut it down in the test.
We can also pass in 0 for the port for the server spin up code to pick a port.
There was a problem hiding this comment.
that is what we were doing before... I think the idea here was to centralize the server code. but I was on the fence when doing this, wdyt @leeavital
There was a problem hiding this comment.
I don't have strong feelings, if we prefer to have a server-per-test, I'm fine with it. My thinking was it was nice to have a single global test server because:
- tests run faster when they don't each have to spin up a server
- tests don't have to deal with managing the lifecycle of
- the nature of a stubbed server makes having a global easy
- (obsolete) it was unlikely for the global server to conflict with other test fixtures since it listened on a nonsense ip (10.10.10.10)
There was a problem hiding this comment.
I was only proposing a local server for this test where we don't use port 53. The other tests can use the global server. In addition, the server creation/run code seems to be using the same var, globalServer, for both servers, which is likely going to be brittle.
| func GetServerIP(t *testing.T, port int) net.IP { | ||
| if port != 53 { | ||
| non53ServerOnce.Do(func() { | ||
| globalServer.Start("tcp", port) |
There was a problem hiding this comment.
How does this work if the dns server with port 53 is already running since you're using the same globalServer variable for both?
There was a problem hiding this comment.
The constructor newServer() used to take care of creating interfaces, and no longer does. We can dispense with storing the server in a global variable completely now that the server uses loopback
| // GetServerIP returns the IP address of the test DNS server. The test DNS server returns canned responses for several | ||
| // known domains that are used in integration tests. | ||
| // | ||
| // see server#start to see which domains are handled. | ||
| func GetServerIP(t *testing.T) net.IP { | ||
| var err error |
There was a problem hiding this comment.
What will happen if:
test #1calls GetServerIP and the sync.Once function failstest #2calls GetServerIP
?
There was a problem hiding this comment.
I see what you mean. I think we need to keep the globalError and have it set by the call to ListenAndServe()
akarpz
dismissed stale reviews from leeavital and hmahmood
The base branch was changed.
What does this PR do?
Motivation
https://datadoghq.atlassian.net/browse/NPM-3012
https://datadoghq.atlassian.net/browse/NPM-3071
https://datadoghq.atlassian.net/browse/NPM-3044
Additional Notes
Possible Drawbacks / Trade-offs
Describe how to test/QA your changes
Reviewer's Checklist
Triagemilestone is set.major_changelabel if your change either has a major impact on the code base, is impacting multiple teams or is changing important well-established internals of the Agent. This label will be use during QA to make sure each team pay extra attention to the changed behavior. For any customer facing change use a releasenote.changelog/no-changeloglabel has been applied.qa/skip-qalabel is not applied.team/..label has been applied, indicating the team(s) that should QA this change.need-change/operatorandneed-change/helmlabels have been applied.k8s/<min-version>label, indicating the lowest Kubernetes version compatible with this feature.