Fix flaky dns test #6923

inteon · 2024-04-20T20:28:23Z

Example flake: https://prow.infra.cert-manager.io/view/gs/cert-manager-prow-artifacts/logs/ci-cert-manager-master-make-test/1781654108378238976

Master:

$ go test -count=100 -timeout 30s -run ^TestReachabilityCustomDnsServers$ github.com/cert-manager/cert-manager/pkg/issuer/acme/http
--- FAIL: TestReachabilityCustomDnsServers (0.12s)
    http_test.go:189: Unexpected error: failed to perform self check GET request 'https://cert-manager.io': Get "https://cert-manager.io": dial tcp: lookup cert-manager.io on 127.0.0.53:53: read udp 127.0.0.1:54187->127.0.0.1:15353: read: connection refused
--- FAIL: TestReachabilityCustomDnsServers (0.11s)
    http_test.go:189: Unexpected error: failed to perform self check GET request 'https://cert-manager.io': Get "https://cert-manager.io": dial tcp: lookup cert-manager.io on 127.0.0.53:53: read udp 127.0.0.1:43837->127.0.0.1:15353: read: connection refused
--- FAIL: TestReachabilityCustomDnsServers (0.11s)
    http_test.go:189: Unexpected error: failed to perform self check GET request 'https://cert-manager.io': Get "https://cert-manager.io": dial tcp: lookup cert-manager.io on 127.0.0.53:53: read udp 127.0.0.1:50259->127.0.0.1:15353: read: connection refused
--- FAIL: TestReachabilityCustomDnsServers (0.11s)
    http_test.go:189: Unexpected error: failed to perform self check GET request 'https://cert-manager.io': Get "https://cert-manager.io": dial tcp: lookup cert-manager.io on 127.0.0.53:53: read udp 127.0.0.1:35645->127.0.0.1:15353: read: connection refused
--- FAIL: TestReachabilityCustomDnsServers (0.11s)
    http_test.go:189: Unexpected error: failed to perform self check GET request 'https://cert-manager.io': Get "https://cert-manager.io": dial tcp: lookup cert-manager.io on 127.0.0.53:53: read udp 127.0.0.1:46554->127.0.0.1:15353: read: connection refused
FAIL
FAIL    github.com/cert-manager/cert-manager/pkg/issuer/acme/http       20.767s
FAIL

This PR:

$ go test -count=100 -timeout 30s -run ^TestReachabilityCustomDnsServers$ github.com/cert-manager/cert-manager/pkg/issuer/acme/http
ok      github.com/cert-manager/cert-manager/pkg/issuer/acme/http       21.122s

Kind

/kind flake

Release Note

NONE

inteon · 2024-04-20T20:28:51Z

pkg/issuer/acme/http/http_test.go

