Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix: umount rootfs when delete a container #2196

Merged

Conversation

HusterWan
Copy link
Contributor

@HusterWan HusterWan commented Sep 4, 2018

Signed-off-by: Michael Wan zirenwan@gmail.com

Ⅰ. Describe what this PR did

When using PouchContainer to replace the old container engine in Alibaba Group, we must take over the old running containers. Maybe somebody has known that we use d2p-migrator to convert those old containers to pouch containers

Since those old containers are not created by pouchd, but taking over by specified a rootfs for container, the remove action is different from normal containers. We no need to delete the snapshot of container because old containers have no snapshots, then we also should umount the rootfs when delete the container.

Ⅱ. Does this pull request fix one issue?

None

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

It is difficult to imitate the situation of taking over containers, So i will add test case in d2p test.

Ⅳ. Describe how to verify it

None

Ⅴ. Special notes for reviews

None

@pouchrobot pouchrobot added kind/bug This is bug report for project size/XS labels Sep 4, 2018
@codecov-io
Copy link

codecov-io commented Sep 4, 2018

Codecov Report

Merging #2196 into master will decrease coverage by 0.11%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2196      +/-   ##
=========================================
- Coverage   65.01%   64.9%   -0.12%     
=========================================
  Files         208     208              
  Lines       16732   16734       +2     
=========================================
- Hits        10879   10861      -18     
- Misses       4504    4516      +12     
- Partials     1349    1357       +8
Flag Coverage Δ
#criv1alpha1test 32.55% <0%> (-0.4%) ⬇️
#criv1alpha2test 33.47% <0%> (+0.1%) ⬆️
#integrationtest 39.95% <0%> (-0.03%) ⬇️
#unittest 23.94% <0%> (-0.01%) ⬇️
Impacted Files Coverage Δ
daemon/mgr/container.go 56.14% <0%> (-0.84%) ⬇️
apis/server/utils.go 61.9% <0%> (-4.77%) ⬇️
ctrd/watch.go 75.75% <0%> (-4.55%) ⬇️
ctrd/container.go 52.87% <0%> (-0.96%) ⬇️
cri/v1alpha1/cri.go 62.39% <0%> (-0.35%) ⬇️

} else {
// if creating the container by specify a rootfs,
// we should umount the mounted rootfs when delete it.
if err := mount.Unmount(c.BaseFS, 0); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please combine the else and if. I think that is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

…nt the mounted rootfs when delete it.

Signed-off-by: Michael Wan <zirenwan@gmail.com>
@allencloud
Copy link
Collaborator

LGTM

@allencloud allencloud merged commit ff5d11f into AliyunContainerService:master Sep 4, 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 size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants