bugfix: delete upperDir and workDir when deleting container taken over by pouch #2524
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #2524 +/- ##
==========================================
- Coverage 69.2% 69.07% -0.14%
==========================================
Files 278 278
Lines 18494 18694 +200
==========================================
+ Hits 12798 12912 +114
- Misses 4243 4309 +66
- Partials 1453 1473 +20
|
HusterWan
force-pushed
the
zr/del-upperDir
branch
from
a11a875
to
d383214
Compare
zhuangqh
reviewed
Dec 5, 2018
|
|
||
| if v, ok := c.Snapshotter.Data["WorkDir"]; ok { | ||
| workDir = v | ||
| } |
There was a problem hiding this comment.
What about refactor it to
if c.Snapshotter == nil || c.Snapshotter.Data == nil {
return
}
for _, dir := range []string{"MergedDir", "UpperDir", "WorkDir"} {
...
v, ok := c.Snapshotter.Data[dir]
if !ok {
continue
}
...
}
There was a problem hiding this comment.
@zhuangqh Your code haven't unlocked.
There was a problem hiding this comment.
I think the better codes like these:
if c.Snapshotter == nil || c.Snapshotter.Data == nil {
return
}
lock()
removeDirs []string
for _, dir := range []string{"MergedDir", "WorkDir", "UpperDir"} {
...
v, ok := c.Snapshotter.Data[dir]
if !ok {
continue
}
removeDirs = append(removeDir, v)
}
unlock()
for _, dir := range removeDirs {
os.Remove(...)
}
There was a problem hiding this comment.
@rudyfly I know. I just briefly point out how to refactor the code structure.
rudyfly
reviewed
Dec 5, 2018
daemon/mgr/container_types.go
Outdated
| // WorkDir. Since the snapshot of container created by containerd will be cleaned by | ||
| // containerd, so we only clean rootfs that is RootFSProvided. | ||
| func (c *Container) CleanRootfsSnapshotDirs() error { | ||
| // if RootFSProvided is set, we no need clean the rootfs |
There was a problem hiding this comment.
The comment is is not set?
|
|
||
| if v, ok := c.Snapshotter.Data["WorkDir"]; ok { | ||
| workDir = v | ||
| } |
There was a problem hiding this comment.
@zhuangqh Your code haven't unlocked.
daemon/mgr/container_types.go
Outdated
| @@ -491,6 +492,51 @@ func (c *Container) GetSpecificBasePath(path string) string { | |||
| return "" | |||
| } | |||
|
|
|||
| // CleanRootfsSnapshotDirs delete container's rootfs snapshot MergedDir, UpperDir and | |||
|
|
||
| if v, ok := c.Snapshotter.Data["WorkDir"]; ok { | ||
| workDir = v | ||
| } |
There was a problem hiding this comment.
I think the better codes like these:
if c.Snapshotter == nil || c.Snapshotter.Data == nil {
return
}
lock()
removeDirs []string
for _, dir := range []string{"MergedDir", "WorkDir", "UpperDir"} {
...
v, ok := c.Snapshotter.Data[dir]
if !ok {
continue
}
removeDirs = append(removeDir, v)
}
unlock()
for _, dir := range removeDirs {
os.Remove(...)
}
d383214
to
61c7e8e
Compare
…r by pouch Signed-off-by: ziren.wzr <ziren.wzr@alibaba-inc.com>
HusterWan
force-pushed
the
zr/del-upperDir
branch
from
61c7e8e
to
30fd855
Compare
zhuangqh
approved these changes
Dec 12, 2018
|
LGTM |
pouchrobot
added
the
LGTM
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: ziren.wzr ziren.wzr@alibaba-inc.com
Ⅰ. Describe what this PR did
When using d2p-migrator to upgrade docker to PouchContainer, we take over the old container's network, volumes, but the snapshot of an old container is not charged by pouchd.
So when we delete the old containers, the
UpperDirandWorkDirstill stay on the host, they will cost many disk space. Moreover, if the directories have disk quota id, it will make the new containers used the same disk quota id cannot use the total storage that they should. we can make it more clear:The old
UpperDirthat has disk quota id 1111 costs 20G disk space, if we create a new container with the same disk quota id 1111 and intend to assign 100G disk space. Since the oldUpperDirhas occupied 20G, so the new container actually just can use 80G disk space.Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews