*: fix leaked shim caused by high IO pressure #8954
Merged
Conversation
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
Signed-off-by: Wei Fu <[email protected]>
Fixes: containerd#7496 containerd#8931 Signed-off-by: Wei Fu <[email protected]>
Fixes: containerd#7496 containerd#8931 Signed-off-by: Wei Fu <[email protected]>
Since the moby/moby can't handle duplicate exit event well, it's hard for containerd to retry shutdown if there is error, like context canceled. In order to prevent from regression like containerd#4769, I add skipped integration case as TODO item and we should rethink about how to handle the task/shim lifecycle. Signed-off-by: Wei Fu <[email protected]>
Signed-off-by: Wei Fu <[email protected]>
cpuguy83
approved these changes
Aug 15, 2023
Member
cpuguy83
left a comment
cpuguy83
left a comment
There was a problem hiding this comment.
LGTM
I have a feeling moby needs a similar patch.
Member
Author
Basically, yes. The leaky shim can be cleanup after containerd restart by the way. Thanks for the review. |
dmcgowan
approved these changes
Aug 16, 2023
fuweid
added
cherry-pick/1.6.x
cherry-pick/1.7.x
This was referenced Aug 17, 2023
fuweid
added
cherry-picked/1.6.x
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
integration: add case to reproduce #7496
When the shim unmounts overlayfs rootfs, kernel will force syncfs if there is no volatile option. In order to reproduce the high IO pressure, this patch uses
straceto delay theumount2syscall.Fixes: #7496 #8931
integration: add ShouldRetryShutdown case based on #7496
Within current design, if the shim is killed before task-service.Delete API call, the callback on connect close will send
137exit code because the callback doesn't have any context about container's exit code.containerd/runtime/v2/shim.go
Lines 170 to 184 in 70a2c95
And the moby/moby can't handle duplicate exit event well. Let's say that the moby receives exit code 0 at first and then the duplicate exit event with different code 137 can override 0, as the #4769 described.
I think the best solution to avoid leaky issue is to redesign the task-service.Delete API, remove the async callback and then let the caller retry. Since the task in shim can't be restart, we can cache the exit code in container bundle so that the shim.Delete binary call and read it and return exit code correctly. However, it doesn't work with running shim server.
In order to prevent from regression like #4769, I add skipped
integration case as TODO item and we should rethink about how to handle
the task/shim lifecycle.
@mikebrow @dmcgowan @mxpv @AkihiroSuda @thaJeztah @laurazard