daemon: daemon.containerRestart: don't cancel restart on context cancel by thaJeztah · Pull Request #46688 · moby/moby

thaJeztah · 2023-10-20T16:36:10Z

fixes Restart container from inside #46682
relates to imageservice: Add context to various methods #44365
relates to don't cancel container stop when cancelling context #45738

commit def549c (#44365) passed through the context to the daemon.ContainerStart function. As a result, restarting containers no longer is an atomic operation, because a context cancellation could interrupt the restart (between "stopping" and "(re)starting"), resulting in the container being stopped, but not restarted.

Restarting a container, or more factually; making a successful request on the /containers/{id]/restart endpoint, should be an atomic operation.

This patch uses a context.WithoutCancel for restart requests.

It's worth noting that daemon.containerStop already uses context.WithoutCancel, so in that function, we'll be wrapping the context twice, but this should likely not cause issues (just redundant for this code-path).

Before this patch, starting a container that bind-mounts the docker socket, then restarting itself from within the container would cancel the restart operation. The container would be stopped, but not started after that:

docker run -dit --name myself -v /var/run/docker.sock:/var/run/docker.sock docker:cli sh
docker exec myself sh -c 'docker restart myself'

docker ps -a
CONTAINER ID   IMAGE         COMMAND                  CREATED          STATUS                       PORTS     NAMES
3a2a741c65ff   docker:cli    "docker-entrypoint.s…"   26 seconds ago   Exited (128) 7 seconds ago             myself

With this patch: the stop still cancels the exec, but does not cancel the restart operation, and the container is started again:

docker run -dit --name myself -v /var/run/docker.sock:/var/run/docker.sock docker:cli sh
docker exec myself sh -c 'docker restart myself'
docker ps
CONTAINER ID   IMAGE        COMMAND                  CREATED              STATUS         PORTS     NAMES
4393a01f7c75   docker:cli   "docker-entrypoint.s…"   About a minute ago   Up 4 seconds             myself

- 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)

thaJeztah · 2023-10-20T16:38:06Z

Looking if i can come up with a test somehow. I can probably write a test that immediately cancels the context, but that wouldn't be testing the "half-way" situation (although knowing that we already fixed canceling the context for containerStop (in #45738), perhaps that's still enough.

thaJeztah · 2023-10-21T13:43:56Z

Test is not happy on windows 😞

=== FAIL: github.com/docker/docker/integration/container TestContainerRestartWithCancelledRequest (4.63s)
    restart_test.go:286: assertion failed: error is not nil: timeout waiting for restart event
    restart_test.go:244: Cleaning up test container failed with error: 
      Error response from daemon: container c25e1bc8c9082a4c579db9d41ceee15c20019b58c4638cdd941b3d3781d915a1: 
        driver "windowsfilter" failed to remove root filesystem: second rename attempt following detach failed: 
          rename C:\Users\runneradmin\AppData\Local\Temp\moby-root\windowsfilter\c25e1bc8c9082a4c579db9d41ceee15c20019b58c4638cdd941b3d3781d915a1 C:\Users\runneradmin\AppData\Local\Temp\moby-root\windowsfilter\c25e1bc8c9082a4c579db9d41ceee15c20019b58c4638cdd941b3d3781d915a1-removing: 
            Access is denied.: 
              rename C:\Users\runneradmin\AppData\Local\Temp\moby-root\windowsfilter\c25e1bc8c9082a4c579db9d41ceee15c20019b58c4638cdd941b3d3781d915a1 C:\Users\runneradmin\AppData\Local\Temp\moby-root\windowsfilter\c25e1bc8c9082a4c579db9d41ceee15c20019b58c4638cdd941b3d3781d915a1-removing: 
                Access is denied.

thaJeztah · 2023-10-23T13:49:58Z

@vvoland @rumpl PTAL (writing the test was a bit finicky; suggestions on better ways to test this welcome!)

corhere

Ideally, the decision on whether the container-restart operation is cancelable should be left up to the caller of daemon.ContainerRestart(). But there are two problems with that:

It would make daemon.ContainerRestart() inconsistent with daemon.ContainerStop(). (#46687)
daemon.ContainerRestart() starts by calling daemon.GetContainer(), which we eventually want to plumb contexts into for containerd reasons. We need a way for the caller to be able to apply a different cancelation policy to the GetContainer() call than to the stop-restart operation itself.

Addressing those problems is a bigger task which would blow up the scope of this PR, so would best be saved for a follow-up.

corhere · 2023-10-23T17:58:41Z

+					return events.Message{}, nil
+				}
+				return events.Message{}, err
+			case <-time.After(restartTimeout):


Each turn of the loop resets the timeout. That doesn't seem right. Shouldn't this timer be started outside the loop?

Doh! I recall I went looking if we have a utility for handling the event stream in tests, and landed on TestEventsVolumeCreate as "somewhat close", and copied that

moby/integration/system/event_test.go

Lines 139 to 155 in 452ca90

getEvents := func(messages <-chan events.Message, errs <-chan error) ([]events.Message, error) {

var evts []events.Message

for {

select {

case m := <-messages:

evts = append(evts, m)

case err := <-errs:

if err == io.EOF {

return evts, nil

}

return nil, err

case <-time.After(time.Second * 3):

return nil, errors.New("timeout hit")

}

}

}

As we're filtering, we'd only expect a single event here, so I can just remove the for loop altogether.

Actually, I wouldn't even need the closure at all here, so let me inline the things.

