-
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
bug:fix fd doesn't close in time #4404
Conversation
Signed-off-by: ningmingxiao <ning.mingxiao@zte.com.cn>
4a4a327
to
9a13e2d
Compare
Is it a bug or a special design? run c start will hang until begin to run system.Exec. Can you review my pr thank you. @kolyshkin @cyphar |
@@ -260,6 +260,7 @@ func (l *linuxStandardInit) Init() error { | |||
return &os.PathError{Op: "write exec fifo", Path: fifoPath, Err: err} | |||
} | |||
|
|||
_ = unix.Close(fd) |
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.
I don't think it's a bug. If you look at the code, you'll see that the parent process uses handleFifoResult
to read all data from the fifo and (once it's closed from the other side) remove it.
Now, the presence of exec.fifo
file is used to distinguish between the created and the running state, as you can see in (*container).RefreshState
function.
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.
Obviously, we don't have any tests to check if the container state is reported correctly for the case when the startContainer hook is still running (which is what this PR essentially breaks).
If you have some time, feel free to write such a test @ningmingxiao
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.
Is it necessary to call "unix.Close(fd)" before to run startcontainer hook?
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.
Is it necessary to call "unix.Close(fd)" before to run startcontainer hook?
No, it's not necessary. As I explained above, it only affects that the container status will be. Once it's closed, the runc status
will show "running". This is why, I think, this commit is a mistake.
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.
thank you.
I add some sleep
and find io.ReadAll(execFifo) will hang 30s.