Skip to content

Conversation

@xiaoxubeii
Copy link
Contributor

Ⅰ. Describe what this PR did

Fix that ContainerManager.Create wouldn't remove snapshot when failed.

Ⅱ. Does this pull request fix one issue?

fixes #2061.

Ⅲ. Describe how you did it

Add a variable cleanups to hold cleanup function, and loop it when failed in defer.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@xiaoxubeii xiaoxubeii changed the title bugfix: fix that ContainerManager.Create wouldn't remove snapshot when failed WIP bugfix: fix that ContainerManager.Create wouldn't remove snapshot when failed Aug 13, 2018
@codecov-io
Copy link

codecov-io commented Aug 13, 2018

Codecov Report

Merging #2080 into master will increase coverage by 25.56%.
The diff coverage is 84.61%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #2080       +/-   ##
===========================================
+ Coverage   39.41%   64.97%   +25.56%     
===========================================
  Files         212      209        -3     
  Lines       16462    16239      -223     
===========================================
+ Hits         6488    10551     +4063     
+ Misses       8955     4375     -4580     
- Partials     1019     1313      +294
Flag Coverage Δ
#criv1alpha1test 33.37% <23.07%> (+25.15%) ⬆️
#criv1alpha2test 33.84% <23.07%> (?)
#integrationtest 39.97% <84.61%> (+0.56%) ⬆️
#unittest 23.79% <0%> (?)
Impacted Files Coverage Δ
daemon/mgr/container.go 55.97% <84.61%> (+2.78%) ⬆️
ctrd/events.go 75% <0%> (-6.82%) ⬇️
ctrd/client.go 56.87% <0%> (-0.21%) ⬇️
storage/volume/types/meta/meta.go 0% <0%> (ø) ⬆️
storage/volume/types/vars.go
storage/volume/types/meta/convert.go
storage/quota/set_diskquota.go
daemon/mgr/container_storage.go 49.48% <0%> (+0.51%) ⬆️
daemon/daemon.go 58.79% <0%> (+1.09%) ⬆️
ctrd/container.go 43.5% <0%> (+1.44%) ⬆️
... and 105 more

@xiaoxubeii xiaoxubeii changed the title WIP bugfix: fix that ContainerManager.Create wouldn't remove snapshot when failed bugfix: fix that ContainerManager.Create wouldn't remove snapshot when failed Aug 13, 2018
@allencloud
Copy link
Collaborator

allencloud commented Aug 13, 2018

Thanks a lot for your contribution. Just double check could we add an integration test for this case? @xiaoxubeii @rudyfly

@xiaoxubeii
Copy link
Contributor Author

@allencloud OK, i will add a new integration testcase.

@allencloud
Copy link
Collaborator

OK, i will add a new integration testcase.

That could not be better. Thanks a lot. @xiaoxubeii

@xiaoxubeii
Copy link
Contributor Author

@allencloud I find it is a little difficult to add a integration testcase of snapshot cleanup. That is because there is no api interface to list snapshots and i also cannot access containerd directly because of lacking the configuration. Any idea can help me to test?

@allencloud allencloud added the kind/bug This is bug report for project label Aug 14, 2018
return nil, err
}
cleanups = append(cleanups, func() {
mgr.Client.RemoveSnapshot(ctx, id)
Copy link
Contributor

Choose a reason for hiding this comment

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

could we add some log here?

@pouchrobot
Copy link
Collaborator

@xiaoxubeii Thanks for your contribution. 🍻
Please sign off in each of your commits.

@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Aug 14, 2018
@allencloud
Copy link
Collaborator

Thanks for your great work. 🍺 @xiaoxubeii

@allencloud allencloud merged commit 199d907 into AliyunContainerService:master Aug 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug This is bug report for project LGTM one maintainer or community participant agrees to merge the pull reuqest. size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bugfix: ContainerManager create haven't deal with error

5 participants