Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ningmingxiao
Copy link

@ningmingxiao ningmingxiao commented Sep 16, 2024

I add some sleep

	s := l.config.SpecState
	s.Pid = unix.Getpid()
	s.Status = specs.StateCreated
	if err := l.config.Config.Hooks.Run(configs.StartContainer, s); err != nil {
		return err
	}
	time.Sleep(time.Second*30)

and find io.ReadAll(execFifo) will hang 30s.

func readFromExecFifo(execFifo io.Reader) error {
	data, err := io.ReadAll(execFifo)
	if err != nil {
		return err
	}
	if len(data) <= 0 {
		return errors.New("cannot start an already running container")
	}
	return nil
}

Signed-off-by: ningmingxiao <ning.mingxiao@zte.com.cn>
@ningmingxiao ningmingxiao changed the title fix fd doesn't in time bug:fix fd doesn't close in time Sep 16, 2024
@ningmingxiao
Copy link
Author

ningmingxiao commented Sep 16, 2024

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)
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants