Skip to content
This repository has been archived by the owner on Feb 8, 2021. It is now read-only.

Outstream #627

Merged
merged 3 commits into from
May 8, 2017
Merged

Outstream #627

merged 3 commits into from
May 8, 2017

Conversation

bergwolf
Copy link
Member

@bergwolf bergwolf commented May 5, 2017

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

type stdWriteCloser struct {
io.Writer
io.Closer
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gnawux
Copy link
Member

gnawux commented May 5, 2017

LGTM

@gnawux
Copy link
Member

gnawux commented May 5, 2017

retest this please, hykins

@gnawux
Copy link
Member

gnawux commented May 5, 2017

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.

@gnawux
Copy link
Member

gnawux commented May 5, 2017

Read again, looks this PR has no effect on exec...

@gnawux
Copy link
Member

gnawux commented May 5, 2017

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.

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}
Copy link
Member

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?

Copy link
Member Author

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.

@gnawux
Copy link
Member

gnawux commented May 5, 2017

test exceed 40 minutes and is killed by hykins. retest this please, hykins

@gnawux
Copy link
Member

gnawux commented May 6, 2017

@bergwolf I think the writeMultiCloser is too complicated. I think just like exec does is enough ---

  • use io.Writer as signature instead of io.WriteCloser
  • at the end, try cast to io.Closer, and Close() only if it is a Closer.
  • pass stdout as writeCloser, and stderr as pure Writer

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 <bergwolf@gmail.com>
Signed-off-by: Peng Tao <bergwolf@gmail.com>
Embedded types are enough here.

Signed-off-by: Peng Tao <bergwolf@gmail.com>
@gnawux
Copy link
Member

gnawux commented May 7, 2017

LGTM again

@laijs
Copy link
Contributor

laijs commented May 8, 2017

LGTM

@laijs laijs merged commit 8c4a2b4 into hyperhq:master May 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No content returned by cat command
3 participants