bugfix: clean up the container if the creation fails #2153
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2153 +/- ##
==========================================
+ Coverage 64.16% 64.28% +0.11%
==========================================
Files 209 209
Lines 16713 16725 +12
==========================================
+ Hits 10724 10751 +27
+ Misses 4648 4641 -7
+ Partials 1341 1333 -8
|
| @@ -550,6 +550,16 @@ func (c *CriManager) CreateContainer(ctx context.Context, r *runtime.CreateConta | |||
|
|
|||
| containerID := createResp.ID | |||
|
|
|||
| defer func() { | |||
There was a problem hiding this comment.
Do we need to put this cleanup in daemon?
There was a problem hiding this comment.
It is easier to handle for cri manager, in addition, it also guarantees the atomicity of the function Create in daemon. WDYT?
There was a problem hiding this comment.
I do not think we should put this in container_manager. @Ace-Tang
There was a problem hiding this comment.
What will daemon do if create container fail? do a container data leaving?
There was a problem hiding this comment.
@starnop , Yes, I just wonder if we not create through cri interface...
There was a problem hiding this comment.
What will daemon do if create container fail? do a container data leaving?
Yeah, just leave it. While for an upper caller, I think if there is an API to provide cleanup action, then call the clean it up.
There was a problem hiding this comment.
Sure, make sense, still wonder who do this caller if not cri interface.
|
LGTM, while I still would like to invite @fuweid to take a look at this. |
pouchrobot
added
the
LGTM
cri/v1alpha2/cri.go
Outdated
| @@ -502,7 +502,7 @@ func (c *CriManager) ListPodSandbox(ctx context.Context, r *runtime.ListPodSandb | |||
| } | |||
|
|
|||
| // CreateContainer creates a new container in the given PodSandbox. | |||
| func (c *CriManager) CreateContainer(ctx context.Context, r *runtime.CreateContainerRequest) (*runtime.CreateContainerResponse, error) { | |||
| func (c *CriManager) CreateContainer(ctx context.Context, r *runtime.CreateContainerRequest) (_ *runtime.CreateContainerResponse, retErr error) { | |||
There was a problem hiding this comment.
could we use var err error to make it readable? because it is hack to use return to assign error to retErr.
Signed-off-by: Starnop <starnop@163.com>
Signed-off-by: Starnop starnop@163.com
Ⅰ. Describe what this PR did
If the container failed to be created, we should clean up the container.
Ⅱ. Does this pull request fix one issue?
None.
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews