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: remove blobs and snapshot when remove image #2075

Merged
merged 1 commit into from
Aug 11, 2018
Merged

bugfix: remove blobs and snapshot when remove image #2075

merged 1 commit into from
Aug 11, 2018

Conversation

fuweid
Copy link
Contributor

@fuweid fuweid commented Aug 11, 2018

Signed-off-by: Wei Fu fhfuwei@163.com

Ⅰ. Describe what this PR did

Don't add lease to the image since the containerdclient.Pull will add
the lease.

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Describe how you did it

the lease should be released after pull. The containerd client has this functionality so that we should add the one.

Ⅳ. Describe how to verify it

  1. pull the image
$ sudo pouch pull busybox:1.28
registry.hub.docker.com/library/busybox:1.28:                                     resolved       |++++++++++++++++++++++++++++++++++++++|
index-sha256:141c253bc4c3fd0a201d32dc1f493bcf3fff003b6df416dea4f41046e0f37d47:    done           |++++++++++++++++++++++++++++++++++++++|
manifest-sha256:74f634b1bc1bd74535d5209589734efbd44a25f4e2dc96d78784576a3eb5b335: done           |++++++++++++++++++++++++++++++++++++++|
layer-sha256:07a152489297fc2bca20be96fab3527ceac5668328a30fd543a160cd689ee548:    done           |++++++++++++++++++++++++++++++++++++++|
config-sha256:8c811b4aec35f259572d0f79207bc0678df4c736eeec50bc9fec37ed936a472a:   done           |++++++++++++++++++++++++++++++++++++++|
elapsed: 9.6 s
  1. check the blobs
root@ubuntu-xenial:/var/lib/pouch/containerd/root/io.containerd.content.v1.content/blobs/sha256# ls -al
total 728
drwxr-xr-x 2 root root   4096 Aug 11 11:12 .
drwxr-xr-x 3 root root   4096 Aug 11 11:06 ..
-r--r--r-- 1 root root 723146 Aug 11 11:12 07a152489297fc2bca20be96fab3527ceac5668328a30fd543a160cd689ee548
-r--r--r-- 1 root root   2699 Aug 11 11:12 141c253bc4c3fd0a201d32dc1f493bcf3fff003b6df416dea4f41046e0f37d47
-r--r--r-- 1 root root    527 Aug 11 11:12 74f634b1bc1bd74535d5209589734efbd44a25f4e2dc96d78784576a3eb5b335
-r--r--r-- 1 root root   1497 Aug 11 11:12 8c811b4aec35f259572d0f79207bc0678df4c736eeec50bc9fec37ed936a472a
  1. remove image
$ sudo pouch rmi busybox:1.28
busybox:1.28

root@ubuntu-xenial:/var/lib/pouch/containerd/root/io.containerd.content.v1.content/blobs/sha256# ls -al
total 8
drwxr-xr-x 2 root root 4096 Aug 11 11:12 .
drwxr-xr-x 3 root root 4096 Aug 11 11:06 ..

Ⅴ. Special notes for reviews

Don't add lease to the image since the containerdclient.Pull will add
the lease.

Signed-off-by: Wei Fu <fhfuwei@163.com>
@pouchrobot pouchrobot added kind/bug This is bug report for project size/XS labels Aug 11, 2018
@fuweid fuweid requested a review from HusterWan August 11, 2018 03:14
@@ -196,8 +195,6 @@ func (c *Client) PullImage(ctx context.Context, ref string, authConfig *types.Au
}

func (c *Client) pullImage(ctx context.Context, wrapperCli *WrapperClient, ref string, options []containerd.RemoteOpt) (containerd.Image, error) {
ctx = leases.WithLease(ctx, wrapperCli.lease.ID())

img, err := wrapperCli.client.Pull(ctx, ref, options...)
Copy link
Contributor

Choose a reason for hiding this comment

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

should double check!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

checked it. The GC doesn't clean your image if you don't remove it.

@codecov-io
Copy link

Codecov Report

Merging #2075 into master will increase coverage by 12.55%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #2075       +/-   ##
===========================================
+ Coverage    51.3%   63.85%   +12.55%     
===========================================
  Files         209      209               
  Lines       16226    16225        -1     
===========================================
+ Hits         8324    10361     +2037     
+ Misses       6798     4570     -2228     
- Partials     1104     1294      +190
Flag Coverage Δ
#criv1alpha1test 32.56% <ø> (+0.06%) ⬆️
#criv1alpha2test 33.2% <ø> (+0.02%) ⬆️
#integrationtest 38.4% <ø> (?)
#unittest 24% <ø> (ø) ⬆️
Impacted Files Coverage Δ
ctrd/image.go 77.19% <ø> (+15.62%) ⬆️
cri/stream/httpstream/spdy/upgrade.go 54.28% <0%> (-5.72%) ⬇️
cri/v1alpha2/cri.go 65.06% <0%> (+0.17%) ⬆️
cri/v1alpha1/cri.go 64.22% <0%> (+0.17%) ⬆️
daemon/logger/jsonfile/utils.go 71.54% <0%> (+0.81%) ⬆️
daemon/containerio/container_io.go 74.58% <0%> (+1.1%) ⬆️
daemon/daemon.go 58.1% <0%> (+1.67%) ⬆️
daemon/mgr/spec_mount.go 77.55% <0%> (+2.04%) ⬆️
pkg/meta/boltdb.go 68.67% <0%> (+2.4%) ⬆️
apis/opts/mountpoint.go 90% <0%> (+2.85%) ⬆️
... and 74 more

@HusterWan
Copy link
Contributor

lgtm

@HusterWan HusterWan merged commit e27a71a into AliyunContainerService:master Aug 11, 2018
@fuweid fuweid deleted the bugfix_remove_blobs_and_snapshots_when_delete_image branch August 22, 2018 10:51
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/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants