daemon.ContainerStop(): fix for a negative timeout #36874

kolyshkin · 2018-04-17T03:05:14Z

As daemon.ContainerStop() documentation says,

If a negative number of seconds is given, ContainerStop
will wait for a graceful termination.

but since commit cfdf84d (PR #32237) this is no longer the case.
This happens because context.WithTimeout(ctx, timeout) is implemented
as WithDeadline(ctx, time.Now().Add(timeout)), resulting in a deadline
which is in the past.

To fix, don't use WithDeadline() if the timeout is negative.

A test case is added to validate the correct behavior and as a means
to prevent a similar regression in the future.

This is a replacement of #35418, and should fix #35311.

kolyshkin · 2018-04-17T03:05:40Z

Created for the sake of CI, will add more later.

codecov · 2018-04-17T05:17:53Z

Codecov Report

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

@@            Coverage Diff            @@
##             master   #36874   +/-   ##
=========================================
  Coverage          ?   34.97%           
=========================================
  Files             ?      614           
  Lines             ?    45641           
  Branches          ?        0           
=========================================
  Hits              ?    15965           
  Misses            ?    27587           
  Partials          ?     2089

kolyshkin · 2018-04-17T16:26:20Z

CI failure on janky (logs here) is a flaky TestAPIServiceUpdatePort (#36501).

CI failure on z (logs here) is a flaky test TestAPISwarmNodeDrainPause (#23516).

CI failure on Windows is weird, the build just timed out without running any tests (logs here); restarted.

cpuguy83 · 2018-04-18T10:31:19Z

integration/container/stop_test.go

+// a timeout works as documented, i.e. in case of negative timeout
+// waiting is not limited (issue #35311).
+func TestStopContainerWithTimeout(t *testing.T) {
+	t.Parallel()


This makes for a flakey test whennused with the main integration daemon.

Can you please elaborate why?

Because it is running in the main daemon.
The test setup takes a snapshot of the current state and then at the end of the test cleans up anything that was not in the original state. But if there are multiple tests doing this cleanup they will conflict with each other, clean up stuff they shouldn't be, etc.

cpuguy83 · 2018-04-18T10:33:05Z

integration/container/stop_test.go

+	}
+
+	for _, d := range testData {
+		name := container.WithName(t.Name() + "_" + strconv.Itoa(d.timeout))


Use "t.Run" so the different cases run as separate tests. This makes it easiee to know which case failed (if there is a failure)

done, thanks for suggestion

The unit test is checking that setting of non-default StopTimeout works, but it checked the value of StopSignal instead. Amazingly, the test was working since the default StopSignal is SIGTERM, which has the numeric value of 15. Fixes: commit e66d210 ("Add config parameter to change ...") Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2018-04-19T21:37:29Z

OK this is no longer WIP. CI failure in experimental is flaky test DockerSwarmSuite.TestAPIServiceUpdatePort (#36501).

kolyshkin · 2018-04-30T16:58:34Z

@cpuguy83 can you PTAL? All your comments were acted upon

cpuguy83 · 2018-05-01T13:09:54Z

integration/container/stop_test.go

+		d := d
+		name := t.Name() + "_" + strconv.Itoa(d.timeout)
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()


I think each one of these is treated as a separate test, so they all get grouped together at the end of the test run with the rest of the parallel tests. So I'm not sure this is any safer than before.

I don't think so. Surely I might be wrong, but according to the golang pkg/testing documentation, such tests are only run in parallel with each other:

Subtests can also be used to control parallelism. A parent test will only complete once all of its subtests complete. In this example, all tests are run in parallel with each other, and only with each other, regardless of other top-level tests that may be defined:
func TestGroupedParallel(t *testing.T) { for _, tc := range tests { tc := tc // capture range variable t.Run(tc.Name, func(t *testing.T) { t.Parallel() ... }) } }

Also, I got the idea from similar tests like this one:

moby/integration/container/create_test.go

Lines 41 to 46 in fe2d3a1

for _, tc := range testCases {

tc := tc

t.Run(tc.doc, func(t *testing.T) {

t.Parallel()

_, err := client.ContainerCreate(context.Background(),

&container.Config{Image: tc.image},

@cpuguy83 PTAL ^^^

Nice, this does seem to work.

1. As daemon.ContainerStop() documentation says, > If a negative number of seconds is given, ContainerStop > will wait for a graceful termination. but since commit cfdf84d (PR moby#32237) this is no longer the case. This happens because `context.WithTimeout(ctx, timeout)` is implemented as `WithDeadline(ctx, time.Now().Add(timeout))`, resulting in a deadline which is in the past. To fix, don't use WithDeadline() if the timeout is negative. 2. Add a test case to validate the correct behavior and as a means to prevent a similar regression in the future. 3. Fix/improve daemon.ContainerStop() and client.ContainerStop() description for clarity and completeness. 4. Fix/improve DefaultStopTimeout description. Fixes: cfdf84d ("Update Container Wait") Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2018-05-03T17:06:52Z

Minor change:

don't add test name to subtest name as it's redundant
don't set container name

-               name := t.Name() + "_" + strconv.Itoa(d.timeout)
-               t.Run(name, func(t *testing.T) {
+               t.Run(strconv.Itoa(d.timeout), func(t *testing.T) {
                        t.Parallel()
-                       id := container.Run(t, ctx, client, testCmd, container.WithName(name))
+                       id := container.Run(t, ctx, client, testCmd)

cpuguy83

LGTM

kolyshkin · 2018-05-09T17:11:17Z

CI failure in TestAPISwarmLeaderElection is #32673

kolyshkin · 2018-05-10T23:27:36Z

not sure why I had it as [WIP] -- it's definitely not )

vdemeester

LGTM 🐯

cpuguy83 · 2018-05-30T17:38:36Z

16 days since last CI run... but this is probably fine.
YOLO merge.

kolyshkin requested a review from vdemeester as a code owner April 17, 2018 03:05

GordonTheTurtle added the status/0-triage label Apr 17, 2018

kolyshkin force-pushed the stop-timeout branch from a71e2c7 to 77d89a5 Compare April 17, 2018 03:07

kolyshkin requested a review from dnephin as a code owner April 17, 2018 03:07

kolyshkin changed the title ~~[WIP] StopContainer(): fix for a negative timeout~~ [WIP] daemon.ContainerStop(): fix for a negative timeout Apr 17, 2018

kolyshkin force-pushed the stop-timeout branch from 77d89a5 to ef5b03d Compare April 17, 2018 05:17

kolyshkin force-pushed the stop-timeout branch from ef5b03d to 2b4d32b Compare April 17, 2018 05:20

vdemeester added status/2-code-review and removed status/0-triage labels Apr 17, 2018

kolyshkin force-pushed the stop-timeout branch from 2b4d32b to 96ad90a Compare April 18, 2018 03:00

kolyshkin mentioned this pull request Apr 18, 2018

Fix negative time out policy when stop the container #35418

Closed

cpuguy83 requested changes Apr 18, 2018

View reviewed changes

kolyshkin force-pushed the stop-timeout branch from 96ad90a to fbcf370 Compare April 19, 2018 18:55

cpuguy83 reviewed May 1, 2018

View reviewed changes

GordonTheTurtle assigned dnephin May 2, 2018

kolyshkin force-pushed the stop-timeout branch from fbcf370 to 69b4fe4 Compare May 3, 2018 17:05

cpuguy83 approved these changes May 9, 2018

View reviewed changes

cpuguy83 added the rebuild/experimental label May 9, 2018

GordonTheTurtle removed the rebuild/experimental label May 9, 2018

cpuguy83 added the rebuild/janky label May 9, 2018

GordonTheTurtle removed the rebuild/janky label May 9, 2018

cpuguy83 added the rebuild/powerpc label May 9, 2018

cpuguy83 added the rebuild/z label May 9, 2018

GordonTheTurtle removed rebuild/powerpc labels May 9, 2018

kolyshkin changed the title ~~[WIP] daemon.ContainerStop(): fix for a negative timeout~~ daemon.ContainerStop(): fix for a negative timeout May 10, 2018

vdemeester approved these changes May 14, 2018

View reviewed changes

vdemeester added the rebuild/experimental label May 14, 2018

GordonTheTurtle removed the rebuild/experimental label May 14, 2018

cpuguy83 merged commit b85799b into moby:master May 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon.ContainerStop(): fix for a negative timeout #36874

daemon.ContainerStop(): fix for a negative timeout #36874

kolyshkin commented Apr 17, 2018 •

edited

Loading

kolyshkin commented Apr 17, 2018

codecov bot commented Apr 17, 2018 •

edited

Loading

kolyshkin commented Apr 17, 2018

cpuguy83 Apr 18, 2018

kolyshkin Apr 19, 2018

cpuguy83 Apr 19, 2018

cpuguy83 Apr 18, 2018

kolyshkin Apr 19, 2018

kolyshkin commented Apr 19, 2018

kolyshkin commented Apr 30, 2018

cpuguy83 May 1, 2018

kolyshkin May 1, 2018 •

edited

Loading

kolyshkin May 3, 2018

cpuguy83 May 9, 2018

kolyshkin commented May 3, 2018

cpuguy83 left a comment

kolyshkin commented May 9, 2018

kolyshkin commented May 10, 2018

vdemeester left a comment

cpuguy83 commented May 30, 2018

	for _, tc := range testCases {
	tc := tc
	t.Run(tc.doc, func(t *testing.T) {
	t.Parallel()
	_, err := client.ContainerCreate(context.Background(),
	&container.Config{Image: tc.image},

daemon.ContainerStop(): fix for a negative timeout #36874

daemon.ContainerStop(): fix for a negative timeout #36874

Conversation

kolyshkin commented Apr 17, 2018 • edited Loading

kolyshkin commented Apr 17, 2018

codecov bot commented Apr 17, 2018 • edited Loading

Codecov Report

kolyshkin commented Apr 17, 2018

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

kolyshkin commented Apr 19, 2018

kolyshkin commented Apr 30, 2018

Choose a reason for hiding this comment

kolyshkin May 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolyshkin commented May 3, 2018

cpuguy83 left a comment

Choose a reason for hiding this comment

kolyshkin commented May 9, 2018

kolyshkin commented May 10, 2018

vdemeester left a comment

Choose a reason for hiding this comment

cpuguy83 commented May 30, 2018

kolyshkin commented Apr 17, 2018 •

edited

Loading

codecov bot commented Apr 17, 2018 •

edited

Loading

kolyshkin May 1, 2018 •

edited

Loading