don't cancel container stop when cancelling context#45738
don't cancel container stop when cancelling context#45738thaJeztah merged 1 commit intomoby:masterfrom
Conversation
|
|
||
| // containerStop sends a stop signal, waits, sends a kill signal. | ||
| func (daemon *Daemon) containerStop(ctx context.Context, ctr *container.Container, options containertypes.StopOptions) (retErr error) { | ||
| func (daemon *Daemon) containerStop(_ context.Context, ctr *container.Container, options containertypes.StopOptions) (retErr error) { |
There was a problem hiding this comment.
Been going back-and-forth whether to remove the context.Context here, but decided to go for the most minimal changes to fix the regression to reduce code-churn (and we may want to have the context available here in future for other purposes).
|
|
||
| // Give the ContainerStop some time to make sure it's being handled, | ||
| // then cancel the context or close the client to cancel the stop. | ||
| time.Sleep(100 * time.Millisecond) |
There was a problem hiding this comment.
Not very happy with the test (and this sleep); better suggestions welcome 😅
There was a problem hiding this comment.
Collude with the container: trap the stop signal and handle it by logging to stdout then sleeping some more, similar to the tests in wait_test.go. The test can then wait for the log message from the container to know when it can proceed.
| switch tc { | ||
| case cancelContext: | ||
| assert.Check(t, is.ErrorType(err, errdefs.IsCancelled)) | ||
| case closeConnection: | ||
| assert.Check(t, err) | ||
| } |
There was a problem hiding this comment.
There's no synchronization between this goroutine and the test. These checks could potentially run after the test function has returned. I recommend sending the error over a channel and performing these checks on the test goroutine, e.g.:
stoppedCh := make(chan error)
go func() {
stoppedCh <- apiClient.ContainerStop(ctxCancel, ...)
}()
// [...]
var stoppedErr error
select {
case stoppedErr = <-stoppedCh:
case <-time.After(...):
t.Fatal("Didn't stop??")
}
// Assert stoppedErrThere was a problem hiding this comment.
Ah, thanks; yes, that probably works
|
|
||
| // Give the ContainerStop some time to make sure it's being handled, | ||
| // then cancel the context or close the client to cancel the stop. | ||
| time.Sleep(100 * time.Millisecond) |
There was a problem hiding this comment.
Collude with the container: trap the stop signal and handle it by logging to stdout then sleeping some more, similar to the tests in wait_test.go. The test can then wait for the log message from the container to know when it can proceed.
| // close the client connection | ||
| assert.Check(t, apiClient.Close()) | ||
| } |
There was a problem hiding this comment.
Actually, this test-case doesn't make sense at all; apiClient.Close() does not terminate requests, it closes idle connections;
Lines 186 to 192 in a978888
b88c10b to
b089f27
Compare
|
Hmm... weird why this failed in the docker-py dockerfile; Unless the base-image changed 🤔 |
|
Commit 90de570 passed through the request context to daemon.ContainerStop(). As a result, cancelling the context would cancel the "graceful" stop of the container, and would proceed with forcefully killing the container. This patch partially reverts the changes from 90de570 and breaks the context to prevent cancelling the context from cancelling the stop. Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
b089f27 to
fc94ed0
Compare
|
Rebased to get the docker-py fix in |
Commit 90de570 passed through the request context to daemon.ContainerStop(). As a result, cancelling the context would cancel the "graceful" stop of the container, and would proceed with forcefully killing the container.
This patch partially reverts the changes from 90de570 and breaks the context to prevent cancelling the context from cancelling the stop.
- 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)