daemon: cleanupContainer: don't fail if container is already stopped#46213
Merged
thaJeztah merged 3 commits intomoby:masterfrom Aug 24, 2023
Merged
daemon: cleanupContainer: don't fail if container is already stopped#46213thaJeztah merged 3 commits intomoby:masterfrom
thaJeztah merged 3 commits intomoby:masterfrom
Conversation
0c30262 to
d72a568
Compare
1 task
17ceaee to
01eb5a6
Compare
Saw this failure in a flaky test, and I wondered why we consider this an
error condition;
=== RUN TestKillWithStopSignalAndRestartPolicies
main_test.go:32: assertion failed: error is not nil: Error response from daemon: Could not kill running container 668f62511f4aa62357269cd405cff1fbe295b7f6d5011e7cfed434e3072330b7, cannot remove - Container 668f62511f4aa62357269cd405cff1fbe295b7f6d5011e7cfed434e3072330b7 is not running: failed to remove 668f62511f4aa62357269cd405cff1fbe295b7f6d5011e7cfed434e3072330b7
--- FAIL: TestKillWithStopSignalAndRestartPolicies (0.84s)
=== RUN TestKillWithStopSignalAndRestartPolicies/same-signal-disables-restart-policy
--- PASS: TestKillWithStopSignalAndRestartPolicies/same-signal-disables-restart-policy (0.42s)
=== RUN TestKillWithStopSignalAndRestartPolicies/different-signal-keep-restart-policy
--- PASS: TestKillWithStopSignalAndRestartPolicies/different-signal-keep-restart-policy (0.23s)
In the above;
1. `Error response from daemon: Could not kill running container 668f62511f4aa62357269cd405cff1fbe295b7f6d5011e7cfed434e3072330b7`
2. `cannot remove - Container 668f62511f4aa62357269cd405cff1fbe295b7f6d5011e7cfed434e3072330b7 is not running`
3. `failed to remove 668f62511f4aa62357269cd405cff1fbe295b7f6d5011e7cfed434e3072330b7`
So it looks like the removal fails because we couldn't kill the container
because it was already stopped, which may be a race condition where the first
check shows the container to be running (but may already be in process to be
removed or killed. In either case, we probably shouldn't fail the removal if
the container is already stopped.
This patch adds a `isNotRunning()` utility, so that we can ignore this case,
and proceed with the removal.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Also remove integration-cli: `DockerAPISuite.TestContainerAPIDeleteConflict`, which was testing the same conditions as `TestRemoveContainerRunning` in integration/container. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
01eb5a6 to
c0568a9
Compare
Member
Author
|
@neersighted @rumpl ptal 🤗 |
1 task
rumpl
approved these changes
Aug 24, 2023
Member
rumpl
left a comment
There was a problem hiding this comment.
LGTM, do we want to change the Cannot kill container error message too while we're at it?
-Error response from daemon: Cannot kill container: 79a: Container 79a19db9dd2ce7be5d9ca5651fbdd5e2c1b4dbe3a0ec9cff18af9e820ac6f88e is not running
+Error response from daemon: cannot kill container: 79a: container 79a19db9dd2ce7be5d9ca5651fbdd5e2c1b4dbe3a0ec9cff18af9e820ac6f88e is not running
Member
Author
|
Good one; I see there's a Let me do those in a follow-up, because it's possible that |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
daemon: cleanupContainer: don't fail if container is already stopped
Saw this failure in a flaky test, and I wondered why we consider this an
error condition;
In the above;
Error response from daemon: Could not kill running container 668f62511f4aa62357269cd405cff1fbe295b7f6d5011e7cfed434e3072330b7cannot remove - Container 668f62511f4aa62357269cd405cff1fbe295b7f6d5011e7cfed434e3072330b7 is not runningfailed to remove 668f62511f4aa62357269cd405cff1fbe295b7f6d5011e7cfed434e3072330b7So it looks like the removal fails because we couldn't kill the container
because it was already stopped, which may be a race condition where the first
check shows the container to be running (but may already be in process to be
removed or killed. In either case, we probably shouldn't fail the removal if
the container is already stopped.
This patch adds a
isNotRunning()utility, so that we can ignore this case,and proceed with the removal.
daemon: cleanupContainer: slightly cleanup error messages
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)