TestPostContainersAttach: minor improvements #36612

kolyshkin · 2018-03-16T03:29:34Z

The API part of this test (the last test case in TestPostContainerAttach) is flaky, i.e. it sometimes fails on powerpc or s390 like this:

> FAIL: docker_api_attach_test.go:98: DockerSuite.TestPostContainersAttach
> docker_api_attach_test.go:211:
>     c.Assert(actualStdout.Bytes(), checker.DeepEquals, []byte("hello\nsuccess"), check.Commentf("Attach didn't return the expected data from stdout"))
> ... obtained []uint8 = []byte{0x73, 0x75, 0x63, 0x63, 0x65, 0x73, 0x73}
> ... expected []uint8 = []byte{0x68, 0x65, 0x6c, 0x6c, 0x6f, 0xa, 0x73, 0x75, 0x63, 0x63, 0x65, 0x73, 0x73}
> ... Attach didn't return the expected data from stdout

Is seems that "success" is returned but "hello\n" is not (as if ContainerAttachOptions.Logs was false).

After a careful code audit I still can't figure out why it fails. The two major differences from the other cases are:

it uses API (ContainerAttach()) rather than sockRequestHijack();
it uses stdcopy.StdCopy() not io.ReadFull(),

Nevertheless it looks almost legit, except for ignoring the error from StdCopy. So I don't know how to fix it. Still, there are some things that can be improved here, including:

Get the container's stderr as well, make sure it's empty.
Check that stdcopy.StdCopy() did not return an error.
Use strings, so the test output will be more readable.
Nitpicks; move/remove comments, simplify var names.

This should ultimately fix #36611

codecov · 2018-03-16T06:58:40Z

Codecov Report

❗ No coverage uploaded for pull request base (master@185ae7e). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #36612   +/-   ##
=========================================
  Coverage          ?   35.08%           
=========================================
  Files             ?      615           
  Lines             ?    45780           
  Branches          ?        0           
=========================================
  Hits              ?    16062           
  Misses            ?    27611           
  Partials          ?     2107

thaJeztah · 2018-03-16T10:59:59Z

@kolyshkin could this have any relation to #36517 ?

kolyshkin · 2018-03-16T19:11:33Z

OK, the amended test revealed the error is returned from StdRead (that was previously ignored):

docker_api_attach_test.go:212:
    c.Assert(err, checker.IsNil)
... value *net.OpError = &net.OpError{Op:"read", Net:"unix", Source:(*net.UnixAddr)(0xc423f66680), Addr:(*net.UnixAddr)(0xc423f666a0), Err:(*poll.TimeoutError)(0x28347a0)} ("read unix @->/go/src/github.com/docker/docker/bundles/test-integration/docker.sock: i/o timeout")

kolyshkin · 2018-03-16T19:26:57Z

@kolyshkin could this have any relation to #36517 ?

It certainly might; let's see if reverting it will help

kolyshkin · 2018-03-17T00:54:58Z

OK, two rounds with commit 3798392 ("Ensure a hijacked connection implements CloseWrite whenever its underlying") reverted shows no failures (with my modified test). It's still not too many runs though.

Let's try without the revert now.

kolyshkin · 2018-03-20T02:10:02Z

Win CI timed out, experimental has some unrelated failures. I am currently out of ideas for this one.

tonistiigi · 2018-03-21T22:41:22Z

@kolyshkin FYI #36663

kolyshkin · 2018-03-22T03:13:53Z

@kolyshkin FYI #36663

@tonistiigi thanks! Also, the intermittent nature of the test case failure is explained by whether there's something in the buffer; if it is empty (which in our CI sometimes happens on slower hardware) the hijackedConnCloseWriter is not performed. Update: it is vise versa?

coolljt0725 · 2018-03-31T00:45:12Z

ping @thaJeztah @tonistiigi @vdemeester

vdemeester

LGTM 🐯

thaJeztah · 2018-04-03T18:22:34Z

integration-cli/docker_api_attach_test.go