-	go server.ListenAndServe()
+
+	go func() {
+		if err := server.ListenAndServe(); err != nil {


Unrelated change: don't silently fail ListenAndServe, fail test instead.

inteon · 2024-04-22T12:05:54Z

/cherrypick release-1.12

cert-manager-bot · 2024-04-22T12:05:57Z

@inteon: once the present PR merges, I will cherry-pick it on top of release-1.12 in a new PR and assign it to you.

In response to this:

/cherrypick release-1.12

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

inteon · 2024-04-22T12:05:58Z

/cherrypick release-1.13

cert-manager-bot · 2024-04-22T12:06:00Z

@inteon: once the present PR merges, I will cherry-pick it on top of release-1.13 in a new PR and assign it to you.

In response to this:

/cherrypick release-1.13

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

inteon · 2024-04-22T12:06:03Z

/cherrypick release-1.14

cert-manager-bot · 2024-04-22T12:06:05Z

@inteon: once the present PR merges, I will cherry-pick it on top of release-1.14 in a new PR and assign it to you.

In response to this:

/cherrypick release-1.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ThatsMrTalbot

Can you explain how using our own Mux fixes things compared to the DefaultServeMux?

…equests Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon · 2024-04-22T13:00:58Z

pkg/issuer/acme/http/http_test.go

 	dnsServerCalled := int32(0)

-	server := &dns.Server{Addr: "127.0.0.1:15353", Net: "udp"}
+	server := &dns.Server{Addr: "127.0.0.1:15353", Net: "udp", NotifyStartedFunc: func() { close(dnsServerStarted) }}


We received a read: connection refused error when the test started before the DNS server was started.

inteon · 2024-04-22T13:02:41Z

pkg/issuer/acme/http/http_test.go

 	defer server.Shutdown()

-	dns.HandleFunc(".", func(w dns.ResponseWriter, r *dns.Msg) {
+	mux := &dns.ServeMux{}


We create a new mux here instead of using the default/ global mux, so that we cannot interfere with other mux registrations (in other tests or when running this test in parallel).

I don't think we can run in parallel since we use a fixed port for the DNS server, but I can see how we would want to reset the state between tests.

inteon · 2024-04-22T13:03:49Z

Can you explain how using our own Mux fixes things compared to the DefaultServeMux?

Thanks for the comment, I actually hadn't fixed the test completely yet.
I now fixed the flake and added a proof in the description.
I also added a few code comments to explain why the changes were made.

ThatsMrTalbot · 2024-04-22T13:35:40Z

Ah, so the main fix was the new channel we wait on to ensure the DNS server is up and running. That makes sense to me.
/lgtm

inteon · 2024-04-22T13:43:44Z

/retest

inteon · 2024-04-22T14:01:16Z

/approve

cert-manager-prow · 2024-04-22T14:01:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [inteon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cert-manager-bot · 2024-04-22T14:14:46Z

@inteon: new pull request created: #6934

In response to this:

/cherrypick release-1.12

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cert-manager-bot · 2024-04-22T14:15:23Z

@inteon: new pull request created: #6935

In response to this:

/cherrypick release-1.13

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cert-manager-bot · 2024-04-22T14:16:00Z

@inteon: new pull request created: #6936

In response to this:

/cherrypick release-1.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

inteon commented Apr 20, 2024

View reviewed changes

ThatsMrTalbot reviewed Apr 22, 2024

View reviewed changes

inteon marked this pull request as draft April 22, 2024 12:45

cert-manager-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 22, 2024

fix flaky dns test, make sure dns server has started before sending r…

0f69abd

…equests Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon force-pushed the fix_dns_test_flake branch from 1682d49 to 0f69abd Compare April 22, 2024 12:54

inteon marked this pull request as ready for review April 22, 2024 12:58

cert-manager-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 22, 2024

inteon commented Apr 22, 2024

View reviewed changes

cert-manager-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2024

cert-manager-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2024

cert-manager-prow bot merged commit 2ca75cc into cert-manager:master Apr 22, 2024
6 checks passed

cert-manager-bot mentioned this pull request Apr 22, 2024

[release-1.12] Fix flaky dns test #6934

Merged

This was referenced Apr 22, 2024

[release-1.13] Fix flaky dns test #6935

Merged

[release-1.14] Fix flaky dns test #6936

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix flaky dns test #6923

Fix flaky dns test #6923

inteon commented Apr 20, 2024 •

edited

inteon Apr 20, 2024

inteon commented Apr 22, 2024

cert-manager-bot commented Apr 22, 2024

inteon commented Apr 22, 2024

cert-manager-bot commented Apr 22, 2024

inteon commented Apr 22, 2024

cert-manager-bot commented Apr 22, 2024

ThatsMrTalbot left a comment

inteon Apr 22, 2024

inteon Apr 22, 2024

ThatsMrTalbot Apr 22, 2024

inteon commented Apr 22, 2024

ThatsMrTalbot commented Apr 22, 2024

inteon commented Apr 22, 2024

inteon commented Apr 22, 2024

cert-manager-prow bot commented Apr 22, 2024

cert-manager-bot commented Apr 22, 2024

cert-manager-bot commented Apr 22, 2024

cert-manager-bot commented Apr 22, 2024

Fix flaky dns test #6923

Fix flaky dns test #6923

Conversation

inteon commented Apr 20, 2024 • edited

Kind

Release Note

inteon Apr 20, 2024

Choose a reason for hiding this comment

inteon commented Apr 22, 2024

cert-manager-bot commented Apr 22, 2024

inteon commented Apr 22, 2024

cert-manager-bot commented Apr 22, 2024

inteon commented Apr 22, 2024

cert-manager-bot commented Apr 22, 2024

ThatsMrTalbot left a comment

Choose a reason for hiding this comment

inteon Apr 22, 2024

Choose a reason for hiding this comment

inteon Apr 22, 2024

Choose a reason for hiding this comment

ThatsMrTalbot Apr 22, 2024

Choose a reason for hiding this comment

inteon commented Apr 22, 2024

ThatsMrTalbot commented Apr 22, 2024

inteon commented Apr 22, 2024

inteon commented Apr 22, 2024

cert-manager-prow bot commented Apr 22, 2024

cert-manager-bot commented Apr 22, 2024

cert-manager-bot commented Apr 22, 2024

cert-manager-bot commented Apr 22, 2024

inteon commented Apr 20, 2024 •

edited