-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
return error if libcontainer desroy failed #4090
Conversation
/cc @kolyshkin @lifubang |
Please fix the lint error, you can see 48ff07c |
fd712f2
to
79e0ea4
Compare
Done. |
@@ -18,7 +18,7 @@ func killContainer(container *libcontainer.Container) error { | |||
for i := 0; i < 100; i++ { | |||
time.Sleep(100 * time.Millisecond) | |||
if err := container.Signal(unix.Signal(0)); err != nil { | |||
destroy(container) | |||
_ = destroy(container) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ignore the error here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just keep others unchanged to make the minimum changes.
Another thing, please use |
Yes, I got it. The merge main commit is generated automatically by an "Update" button on Github pages. I should not click it the next time I saw it. |
Signed-off-by: Zhang Tianyang <burning9699@gmail.com>
82842b3
to
3c39079
Compare
@lifubang Can you help retry the ci? |
@lifubang @kolyshkin Does this pr need to be improved? I'm more concerned about the feasibility of the repair, not the time being merged. |
Ping @lifubang @kolyshkin |
Sorry, we are still in discussion. |
Yes, I think we should do this. I have written a similar commit in #4102 and put you (@Burning1020) as a co-author as you wrote it first; PTAL. |
Closing in favor of #4102 |
If there is an error in the destroy of libcontainer, it should be returned immediately and let the upper caller decided how to handle it.
Fix #4040