Conversation
Cherry-picked several WIP commits from https://github.com/moby/moby/commits/b0a592798f4d9d7162f8aedca89ada3a29d60e2c/ Originally-authored-by: Rodrigo Campos <rodrigoca@microsoft.com> Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
There was a problem hiding this comment.
@AkihiroSuda thanks for moving this! Left some comments. Probably we need to test some things to be sure they are needed.
Also, I didn't tackle the tests in docker-py, that also were failing and they are not waiting for the container. But it might be worth doing too (can be another PR :))
| poll.WaitOn(t, container.IsStopped(ctx, clientNoUserRemap, cid), poll.WithDelay(100*time.Millisecond)) | ||
| logReader, err := clientNoUserRemap.ContainerLogs(ctx, cid, containertypes.LogsOptions{ | ||
| ShowStdout: true, | ||
| ShowStderr: true, |
There was a problem hiding this comment.
We don't need this. I did it in the WIP to see if it printed something else, but I think we can drop it
| var err error | ||
| var conn net.Conn | ||
| for retry := 0; retry < 10; retry++ { | ||
| conn, err = net.Dial("tcp", net.JoinHostPort(endpoint.String(), "8080")) | ||
| if err == nil { | ||
| break | ||
| } | ||
| time.Sleep(200 * time.Millisecond) | ||
| } |
There was a problem hiding this comment.
I think these fixes didn't make much of a difference and I ended up skipping the test, as there was an issue open for 4 years and some other tests were skipped.
I pushed the branch here: https://github.com/rata/moby/tree/runc-1.2-test-fixes. See the last commit, that added a SKIP and mentions the issue. There are other test in this file that use it too.
Maybe we can just drop the retries in this file, just keep the chunk about waiting for the container to be running?
| var err error | ||
| var resp *http.Response | ||
| for retry := 0; retry < 10; retry++ { | ||
| resp, err = http.Get("http://[::1]:" + hostPort) | ||
| if err == nil { | ||
| break | ||
| } | ||
| time.Sleep(100 * time.Millisecond) |
There was a problem hiding this comment.
I'm unsure if this was useful too. I think maybe it is, but I can't remember now. We should test it, probably.
| poll.WaitOn(t, container.IsStopped(ctx, client, cid), poll.WithDelay(100*time.Millisecond)) | ||
| reader, err := client.ContainerLogs(ctx, cid, containertypes.LogsOptions{ | ||
| ShowStdout: true, | ||
| ShowStderr: true, |
There was a problem hiding this comment.
Same as cb4b816#r1809141990, I guess, i.e. this is not needed.
| resp, err := http.Get("http://[::1]:" + hostPort) | ||
| var err error | ||
| var resp *http.Response |
There was a problem hiding this comment.
resp and error a redefined here.
Maybe do something like this:
resp, err := http.Get("http://[::1]:" + hostPort)
for retry := 0; retry < 10; retry++ {
if err == nil {
break
}
time.Sleep(100 * time.Millisecond)
resp, err = http.Get("http://[::1]:" + hostPort)
}|
I am carrying this one (with some fixes and other minor changes) in #48160. |
Thanks |
Cherry-picked several WIP commits from
https://github.com/moby/moby/commits/b0a592798f4d9d7162f8aedca89ada3a29d60e2c/
For the sake of deflaking the tests:
Originally-authored-by: @rata