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: should add snapshot gc label to config #2635

Merged
merged 1 commit into from
Jan 2, 2019
Merged

bugfix: should add snapshot gc label to config #2635

merged 1 commit into from
Jan 2, 2019

Conversation

fuweid
Copy link
Contributor

@fuweid fuweid commented Dec 29, 2018

Ⅰ. Describe what this PR did

# containerd content/snapshot gc reference overview
+----------+        +-------------+     +-----------------+
| manifest +------> | config blob +---> | rootfs snapshot |
+-----+----+        +-------------+     +-----------------+
      |             +-------------+
      +-----------> | layer blob  |
                    +-------------+

For containerd.io/gc.root label means the content/snapshot cannot be
removed during gc. If we use it for snapshot, the snapshot cannot be
removed during removing image.

By default, we should add
containerd.io/gc.ref.snapshot.{snapshot-name} gc label to the image
config so that gc can remove the snapshot if user tries to remove the
image.

Signed-off-by: Wei Fu fuweid89@gmail.com

Ⅱ. Does this pull request fix one issue?

close #2556

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

no need

Ⅳ. Describe how to verify it

# create container
$ sudo pouch run -d --name test1 busybox sh -c "echo 1 > /tmp/1 && sleep 1000"
be0d930081334dfc409e96aa1ec7184c89de3357be82ec28febba09921ee64a8

# commit it
$ sudo pouch commit test1 hello:1
b57ae9798515

# run it make sure that it is correct.
$ sudo pouch run --rm hello:1 sh -c "cat /tmp/1"
1

# check content gc label use by image id(= config digest)
$ sudo ctr -a /var/run/containerd.sock content ls | grep b57ae9798515 | grep snapshot
sha256:b57ae9798515d642a7b9c9ec5beea9dac5870d1cf770b76e70b76d134afb06eb 822 B           2 minutes       containerd.io/gc.ref.snapshot.overlayfs=sha256:964c0a9b08d55c0b51b044c0affe021fdd9fd366deffd1df3abe82baaf95d6ac

# check the snapshot (id is in the gc label)
$ sudo ctr -a /var/run/containerd.sock snapshot info sha256:964c0a9b08d55c0b51b044c0affe021fdd9fd366deffd1df3abe82baaf95d6ac
{
    "Kind": "Committed",
    "Name": "sha256:964c0a9b08d55c0b51b044c0affe021fdd9fd366deffd1df3abe82baaf95d6ac",
    "Parent": "sha256:23bc2b70b2014dec0ac22f27bb93e9babd08cdd6f1115d0c955b9ff22b382f5a",
    "Created": "2018-12-29T05:24:14.337401258Z",
    "Updated": "2018-12-29T05:24:14.337401258Z"
}

# time to remove the image
$ sudo pouch rmi hello:1

# check the snapshot
$ sudo ctr -a /var/run/containerd.sock snapshot info sha256:964c0a9b08d55c0b51b044c0affe021fdd9fd366deffd1df3abe82baaf95d6ac
ctr: snapshot sha256:964c0a9b08d55c0b51b044c0affe021fdd9fd366deffd1df3abe82baaf95d6ac does not exist: not found

Ⅴ. Special notes for reviews

@codecov
Copy link

codecov bot commented Dec 29, 2018

Codecov Report

Merging #2635 into master will increase coverage by 0.03%.
The diff coverage is 65.11%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2635      +/-   ##
=========================================
+ Coverage   69.57%   69.6%   +0.03%     
=========================================
  Files         280     280              
  Lines       18844   18844              
=========================================
+ Hits        13111   13117       +6     
+ Misses       4268    4261       -7     
- Partials     1465    1466       +1
Flag Coverage Δ
#criv1alpha1test 31.45% <0%> (+0.04%) ⬆️
#criv1alpha2test 35.69% <0%> (-0.02%) ⬇️
#integrationtest 41.69% <65.11%> (+0.01%) ⬆️
#nodee2etest 32.74% <0%> (ø) ⬆️
#unittest 26.91% <0%> (ø) ⬆️
Impacted Files Coverage Δ
ctrd/image_commit.go 68.87% <65.11%> (ø) ⬆️
daemon/logger/jsonfile/utils.go 71.54% <0%> (-1.63%) ⬇️
cri/v1alpha2/cri_wrapper.go 65.59% <0%> (-1.21%) ⬇️
cri/v1alpha2/cri.go 67.82% <0%> (-0.25%) ⬇️
daemon/mgr/container.go 59.21% <0%> (+0.21%) ⬆️
ctrd/container.go 59.28% <0%> (+1.18%) ⬆️
ctrd/image.go 78.18% <0%> (+2.27%) ⬆️

@pouchrobot pouchrobot added kind/bug This is bug report for project size/L labels Dec 29, 2018
For `containerd.io/gc.root` label means the content/snapshot cannot be
removed during gc. If we use it for snapshot, the snapshot cannot be
removed during removing image.

By default, we should add
`containerd.io/gc.ref.snapshot.{snapshot-name}` gc label to the image
config so that gc can remove the snapshot if user tries to remove the
image.

close #2556

Signed-off-by: Wei Fu <fuweid89@gmail.com>
@fuweid fuweid requested a review from ZYecho December 29, 2018 06:07
@wangforthinker
Copy link
Contributor

LGTM

@Ace-Tang Ace-Tang merged commit d28d0b0 into AliyunContainerService:master Jan 2, 2019
@fuweid fuweid deleted the bugfix_use_the_wrong_gc_label branch January 8, 2019 03:30
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/L
Projects
None yet
4 participants