Skip to content

v2: Call shim.Delete at first when create is failed#3921

Merged
estesp merged 1 commit intocontainerd:masterfrom
darfux:v2_try_to_delete_shim_when_create_fail
Dec 31, 2019
Merged

v2: Call shim.Delete at first when create is failed#3921
estesp merged 1 commit intocontainerd:masterfrom
darfux:v2_try_to_delete_shim_when_create_fail

Conversation

@darfux
Copy link
Contributor

@darfux darfux commented Dec 27, 2019

If the context is cancelled during shim.Create(), such as the client disconnects unexpectedly. The created shim will never be deleted.
What's more, if the context is cancelled during openShimLog(), the fifo will be closed and block the shim output.

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

If the context is cancelled during `shim.Create()`, such as the client
disconnects unexpectedly. The created shim will never be deleted.
What's more, if the context is cancelled during `openShimLog()`, the
fifo will be closed and block the shim output.

Signed-off-by: Li Yuxuan <liyuxuan04@baidu.com>
@darfux darfux force-pushed the v2_try_to_delete_shim_when_create_fail branch from be323b2 to d82fa43 Compare December 27, 2019 16:02
@theopenlab-ci
Copy link

theopenlab-ci bot commented Dec 27, 2019

Build succeeded.

@codecov-io
Copy link

Codecov Report

Merging #3921 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3921      +/-   ##
==========================================
- Coverage   42.43%   42.42%   -0.02%     
==========================================
  Files         130      130              
  Lines       14706    14710       +4     
==========================================
  Hits         6241     6241              
- Misses       7544     7548       +4     
  Partials      921      921
Flag Coverage Δ
#linux 45.81% <0%> (-0.02%) ⬇️
#windows 37.94% <0%> (-0.02%) ⬇️
Impacted Files Coverage Δ
runtime/v2/manager.go 4.67% <0%> (-0.09%) ⬇️
runtime/v2/shim.go 1.16% <0%> (ø) ⬆️
runtime/v2/binary.go 0% <0%> (ø) ⬆️

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 6b94b64...d82fa43. Read the comment docs.

@crosbymichael
Copy link
Member

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 537afb1 into containerd:master Dec 31, 2019
return nil, err
}
f, err := openShimLog(ctx, b.bundle, client.AnonDialer)
f, err := openShimLog(context.Background(), b.bundle, client.AnonDialer)
Copy link
Contributor

Choose a reason for hiding this comment

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

@darfux: This change broke support on Windows, as openShimLog in shim_windows.go requires a Namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for missing the windows condition😂. I'm trying to fix it on #3929, PTAL

darfux added a commit to darfux/containerd that referenced this pull request Jan 5, 2020
Related to
containerd#3921 (comment)

Signed-off-by: Li Yuxuan <liyuxuan04@baidu.com>
tussennet pushed a commit to tussennet/containerd that referenced this pull request Sep 11, 2020
Related to
containerd#3921 (comment)

Signed-off-by: Li Yuxuan <liyuxuan04@baidu.com>
thaJeztah pushed a commit to thaJeztah/containerd that referenced this pull request Nov 19, 2020
Related to
containerd#3921 (comment)

Signed-off-by: Li Yuxuan <liyuxuan04@baidu.com>
(cherry picked from commit 1fb1d93)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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