attach: Use ContainerWait instead of Inspect #696

mlaventure · 2017-11-16T03:24:26Z

Signed-off-by: Kenfe-Mickael Laventure mickael.laventure@gmail.com

--

Addresses moby/moby#35497

- What I did

Use ContainerWait() instead of Inspect to retrieve the final exit code in atach

- How I did it

Changed the code ;-)

- How to verify it

$ docker run -f --name exit alpine sh -c 'sleep 10; exit 21'
$ docker attach exit
$ echo $?
21

- Description for the changelog

Ensure attach exit code matches container's

- A picture of a cute animal (not mandatory but encouraged)
🐶

dnephin

Change looks good, but there are test and lint failure

Would you mind writing a TestAttach in e2e/container/attach_test.go ?

mlaventure · 2017-11-16T16:28:55Z

Sure, will give it a go

dnephin · 2017-11-16T16:29:18Z

cli/command/container/attach.go

@@ -66,6 +66,9 @@ func runAttach(dockerCli command.Cli, opts *attachOptions) error {
 	ctx := context.Background()
 	client := dockerCli.Client()

+	// request channel to wait for client
+	resultC, errC := client.ContainerWait(ctx, opts.container, "")


Oh, actually I think this should come after inspectContainerAndCheckState() which is a sanity check on the state of the container.

I wanted to avoid a race when by the time we return from inspectContainerAndCheckState() the container had exited, but I don't mind moving it afterwards.

good point, maybe it is more correct before the inspect. The error from ContainerWait() wouldn't be reported until later, so the user would still see the helpful error message.

I think this is correct, ignore my comment.

codecov-io · 2017-11-17T16:13:25Z

Codecov Report

Merging #696 into master will decrease coverage by 0.67%.
The diff coverage is 93.75%.

@@            Coverage Diff            @@
##           master    #696      +/-   ##
=========================================
- Coverage   51.28%   50.6%   -0.68%     
=========================================
  Files         216     216              
  Lines       17743   17728      -15     
=========================================
- Hits         9099    8972     -127     
- Misses       8175    8304     +129     
+ Partials      469     452      -17

mlaventure · 2017-11-17T16:59:13Z

I'm not sure what to do about the gocyclo complain. I don't want to make a function just because of it. So if we don't want to raise the limit, I would need to think of a way of refactoring the would function.

Regarding the test failure, it is very strange, it looks like we do get the output, but don't notice that the process is dead? I haven't been able to reproduce it locally.

dnephin · 2017-11-17T17:20:58Z

cli/command/container/attach.go

@@ -140,7 +143,18 @@ func runAttach(dockerCli command.Cli, opts *attachOptions) error {
 	if errAttach != nil {
 		return errAttach
 	}
-	return getExitStatus(ctx, dockerCli.Client(), opts.container)
+
+	// get status code


This seems like a perfect place to create a new function. A function is always better than an inline comment as it provides a scope for the documentation.

I think the unit test you removed could be preserved and used to test the function, with a few small changes to the test setup. The 3 cases in the table test are still relevant.

It feels weird given that it's really just a select, the comment could be removed without affecting readability, but I'll try that.

dnephin · 2017-11-17T17:27:31Z

e2e/container/attach_test.go

+	cmd := icmd.Command("docker", "wait", cName)
+	res := icmd.StartCmd(cmd)
+	icmd.RunCommand("docker", "attach", cName).Assert(t, icmd.Expected{ExitCode: 21})
+	icmd.WaitOnCmd(8, res).Assert(t, icmd.Expected{ExitCode: 0, Out: "21"})


This is only waiting 8ms, I'm not sure if that's related to the failure.

I'll see if I can get this working

This test fails for me locally as well so I'll prepare a patch to get it working.

I think the docker wait command is unnecessary, all we need is a poll.WaitOn() for the container to be started.

Didn't realize it was in ms 😅

Thanks poll.WaitOn() seems to be what I was looking for. I'll try updating the code tonight.

I pushed a commit, it seems to be working now. I didn't use the WaitOn for container start. I'm not sure if this test can flake, maybe runBackgroundContainsWithExitCode() could use a WaitOn if we see it flake.

dnephin · 2017-11-17T17:47:10Z

I pushed a commit to this branch which I think will fix the e2e suite.

Edit: build didn't trigger on jenkins for some reason, so I forced a run from jenkins ui

Edit: it's green!

Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>

Signed-off-by: Daniel Nephin <dnephin@docker.com>

mlaventure · 2017-11-18T02:16:43Z

All green 💚

dnephin

LGTM

tonistiigi · 2017-12-09T00:13:43Z

What caused this change? Was there a problem in all previous versions or did the API change? The linked issue seems like a regression.

mlaventure · 2017-12-09T00:26:20Z

From my understanding of the code, the race was always here, is just that a bit more of the post processing has been moved to the event handler now and increased the latency.

tonistiigi · 2017-12-15T06:50:26Z

LGTM

thaJeztah · 2018-01-20T22:55:11Z

Looks like we have 2 LGTM's; merging

andrewhsu · 2018-01-31T05:50:24Z

FYI this PR don't play nice with latest code from moby/moby in docker-ce 18.02.0 rc1. Getting failures like:

panic: DockerSuite.TestAttachAfterDetach test timed out after 5m0s [recovered]
	panic: DockerSuite.TestAttachAfterDetach test timed out after 5m0s

Reverting the changes in this PR and stuff goes green.

dnephin · 2018-01-31T23:53:06Z

That test seems to have a 2s timeout for attach so I'm not sure how it's failing this way. I'll investigate

dnephin · 2018-02-20T23:28:50Z

Seems to be an issue with the test, Opened moby/moby#36363

GordonTheTurtle added the status/0-triage label Nov 16, 2017

dnephin added status/2-code-review and removed status/0-triage labels Nov 16, 2017

dnephin reviewed Nov 16, 2017

View reviewed changes

mlaventure force-pushed the attach-use-containerwait branch 2 times, most recently from 864ba8b to 992c9ee Compare November 17, 2017 16:13

dnephin reviewed Nov 17, 2017

View reviewed changes

mlaventure force-pushed the attach-use-containerwait branch 2 times, most recently from 361354c to e84bf6c Compare November 18, 2017 01:43

mlaventure and others added 2 commits November 17, 2017 18:12

attach: Use ContainerWait instead of Inspect

6666176

Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>

Fix TestAttachExitCode

bace33d

Signed-off-by: Daniel Nephin <dnephin@docker.com>

mlaventure force-pushed the attach-use-containerwait branch from e84bf6c to bace33d Compare November 18, 2017 02:12

mlaventure mentioned this pull request Nov 18, 2017

Add missing lock in ProcessEvent moby/moby#35516

Merged

dnephin approved these changes Nov 19, 2017

View reviewed changes

thaJeztah assigned tonistiigi Nov 30, 2017

thaJeztah added this to the 17.12.0 milestone Nov 30, 2017

thaJeztah removed this from the 17.12.0 milestone Jan 20, 2018

thaJeztah merged commit 44a1168 into docker:master Jan 20, 2018

GordonTheTurtle added this to the 18.02.0 milestone Jan 20, 2018

thaJeztah added the impact/changelog label Jan 20, 2018

mlaventure deleted the attach-use-containerwait branch January 27, 2018 05:46

andrewhsu mentioned this pull request Jan 31, 2018

[18.02] revert attach: Use ContainerWait instead of Inspect docker-archive/docker-ce#406

Merged

dnephin mentioned this pull request Feb 20, 2018

Fix TestAttachAfterDetach to work with latest client moby/moby#36363

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

attach: Use ContainerWait instead of Inspect #696

attach: Use ContainerWait instead of Inspect #696

mlaventure commented Nov 16, 2017

dnephin left a comment

mlaventure commented Nov 16, 2017

dnephin Nov 16, 2017

mlaventure Nov 16, 2017

dnephin Nov 16, 2017

codecov-io commented Nov 17, 2017 •

edited

Loading

mlaventure commented Nov 17, 2017

dnephin Nov 17, 2017 •

edited

Loading

mlaventure Nov 17, 2017

dnephin Nov 17, 2017

dnephin Nov 17, 2017

mlaventure Nov 17, 2017

dnephin Nov 17, 2017

dnephin commented Nov 17, 2017 •

edited

Loading

mlaventure commented Nov 18, 2017

dnephin left a comment

tonistiigi commented Dec 9, 2017

mlaventure commented Dec 9, 2017

tonistiigi commented Dec 15, 2017

thaJeztah commented Jan 20, 2018

andrewhsu commented Jan 31, 2018

dnephin commented Jan 31, 2018

dnephin commented Feb 20, 2018

attach: Use ContainerWait instead of Inspect #696

attach: Use ContainerWait instead of Inspect #696

Conversation

mlaventure commented Nov 16, 2017

dnephin left a comment

Choose a reason for hiding this comment

mlaventure commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 17, 2017 • edited Loading

Codecov Report

mlaventure commented Nov 17, 2017

dnephin Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin commented Nov 17, 2017 • edited Loading

mlaventure commented Nov 18, 2017

dnephin left a comment

Choose a reason for hiding this comment

tonistiigi commented Dec 9, 2017

mlaventure commented Dec 9, 2017

tonistiigi commented Dec 15, 2017

thaJeztah commented Jan 20, 2018

andrewhsu commented Jan 31, 2018

dnephin commented Jan 31, 2018

dnephin commented Feb 20, 2018

codecov-io commented Nov 17, 2017 •

edited

Loading

dnephin Nov 17, 2017 •

edited

Loading

dnephin commented Nov 17, 2017 •

edited

Loading