-	c.Assert(actualStdout.Bytes(), checker.DeepEquals, []byte("hello\nsuccess"), check.Commentf("Attach didn't return the expected data from stdout"))
+	var outBuf, errBuf bytes.Buffer
+	_, err = stdcopy.StdCopy(&outBuf, &errBuf, resp.Reader)
+	if errors.Cause(err).(net.Error).Timeout() {


This would panic if err is nil, is that an issue?

Good catch, thank you, I have updated the commit

boaz0

LGTM 👍

When this test fails, the error looks like this: > FAIL: docker_api_attach_test.go:98: DockerSuite.TestPostContainersAttach > docker_api_attach_test.go:211: > c.Assert(actualStdout.Bytes(), checker.DeepEquals, []byte("hello\nsuccess"), check.Commentf("Attach didn't return the expected data from stdout")) > ... obtained []uint8 = []byte{0x73, 0x75, 0x63, 0x63, 0x65, 0x73, 0x73} > ... expected []uint8 = []byte{0x68, 0x65, 0x6c, 0x6c, 0x6f, 0xa, 0x73, 0x75, 0x63, 0x63, 0x65, 0x73, 0x73} > ... Attach didn't return the expected data from stdout Let's use strings for comparisons to make the output more readable. While at it, - get the container's stderr as well, and make sure it's empty; - check that stdcopy.StdCopy() did not return an error, except for the timeout which is expected; - move/remove comments, simplify var names. Signed-off-by: Kir Kolyshkin <[email protected]>

thaJeztah

LGTM

kolyshkin requested review from dnephin and vdemeester as code owners March 16, 2018 03:29

GordonTheTurtle added the status/0-triage label Mar 16, 2018

kolyshkin force-pushed the t-attach branch from 0f29403 to 5ab655f Compare March 16, 2018 06:58

vdemeester added status/2-code-review and removed status/0-triage labels Mar 16, 2018

kolyshkin changed the title ~~[do not merge] TestPostContainerAttach: improve the API part~~ [do not merge] TestPostContainersAttach: improve the API part Mar 16, 2018

kolyshkin force-pushed the t-attach branch from 5ab655f to 4980ec5 Compare March 16, 2018 19:24

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Mar 16, 2018

kolyshkin force-pushed the t-attach branch from 4980ec5 to cac65b5 Compare March 16, 2018 19:25

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Mar 16, 2018

kolyshkin force-pushed the t-attach branch from cac65b5 to 29eb72e Compare March 16, 2018 21:50

kolyshkin force-pushed the t-attach branch from 29eb72e to c794b77 Compare March 17, 2018 00:55

thaJeztah mentioned this pull request Mar 17, 2018

[18.03] Ensure a hijacked connection implements CloseWrite whenever its underlying docker-archive/docker-ce#467

Closed

thaJeztah added the rebuild/* label Mar 22, 2018

GordonTheTurtle removed the rebuild/* label Mar 22, 2018

kolyshkin force-pushed the t-attach branch from d9492d5 to cfc3b08 Compare March 26, 2018 16:48

kolyshkin changed the title ~~[do not merge] TestPostContainersAttach: improve the API part~~ TestPostContainersAttach: minor improvements Mar 26, 2018

GordonTheTurtle assigned coolljt0725 Mar 31, 2018

moby deleted a comment from GordonTheTurtle Apr 3, 2018

vdemeester approved these changes Apr 3, 2018

View reviewed changes

thaJeztah reviewed Apr 3, 2018

View reviewed changes

boaz0 approved these changes Apr 3, 2018

View reviewed changes

vdemeester added rebuild/windowsRS1 labels Apr 4, 2018

GordonTheTurtle removed rebuild/powerpc labels Apr 4, 2018

yongtang added the rebuild/powerpc label Apr 7, 2018

GordonTheTurtle removed the rebuild/powerpc label Apr 7, 2018

yongtang added the rebuild/powerpc label Apr 20, 2018

GordonTheTurtle removed the rebuild/powerpc label Apr 20, 2018

kolyshkin force-pushed the t-attach branch from cfc3b08 to ecc5488 Compare May 3, 2018 17:16

thaJeztah approved these changes May 3, 2018

View reviewed changes

thaJeztah added status/4-merge area/testing rebuild/* and removed status/2-code-review labels May 3, 2018

GordonTheTurtle removed the rebuild/* label May 3, 2018

thaJeztah added the rebuild/* label May 7, 2018

GordonTheTurtle removed the rebuild/* label May 7, 2018

yongtang merged commit bd5eb6b into moby:master May 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TestPostContainersAttach: minor improvements #36612

TestPostContainersAttach: minor improvements #36612

kolyshkin commented Mar 16, 2018 •

edited

Loading

codecov bot commented Mar 16, 2018 •

edited

Loading

thaJeztah commented Mar 16, 2018

kolyshkin commented Mar 16, 2018

kolyshkin commented Mar 16, 2018

kolyshkin commented Mar 17, 2018

kolyshkin commented Mar 20, 2018

tonistiigi commented Mar 21, 2018

kolyshkin commented Mar 22, 2018 •

edited

Loading

coolljt0725 commented Mar 31, 2018

vdemeester left a comment

thaJeztah Apr 3, 2018

kolyshkin May 3, 2018

boaz0 left a comment

thaJeztah left a comment

TestPostContainersAttach: minor improvements #36612

TestPostContainersAttach: minor improvements #36612

Conversation

kolyshkin commented Mar 16, 2018 • edited Loading

codecov bot commented Mar 16, 2018 • edited Loading

Codecov Report

thaJeztah commented Mar 16, 2018

kolyshkin commented Mar 16, 2018

kolyshkin commented Mar 16, 2018

kolyshkin commented Mar 17, 2018

kolyshkin commented Mar 20, 2018

tonistiigi commented Mar 21, 2018

kolyshkin commented Mar 22, 2018 • edited Loading

coolljt0725 commented Mar 31, 2018

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah Apr 3, 2018

Choose a reason for hiding this comment

kolyshkin May 3, 2018

Choose a reason for hiding this comment

boaz0 left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

kolyshkin commented Mar 16, 2018 •

edited

Loading

codecov bot commented Mar 16, 2018 •

edited

Loading

kolyshkin commented Mar 22, 2018 •

edited

Loading