Skip to content

Ignore fifo error when using v2 multi-container shim#3536

Merged
crosbymichael merged 1 commit intocontainerd:masterfrom
darfux:ignore_closed_fifo_error_under_multi_cntr
Aug 19, 2019
Merged

Ignore fifo error when using v2 multi-container shim#3536
crosbymichael merged 1 commit intocontainerd:masterfrom
darfux:ignore_closed_fifo_error_under_multi_cntr

Conversation

@darfux
Copy link
Copy Markdown
Contributor

@darfux darfux commented Aug 15, 2019

When using a multi-container shim, the fifo of the 2nd to Nth container will not be opened when the ctx is done. This will cause a "closed fifo" error that can be ignored.

Signed-off-by: Li Yuxuan [email protected]

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Aug 15, 2019

Build succeeded.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Aug 15, 2019

Codecov Report

Merging #3536 into master will decrease coverage by 2.72%.
The diff coverage is 78.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3536      +/-   ##
==========================================
- Coverage   44.94%   42.21%   -2.73%     
==========================================
  Files         115      124       +9     
  Lines       12790    13692     +902     
==========================================
+ Hits         5748     5780      +32     
- Misses       6170     7039     +869     
- Partials      872      873       +1
Flag Coverage Δ
#linux 45.72% <63.63%> (-3.06%) ⬇️
#windows 37.27% <66.66%> (-2.92%) ⬇️
Impacted Files Coverage Δ
runtime/v2/binary.go 0% <0%> (ø)
runtime/v2/shim_windows.go 17.77% <100%> (ø)
runtime/v2/shim_unix.go 77.77% <100%> (ø)
runtime/v2/manager.go 4.71% <0%> (ø)
runtime/v2/process.go 0% <0%> (ø)
runtime/v2/shim.go 0% <0%> (ø)
runtime/v2/bundle.go 0% <0%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c62b744...04caf1f. Read the comment docs.

@crosbymichael
Copy link
Copy Markdown
Member

LGTM

Comment thread runtime/v2/binary.go Outdated
@darfux darfux force-pushed the ignore_closed_fifo_error_under_multi_cntr branch from bc690df to 751c1b6 Compare August 16, 2019 17:18
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Aug 16, 2019

Build succeeded.

@darfux darfux force-pushed the ignore_closed_fifo_error_under_multi_cntr branch from 751c1b6 to ed75717 Compare August 16, 2019 17:40
@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Aug 16, 2019

I'll do the rebase after #3545 has been merged

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Aug 16, 2019

Build succeeded.

@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Aug 16, 2019

seems that the CI of Windows is still not happy with fifo...working on it

@darfux darfux force-pushed the ignore_closed_fifo_error_under_multi_cntr branch 2 times, most recently from 6ae8f26 to f311ee6 Compare August 16, 2019 18:25
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Aug 16, 2019

Build succeeded.

@darfux darfux force-pushed the ignore_closed_fifo_error_under_multi_cntr branch 2 times, most recently from 6047504 to 0829e71 Compare August 16, 2019 19:25
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Aug 16, 2019

Build succeeded.

@darfux darfux force-pushed the ignore_closed_fifo_error_under_multi_cntr branch from 0829e71 to ec76380 Compare August 16, 2019 19:39
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Aug 16, 2019

Build succeeded.

@darfux darfux force-pushed the ignore_closed_fifo_error_under_multi_cntr branch from ec76380 to 754a911 Compare August 16, 2019 19:57
@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Aug 16, 2019

seems that the CI of Windows is still not happy with fifo...working on it

Resolved by using separate check functions between unix and windows. I'm not sure it is ok to do this, but it seems that the ErrNotExist will not occur under unix as we open the fifo with O_CREAT.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Aug 16, 2019

Build succeeded.

@darfux darfux force-pushed the ignore_closed_fifo_error_under_multi_cntr branch from 754a911 to ef29d86 Compare August 16, 2019 23:51
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Aug 17, 2019

Build succeeded.

@estesp estesp changed the title Ignore fifo error when using v2 mutil-container shim Ignore fifo error when using v2 multi-container shim Aug 17, 2019
When using a multi-container shim, the fifo of the 2nd to Nth container
will not be opened when the ctx is done. This will cause an
`ErrReadClosed` that can be ignored.

Signed-off-by: Li Yuxuan <[email protected]>
@darfux darfux force-pushed the ignore_closed_fifo_error_under_multi_cntr branch from ef29d86 to 04caf1f Compare August 17, 2019 01:40
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Aug 17, 2019

Build succeeded.

@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Aug 17, 2019

@estesp thanks for correcting the title! I've corrected the commit title as well :) And it seems that all the CI would be happy now😃

Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@crosbymichael
Copy link
Copy Markdown
Member

LGTM

@crosbymichael crosbymichael merged commit 6cb56bb into containerd:master Aug 19, 2019
@darfux darfux deleted the ignore_closed_fifo_error_under_multi_cntr branch August 22, 2019 15:13
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.

5 participants