-
Notifications
You must be signed in to change notification settings - Fork 191
Conversation
daemon/pod/container.go
Outdated
type stdWriteCloser struct { | ||
io.Writer | ||
io.Closer | ||
} |
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.
LGTM |
retest this please, hykins |
on the result of hykins -- http://ci.hypercontainer.io:8080/job/hyperd-auto/404/console I am afraid that this PR will affect exec output in some case. We need make it clear before merge it. |
Read again, looks this PR has no effect on exec... |
I think the failure of http://ci.hypercontainer.io:8080/job/hyperd-auto/404/console is because the exec exit before the tty stream is finished. And it does not related with this PR. |
daemon/pod/container.go
Outdated
stderr = stdcopy.NewStdWriter(stdout, stdcopy.Stderr) | ||
stdout = stdcopy.NewStdWriter(stdout, stdcopy.Stdout) | ||
stderr = &writeCloser{stdcopy.NewStdWriter(stdout, stdcopy.Stderr), stdout} | ||
stdout = &writeCloser{stdcopy.NewStdWriter(stdout, stdcopy.Stdout), stdout} |
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.
will this cause double Close()
of stdout
? And does this hurt?
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.
hmm, good point! Impact of a second call to Close() is undefined. We should avoid it IMO.
test exceed 40 minutes and is killed by hykins. retest this please, hykins |
@bergwolf I think the
|
The container logger copier expect to read EOF so that it can determine the container output stream has ended. If we do not close stdout/stderr streams, the logger copier will continue waiting for a new liner to stop reading. When container stops and closes the logger, container logs are lost forever. Signed-off-by: Peng Tao <[email protected]>
Signed-off-by: Peng Tao <[email protected]>
Embedded types are enough here. Signed-off-by: Peng Tao <[email protected]>
LGTM again |
LGTM |
The container logger copier expect to read EOF so that it can determine
the container output stream has ended. If we do not close stdout/stderr
streams, the logger copier will continue waiting for a new liner to stop
reading. When container stops and closes the logger, container logs are
lost forever.
fixes: #577