Merged
Conversation
This was referenced Nov 26, 2024
thaJeztah
commented
Nov 26, 2024
integration/container/nat_test.go
Outdated
Comment on lines
+126
to
+128
|
|
||
| poll.WaitOn(t, container.IsInState(ctx, apiClient, id, "running"), poll.WithDelay(100*time.Millisecond)) | ||
| return id |
Member
Author
There was a problem hiding this comment.
Honestly, these are the ones I'm not 100% sure about; to my knowledge, container.Run should only return when the container is running, so I'm wondering if we actually need a wait for it to be running here 🤔
integration/daemon/daemon_test.go
Outdated
Comment on lines
+555
to
+556
| poll.WaitOn(t, container.IsInState(ctx, c, cID, "running"), poll.WithDelay(100*time.Millisecond)) | ||
|
|
Member
Author
There was a problem hiding this comment.
Same for this one (and other ones that check for running state after calling container.Run
fe1ebfb to
686cfad
Compare
thaJeztah
commented
Nov 26, 2024
| container.WithCmd("/bin/sh", "-c", "cat /hello"), | ||
| ) | ||
|
|
||
| poll.WaitOn(t, container.IsStopped(ctx, client, cid), poll.WithDelay(100*time.Millisecond)) |
Member
Author
There was a problem hiding this comment.
Looks like the default is 100 miliseconds, so we can remove this as well;
moby/vendor/gotest.tools/v3/poll/poll.go
Line 115 in d8fa058
moby/vendor/gotest.tools/v3/poll/poll.go
Lines 39 to 40 in d8fa058
686cfad to
2f49d3b
Compare
Benehiko
reviewed
Nov 27, 2024
integration/container/nat_test.go
Outdated
Comment on lines
+33
to
+39
| retry: | ||
| conn, err := net.Dial("tcp", net.JoinHostPort(endpoint.String(), "8080")) | ||
| if err != nil && try < 10 { | ||
| try++ | ||
| time.Sleep(200 * time.Millisecond) | ||
| goto retry | ||
| } |
Contributor
There was a problem hiding this comment.
I'm not really a fan of the go labels, maybe we can refactor this to be:
Suggested change
| retry: | |
| conn, err := net.Dial("tcp", net.JoinHostPort(endpoint.String(), "8080")) | |
| if err != nil && try < 10 { | |
| try++ | |
| time.Sleep(200 * time.Millisecond) | |
| goto retry | |
| } | |
| var conn *net.Conn | |
| var err error | |
| for i:=0; i < 10; i++ { | |
| conn, err = net.Dial("tcp", net.JoinHostPort(endpoint.String(), "8080")) | |
| if err == nil && conn != nil { | |
| break | |
| } | |
| time.Sleep(200 *time.Milisecond) | |
| } | |
| assert.NilError(t, err, "could not dial ...") | |
| assert.NotNil(t, conn, "connection should not be nil") |
Member
Author
There was a problem hiding this comment.
Yeah, not a fan generally as well. This was part of the original patch that I took it from. I was considering rewriting it to a poll.Check (and to use poll.WaitOn here) but chose to keep the changes as they were;
moby/vendor/gotest.tools/v3/poll/check.go
Lines 8 to 9 in f052dbb
I can have a quick try though
Member
Author
There was a problem hiding this comment.
Rewrote it to use poll.WaitOn;
diff --git a/integration/container/nat_test.go b/integration/container/nat_test.go
index 839619f8e5..5219548fc5 100644
--- a/integration/container/nat_test.go
+++ b/integration/container/nat_test.go
@@ -6,6 +6,7 @@ import (
"fmt"
"io"
"net"
+ "strconv"
"strings"
"testing"
"time"
@@ -25,20 +26,25 @@ func TestNetworkNat(t *testing.T) {
ctx := setupTest(t)
- msg := "it works"
- startServerContainer(ctx, t, msg, 8080)
+ const msg = "it works"
+ const port = 8080
+ startServerContainer(ctx, t, msg, port)
endpoint := getExternalAddress(t)
- try := 0
-retry:
- conn, err := net.Dial("tcp", net.JoinHostPort(endpoint.String(), "8080"))
- if err != nil && try < 10 {
- try++
- time.Sleep(200 * time.Millisecond)
- goto retry
- }
- assert.NilError(t, err)
- defer conn.Close()
+
+ var conn net.Conn
+ addr := net.JoinHostPort(endpoint.String(), strconv.Itoa(port))
+ poll.WaitOn(t, func(t poll.LogT) poll.Result {
+ var err error
+ conn, err = net.Dial("tcp", addr)
+ if err != nil {
+ return poll.Continue("waiting for %s to be accessible: %v", addr, err)
+ }
+ return poll.Success()
+ })
+ defer func() {
+ assert.Check(t, conn.Close())
+ }()
data, err := io.ReadAll(conn)
assert.NilError(t, err)
@@ -47,23 +53,26 @@ retry:
func TestNetworkLocalhostTCPNat(t *testing.T) {
skip.If(t, testEnv.IsRemoteDaemon)
- skip.If(t, testEnv.GitHubActions, "FIXME: https://github.com/moby/moby/issues/41561")
ctx := setupTest(t)
- msg := "hi yall"
- startServerContainer(ctx, t, msg, 8081)
-
- try := 0
-retry:
- conn, err := net.Dial("tcp", "localhost:8081")
- if err != nil && try < 10 {
- try++
- time.Sleep(200 * time.Millisecond)
- goto retry
- }
- assert.NilError(t, err)
- defer conn.Close()
+ const msg = "hi yall"
+ const port = 8081
+ startServerContainer(ctx, t, msg, port)
+
+ var conn net.Conn
+ addr := net.JoinHostPort("localhost", strconv.Itoa(port))
+ poll.WaitOn(t, func(t poll.LogT) poll.Result {
+ var err error
+ conn, err = net.Dial("tcp", addr)
+ if err != nil {
+ return poll.Continue("waiting for %s to be accessible: %v", addr, err)
+ }
+ return poll.Success()
+ })
+ defer func() {
+ assert.Check(t, conn.Close())
+ }()
data, err := io.ReadAll(conn)
assert.NilError(t, err)
diff --git a/integration/networking/port_mapping_linux_test.go b/integration/networking/port_mapping_linux_test.go
index 494f4a4ba1..ad87e7cf4a 100644
--- a/integration/networking/port_mapping_linux_test.go
+++ b/integration/networking/port_mapping_linux_test.go
@@ -25,6 +25,7 @@ import (
"gotest.tools/v3/assert"
is "gotest.tools/v3/assert/cmp"
"gotest.tools/v3/icmd"
+ "gotest.tools/v3/poll"
"gotest.tools/v3/skip"
)
@@ -254,15 +255,16 @@ func TestProxy4To6(t *testing.T) {
inspect := container.Inspect(ctx, t, c, serverId)
hostPort := inspect.NetworkSettings.Ports["80/tcp"][0].HostPort
- try := 0
-retry:
- resp, err := http.Get("http://[::1]:" + hostPort)
- if err != nil && try < 10 {
- try++
- time.Sleep(100 * time.Millisecond)
- goto retry
- }
- assert.NilError(t, err)
+ var resp *http.Response
+ addr := "http://[::1]:" + hostPort
+ poll.WaitOn(t, func(t poll.LogT) poll.Result {
+ var err error
+ resp, err = http.Get(addr)
+ if err != nil {
+ return poll.Continue("waiting for %s to be accessible: %v", addr, err)
+ }
+ return poll.Success()
+ })
assert.Check(t, is.Equal(resp.StatusCode, 404))
}
Member
Author
There was a problem hiding this comment.
Failure with this patch looks something like this;
make BIND_DIR=. TEST_FILTER='TestNetworkLocalhostTCPNat' test-integration
...
=== RUN TestNetworkLocalhostTCPNat
nat_test.go:60: timeout hit after 10s: waiting for localhost:8081 to be accessible: dial tcp [::1]:8081: connect: connection refused
--- FAIL: TestNetworkLocalhostTCPNat (10.24s)2f49d3b to
64bae63
Compare
thaJeztah
commented
Nov 27, 2024
|
|
||
| var conn net.Conn | ||
| addr := net.JoinHostPort(endpoint.String(), strconv.Itoa(port)) | ||
| poll.WaitOn(t, func(t poll.LogT) poll.Result { |
Member
Author
There was a problem hiding this comment.
FWIW; I didn't bother setting a custom interval (some of these used 200 ms) or a custom timeout (e.g. original used 10 * 200 ms = 2 seconds), so the defaults (100 ms interval and 10 seconds timeout) is used for these.
thaJeztah
commented
Nov 27, 2024
|
|
||
| func TestNetworkLocalhostTCPNat(t *testing.T) { | ||
| skip.If(t, testEnv.IsRemoteDaemon) | ||
| skip.If(t, testEnv.GitHubActions, "FIXME: https://github.com/moby/moby/issues/41561") |
Member
Author
There was a problem hiding this comment.
Removed this skip as well; I think this patch should also fix that issue;
Benehiko
approved these changes
Nov 27, 2024
Member
Author
|
😭 linter is complaining now for something silly. Guess I need a |
Cherry-picked several WIP commits from https://github.com/moby/moby/commits/b0a592798f4d9d7162f8aedca89ada3a29d60e2c/ Originally-authored-by: Rodrigo Campos <rodrigoca@microsoft.com> Co-Authored-by: Kir Kolyshkin <kolyshkin@gmail.com> Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
64bae63 to
fb6e650
Compare
Member
Author
|
note for back porting; one part doesn't apply clean in older branches, but is trivial to fix; see #48969 (comment) |
robmry
approved these changes
Nov 27, 2024
Contributor
robmry
left a comment
There was a problem hiding this comment.
Waiting for containers to be stopped before looking at their logs makes sense I think... but I'm not sure why the wait is needed for the networking tests.
Port mappings should be set up and working before the start request returns, but maybe there's some issue there - or the process in the container isn't running and listening before the request returns? I don't think packet loss should be an issue, it's unlikely to happen, and there should be retries.
It's something we should come back to, but stabilising the tests in the meantime makes sense too.
Contributor
|
I restarted this one - not changed by this PR ... test output with log |
Member
Author
|
Thanks! Ok let's bring this one in and see if CI is more stable after this |
This was referenced Nov 30, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
To see if this addresses the flaky tests mentioned there;
Cherry-picked several WIP commits from
https://github.com/moby/moby/commits/b0a592798f4d9d7162f8aedca89ada3a29d60e2c/
Originally-authored-by: Rodrigo Campos rodrigoca@microsoft.com
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)