commit def549c passed through the context to the daemon.ContainerStart function. As a result, restarting containers no longer is an atomic operation, because a context cancellation could interrupt the restart (between "stopping" and "(re)starting"), resulting in the container being stopped, but not restarted. Restarting a container, or more factually; making a successful request on the `/containers/{id]/restart` endpoint, should be an atomic operation. This patch uses a context.WithoutCancel for restart requests. It's worth noting that daemon.containerStop already uses context.WithoutCancel, so in that function, we'll be wrapping the context twice, but this should likely not cause issues (just redundant for this code-path). Before this patch, starting a container that bind-mounts the docker socket, then restarting itself from within the container would cancel the restart operation. The container would be stopped, but not started after that: docker run -dit --name myself -v /var/run/docker.sock:/var/run/docker.sock docker:cli sh docker exec myself sh -c 'docker restart myself' docker ps -a CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES 3a2a741c65ff docker:cli "docker-entrypoint.s…" 26 seconds ago Exited (128) 7 seconds ago myself With this patch: the stop still cancels the exec, but does not cancel the restart operation, and the container is started again: docker run -dit --name myself -v /var/run/docker.sock:/var/run/docker.sock docker:cli sh docker exec myself sh -c 'docker restart myself' docker ps CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES 4393a01f7c75 docker:cli "docker-entrypoint.s…" About a minute ago Up 4 seconds myself Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2023-10-24T13:02:54Z

okay; this should be ready for re-review; @vvoland @rumpl @corhere

rumpl · 2023-10-26T07:57:17Z

+
+	// Make restart request, but cancel the request before the container
+	// is (forcibly) killed.
+	ctx2, cancel := context.WithTimeout(ctx, 100*time.Millisecond)


Do we need the timeout? Can we use WithCancel here?

Hm, good point; not sure why I did the timeout here 🤔 this may have been from an earlier iteration where I tried a slightly different approach 🙈

Guess it doesn't harm, but indeed probably wouldn't have been needed.

rumpl

LGTM just have one question

thaJeztah added status/2-code-review process/cherry-pick kind/bugfix PR's that fix bugs process/cherry-pick/24.0 labels Oct 20, 2023

thaJeztah added this to the 25.0.0 milestone Oct 20, 2023

thaJeztah force-pushed the restart_nocancel branch 3 times, most recently from fe084a1 to ffd9765 Compare October 21, 2023 12:14

thaJeztah mentioned this pull request Oct 21, 2023

[24.0 backport] daemon: daemon.containerRestart: don't cancel restart on context cancel #46697

Merged

2 tasks

thaJeztah force-pushed the restart_nocancel branch 2 times, most recently from c72740e to 88225ad Compare October 23, 2023 09:41

thaJeztah added the kind/regression label Oct 23, 2023

corhere reviewed Oct 23, 2023

View reviewed changes

thaJeztah force-pushed the restart_nocancel branch from 88225ad to 2397030 Compare October 24, 2023 10:27

thaJeztah force-pushed the restart_nocancel branch from 2397030 to aeb8972 Compare October 24, 2023 10:56

corhere approved these changes Oct 24, 2023

View reviewed changes

thaJeztah added process/cherry-picked and removed process/cherry-pick process/cherry-pick/24.0 labels Oct 24, 2023

vvoland approved these changes Oct 26, 2023

View reviewed changes

thaJeztah merged commit e0476be into moby:master Oct 26, 2023

thaJeztah deleted the restart_nocancel branch October 26, 2023 07:50

rumpl reviewed Oct 26, 2023

View reviewed changes

rumpl approved these changes Oct 26, 2023

View reviewed changes

thaJeztah mentioned this pull request Oct 31, 2023

docker run --rm can end up not removing containers #46749

Open

thaJeztah mentioned this pull request Nov 10, 2023

chore: verify context cancellation for endpoints that mutate state #46798

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon: daemon.containerRestart: don't cancel restart on context cancel#46688

daemon: daemon.containerRestart: don't cancel restart on context cancel#46688
thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah:restart_nocancel

thaJeztah commented Oct 20, 2023 •

edited

Loading

Uh oh!

thaJeztah commented Oct 20, 2023

Uh oh!

thaJeztah commented Oct 21, 2023 •

edited

Loading

Uh oh!

thaJeztah commented Oct 23, 2023

Uh oh!

corhere left a comment

Uh oh!

corhere Oct 23, 2023

Uh oh!

thaJeztah Oct 24, 2023

Uh oh!

thaJeztah commented Oct 24, 2023

Uh oh!

rumpl Oct 26, 2023

Uh oh!

thaJeztah Oct 26, 2023

Uh oh!

rumpl left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	getEvents := func(messages <-chan events.Message, errs <-chan error) ([]events.Message, error) {
	var evts []events.Message

	for {
	select {
	case m := <-messages:
	evts = append(evts, m)
	case err := <-errs:
	if err == io.EOF {
	return evts, nil
	}
	return nil, err
	case <-time.After(time.Second * 3):
	return nil, errors.New("timeout hit")
	}
	}
	}

Conversation

thaJeztah commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Oct 20, 2023

Uh oh!

thaJeztah commented Oct 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Oct 23, 2023

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

corhere Oct 23, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 24, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 24, 2023

Uh oh!

rumpl Oct 26, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 26, 2023

Choose a reason for hiding this comment

Uh oh!

rumpl left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thaJeztah commented Oct 20, 2023 •

edited

Loading

thaJeztah commented Oct 21, 2023 •

edited

Loading