Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

v2: Open log fifo with RDWR instead of WRONLY#2591

Merged
grahamwhaley merged 1 commit into
kata-containers:masterfrom
darfux:change_log_outpipe_to_rdwr
Apr 14, 2020
Merged

v2: Open log fifo with RDWR instead of WRONLY#2591
grahamwhaley merged 1 commit into
kata-containers:masterfrom
darfux:change_log_outpipe_to_rdwr

Conversation

@darfux
Copy link
Copy Markdown
Contributor

@darfux darfux commented Apr 7, 2020

The container log fifo is opened as O_WRONLY now. When the read side of fifo is closed temporarily such as restarting contaienrd, write to tty.Stdout will get an EPIPE error and finally cause io.CopyBuffer return. Then ioCopy closes the tty io and exits. Thus after containerd restarted, the log fifo can't be reopened. The container will be blocked forever after stdout/stderr buffer is full.

Opening the log fifo with RDWR instead of WRONLY avoids the fifo returning EPIPE when the read side is closed, and keeps the fifo open until the reader reopening it.

Fixes: #2590

Signed-off-by: Li Yuxuan liyuxuan04@baidu.com

@amshinde
Copy link
Copy Markdown
Member

amshinde commented Apr 7, 2020

Nice catch @darfux!

@grahamwhaley
Copy link
Copy Markdown
Contributor

/test
Much though I'd like to say we should have a test added for this as well, that might not be trivial to construct and test?

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 7, 2020

Codecov Report

Merging #2591 into master will increase coverage by 0.08%.
The diff coverage is 29.41%.

@@            Coverage Diff             @@
##           master    #2591      +/-   ##
==========================================
+ Coverage   50.47%   50.55%   +0.08%     
==========================================
  Files         118      118              
  Lines       17016    17030      +14     
==========================================
+ Hits         8588     8610      +22     
+ Misses       7387     7372      -15     
- Partials     1041     1048       +7     

@darfux darfux force-pushed the change_log_outpipe_to_rdwr branch 2 times, most recently from 5db9db5 to 5d3029e Compare April 7, 2020 12:03
@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Apr 7, 2020

Hi @grahamwhaley . I've added a test for newTtyIO, PTAL 😃

@grahamwhaley
Copy link
Copy Markdown
Contributor

/test
nice - let's have @amshinde and @lifupan review that - thanks!!!

Copy link
Copy Markdown

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

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

Thanks @darfux.

lgtm

@Pennyzct
Copy link
Copy Markdown
Contributor

Pennyzct commented Apr 8, 2020

/test

1 similar comment
@lifupan
Copy link
Copy Markdown
Member

lifupan commented Apr 9, 2020

/test

@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Apr 9, 2020

I wonder why the travis-ci is always in pending state. Is there something wrong with it...?

@darfux darfux force-pushed the change_log_outpipe_to_rdwr branch from 5d3029e to 039c46a Compare April 9, 2020 15:26
@jodh-intel
Copy link
Copy Markdown

jodh-intel commented Apr 9, 2020

It looks like Travis passed, but must have failed to send that result back to GitHub. I've restarted the Travis job so hopefully it will work this time...

@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Apr 9, 2020

repushed to see whether the travis-ci can be triggered 👀

@jodh-intel
Copy link
Copy Markdown

s/Jenkins/Travis/.

@darfux - ah thanks ;)

@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Apr 9, 2020

@jodh-intel Thanks for explain XD

The container log fifo is opened as `O_WRONLY` now. When the read side
of fifo is closed temporarily such as restarting contaienrd, write to
`tty.Stdout` will get an EPIPE error and finally cause `io.CopyBuffer`
return. Then `ioCopy` closes the tty io and exits. Thus after containerd
restarted, the log fifo can't be reopened. The container will be blocked
forever after stdout/stderr buffer is full.

Opening the log fifo with `RDWR` instead of `WRONLY` avoids the fifo
returning EPIPE when the read side is closed, and keeps the fifo open
until the reader reopening it.

Fixes: kata-containers#2590

Signed-off-by: Li Yuxuan <liyuxuan04@baidu.com>
@darfux darfux force-pushed the change_log_outpipe_to_rdwr branch from 039c46a to 8e0f891 Compare April 10, 2020 06:59
@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Apr 10, 2020

Hi @jodh-intel . I've repushed twice but it seems that Travis still failed to send the result back😂 Is there any way else to fix it?

@devimc
Copy link
Copy Markdown

devimc commented Apr 13, 2020

@darfux please try closing and reopening this PR

@darfux darfux closed this Apr 13, 2020
@darfux darfux reopened this Apr 13, 2020
@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Apr 13, 2020

thanks @devimc

@devimc
Copy link
Copy Markdown

devimc commented Apr 13, 2020

ouch! travis still stuck 😒

@grahamwhaley
Copy link
Copy Markdown
Contributor

Travis looks to have ack'd now - there are 2 other CI fails - expected??

@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Apr 14, 2020

@grahamwhaley
Copy link
Copy Markdown
Contributor

sure, two CIs restarted...

@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Apr 14, 2020

Thanks @grahamwhaley . All CIs are happy now 🎊

@grahamwhaley grahamwhaley merged commit 0fe23c8 into kata-containers:master Apr 14, 2020
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.

Container log pipe was closed after containerd restarting

7 participants