From a3049653c17f1e4b1c319272413fb3306cbd7848 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Wed, 5 Jul 2023 12:09:37 -0400 Subject: [PATCH 1/2] pkg/plugins: make unit test less time sensitive TestClientWithRequestTimeout has been observed to flake in CI. The timing in the test is quite tight, only giving the client a 10ms window to time out, which could potentially be missed if the host is under load and the goroutine scheduling is unlucky. Give the client a full five seconds of grace to time out before failing the test. Signed-off-by: Cory Snider (cherry picked from commit 9cee34bc94fb5c78a8a79a0b36118d13f27f2f8b) Signed-off-by: Cory Snider --- pkg/plugins/client_test.go | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/pkg/plugins/client_test.go b/pkg/plugins/client_test.go index f93734d367..f60d0cdb6b 100644 --- a/pkg/plugins/client_test.go +++ b/pkg/plugins/client_test.go @@ -3,6 +3,7 @@ package plugins // import "github.com/docker/docker/pkg/plugins" import ( "bytes" "encoding/json" + "errors" "io" "net/http" "net/http/httptest" @@ -13,7 +14,6 @@ import ( "github.com/docker/docker/pkg/plugins/transport" "github.com/docker/go-connections/tlsconfig" - "github.com/pkg/errors" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" ) @@ -240,22 +240,39 @@ func TestClientWithRequestTimeout(t *testing.T) { Timeout() bool } - timeout := 1 * time.Millisecond + unblock := make(chan struct{}) testHandler := func(w http.ResponseWriter, r *http.Request) { - time.Sleep(timeout + 10*time.Millisecond) + select { + case <-unblock: + case <-r.Context().Done(): + } w.WriteHeader(http.StatusOK) } srv := httptest.NewServer(http.HandlerFunc(testHandler)) - defer srv.Close() + defer func() { + close(unblock) + srv.Close() + }() client := &Client{http: srv.Client(), requestFactory: &testRequestWrapper{srv}} - _, err := client.callWithRetry("/Plugin.Hello", nil, false, WithRequestTimeout(timeout)) - assert.Assert(t, is.ErrorContains(err, ""), "expected error") + errCh := make(chan error, 1) + go func() { + _, err := client.callWithRetry("/Plugin.Hello", nil, false, WithRequestTimeout(time.Millisecond)) + errCh <- err + }() - var tErr timeoutError - assert.Assert(t, errors.As(err, &tErr)) - assert.Assert(t, tErr.Timeout()) + timer := time.NewTimer(5 * time.Second) + defer timer.Stop() + select { + case err := <-errCh: + var tErr timeoutError + if assert.Check(t, errors.As(err, &tErr), "want timeout error, got %T", err) { + assert.Check(t, tErr.Timeout()) + } + case <-timer.C: + t.Fatal("client request did not time out in time") + } } type testRequestWrapper struct { From 0e88c57c470651400d677544b2681d44b8b14525 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Wed, 5 Jul 2023 13:49:53 -0400 Subject: [PATCH 2/2] integration: disable iptables in parallel tests Multiple daemons starting/running concurrently can collide with each other when editing iptables rules. Most integration tests which opt into parallelism and start daemons work around this problem by starting the daemon with the --iptables=false option. However, some of the tests neglect to pass the option when starting or restarting the daemon, resulting in those tests being flaky. Audit the integration tests which call t.Parallel() and (*Daemon).Stop() and add --iptables=false arguments where needed. Signed-off-by: Cory Snider (cherry picked from commit cdcb7c28c5f6d29652fa9d37dc45041b190d1cd4) Signed-off-by: Cory Snider --- integration/container/daemon_linux_test.go | 8 ++++---- integration/container/daemon_test.go | 2 +- integration/image/import_test.go | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/integration/container/daemon_linux_test.go b/integration/container/daemon_linux_test.go index d1d6c61a9d..b67dece2c9 100644 --- a/integration/container/daemon_linux_test.go +++ b/integration/container/daemon_linux_test.go @@ -140,7 +140,7 @@ func TestDaemonHostGatewayIP(t *testing.T) { // Verify the IP in /etc/hosts is same as host-gateway-ip d := daemon.New(t) // Verify the IP in /etc/hosts is same as the default bridge's IP - d.StartWithBusybox(t) + d.StartWithBusybox(t, "--iptables=false") c := d.NewClientT(t) ctx := context.Background() cID := container.Run(ctx, t, c, @@ -157,7 +157,7 @@ func TestDaemonHostGatewayIP(t *testing.T) { d.Stop(t) // Verify the IP in /etc/hosts is same as host-gateway-ip - d.StartWithBusybox(t, "--host-gateway-ip=6.7.8.9") + d.StartWithBusybox(t, "--iptables=false", "--host-gateway-ip=6.7.8.9") cID = container.Run(ctx, t, c, container.WithExtraHost("host.docker.internal:host-gateway"), ) @@ -208,7 +208,7 @@ func TestRestartDaemonWithRestartingContainer(t *testing.T) { c.HasBeenStartedBefore = true }) - d.Start(t) + d.Start(t, "--iptables=false") ctxTimeout, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() @@ -256,7 +256,7 @@ func TestHardRestartWhenContainerIsRunning(t *testing.T) { }) } - d.Start(t) + d.Start(t, "--iptables=false") t.Run("RestartPolicy=none", func(t *testing.T) { ctx, cancel := context.WithTimeout(ctx, 5*time.Second) diff --git a/integration/container/daemon_test.go b/integration/container/daemon_test.go index 94468a4409..a0a66b7247 100644 --- a/integration/container/daemon_test.go +++ b/integration/container/daemon_test.go @@ -43,7 +43,7 @@ func TestContainerKillOnDaemonStart(t *testing.T) { assert.Assert(t, inspect.State.Running) assert.NilError(t, d.Kill()) - d.Start(t) + d.Start(t, "--iptables=false") inspect, err = client.ContainerInspect(ctx, id) assert.Check(t, is.Nil(err)) diff --git a/integration/image/import_test.go b/integration/image/import_test.go index 110ab87a5f..9ee647867a 100644 --- a/integration/image/import_test.go +++ b/integration/image/import_test.go @@ -27,7 +27,7 @@ func TestImportExtremelyLargeImageWorks(t *testing.T) { // Spin up a new daemon, so that we can run this test in parallel (it's a slow test) d := daemon.New(t) - d.Start(t) + d.Start(t, "--iptables=false") defer d.Stop(t) client := d.NewClientT(t)