Skip to content

v2: Cancel shim log ctx when ttrpc is closed#4046

Merged
estesp merged 1 commit intocontainerd:masterfrom
darfux:cancel_shim_log_ctx_by_onclose
Feb 20, 2020
Merged

v2: Cancel shim log ctx when ttrpc is closed#4046
estesp merged 1 commit intocontainerd:masterfrom
darfux:cancel_shim_log_ctx_by_onclose

Conversation

@darfux
Copy link
Contributor

@darfux darfux commented Feb 20, 2020

The background context aovids shim blocking when the ctx is cancelled unexpectedly during shim start (#3921). But if the shim exits unexpectedly before opening the pipe, the fd will never be closed. onCloseWithShimLog makes sure that the shim log fd is closed properly once the shim disconnects.

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

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 20, 2020

Build succeeded.

@darfux darfux force-pushed the cancel_shim_log_ctx_by_onclose branch from 8bb3ad7 to 0ded0f6 Compare February 20, 2020 14:15
@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 20, 2020

Build succeeded.

@darfux darfux force-pushed the cancel_shim_log_ctx_by_onclose branch from 0ded0f6 to 8bf491f Compare February 20, 2020 15:18
The background context aovids shim blocking when the ctx is cancelled
unexpectedly during shim start. But if the shim exits unexpectedly
before opening the pipe, the fd will never be closed.
`onCloseWithShimLog` makes sure that the shim log fd is closed properly
once the shim disconnects.

Signed-off-by: Li Yuxuan <liyuxuan04@baidu.com>
@darfux darfux force-pushed the cancel_shim_log_ctx_by_onclose branch from 8bf491f to 84464b8 Compare February 20, 2020 15:20
@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 20, 2020

Build succeeded.

@codecov-io
Copy link

codecov-io commented Feb 20, 2020

Codecov Report

Merging #4046 into master will decrease coverage by 3.42%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4046      +/-   ##
==========================================
- Coverage    46.1%   42.67%   -3.43%     
==========================================
  Files         117      130      +13     
  Lines       11837    14778    +2941     
==========================================
+ Hits         5457     6307     +850     
- Misses       5462     7552    +2090     
- Partials      918      919       +1
Flag Coverage Δ
#linux 46.07% <0%> (-0.03%) ⬇️
#windows 38.26% <0%> (?)
Impacted Files Coverage Δ
runtime/v2/binary.go 0% <0%> (ø) ⬆️
archive/tar_opts.go 58.82% <0%> (-12.61%) ⬇️
snapshots/native/native.go 42.42% <0%> (-10.27%) ⬇️
archive/tar.go 48.58% <0%> (-8.63%) ⬇️
cio/io.go 46.47% <0%> (-8.53%) ⬇️
metadata/snapshot.go 47.57% <0%> (-7.2%) ⬇️
metadata/containers.go 49.84% <0%> (-6.26%) ⬇️
content/local/writer.go 58.65% <0%> (-5.55%) ⬇️
content/local/store.go 49.52% <0%> (-5.29%) ⬇️
metadata/images.go 57.41% <0%> (-4.97%) ⬇️
... and 75 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 986d067...84464b8. Read the comment docs.

Copy link
Contributor

@jterry75 jterry75 left a comment

Choose a reason for hiding this comment

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

LGTM

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

@estesp estesp merged commit 0e08405 into containerd:master Feb 20, 2020
@estesp
Copy link
Member

estesp commented Feb 21, 2020

This looks like a bug fix that would apply to release/1.2 and release/1.3; marking for cherry-pick; notify @containerd/containerd-maintainers in case anyone else has an opinion.

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.

4 participants