WIP: Registry image pruning reworked #5073

miminar · 2015-10-12T12:33:39Z

Rebases docker/distribution to current upstream master with storage API expose patch applied.

Replaces /admin/* routes with just /admin/prune that triggers garbage collection.

Garbage collection loads repositories (ImageStreams and ImageStreamDeletions), all their manifest revisions (Images) and dependend objects (layers and signatures). Manifest revision is marked for deletion if it has DeletionTimestamp set. Repository is marked for deletion if it is listed among ImageStreamDeletions. Any other dependent, loaded object is preserved if it's referenced by at least one object not marked for deletion.

Resolves #3333
Resolves #4339

TODOs:

~~Find a way, how to keep ImageStreamDeletion type without the need to export it in cli (which now prevents OSO's compilation).~~ ImageStreamDeletions can now be described.
~~Rework oadm prune command to just mark obsolete Images for deletion.~~
~~Rework deletion of image streams to mark for deletion any referenced image, delete image stream itself and add its name to imageStreamDeletions.~~ done
~~Define finalizer for Images and trigger its Finalize() from inside of docker registry.~~ done
Remove admin/prune route - now used for testing.
Test it.
Let the garbage collection runs be configured.

miminar · 2015-10-12T12:54:20Z

@ncdc, @Kargakis: PTAL.

ncdc · 2015-10-12T13:03:46Z

Rework deletion of image streams to mark for deletion any referenced image

This brings up an interesting question. Let's say I push project/repo1:latest, then do oc tag project/repo1:latest project/repo2:sometag, and then delete project/repo1. If you delete the image because you've deleted repo1, you can no longer pull project/repo2:sometag because it points to the image you've just marked for deletion.

ncdc · 2015-10-12T13:03:51Z

cc @smarterclayton

ncdc · 2015-10-12T13:06:25Z

I would like to get rid of the custom /admin route(s) if possible.

Also, when reapplying carried patches, please cherry-pick them individually instead of bundling them into a new commit. It is crucial that patches have commit messages with UPSTREAM (docker/distribution): <pr #>: <description>. Or if there isn't an upstream PR and we want to just carry the patch, replace <pr #> with carry.

0xmichalis · 2015-10-12T13:33:20Z

and then delete project/repo1

@ncdc should we not allow deleting image streams that have symlinks?

miminar · 2015-10-12T13:51:19Z

If you delete the image because you've deleted repo1, you can no longer pull project/repo2:sometag because it points to the image you've just marked for deletion.

I'm not sure, how oc tag works. Ideally, it would create a new manifest with almost identical content except for Name and Tag and re-push. Re-push is needed to create layer links and signatures. Layers are already there. I'd rather have tags in consistence with registry's storage instead of preventing deletion if there were any tags created.

Update: could be solved in the registry during pruning. Manifest revision missing any layer or signature link could be rewritten.

Also, when reapplying carried patches, please cherry-pick them individually instead of bundling them into a new commit.

~~I'll try to look them up.~~ Done.

liggitt · 2015-10-15T18:13:48Z

can you add UPSTREAM: PR#|<carry>|<drop>: ... to the titles of all the commits that touch the distribution Godeps?

liggitt · 2015-10-15T18:27:22Z

and any commits that are just straight godep adds/bumps should have titles of bump(<dep name>): <version hash>

…6a54d28ff Signed-off-by: Michal Minar <[email protected]>

…bf78f8b4 New dependency for github.com/docker/distribution Signed-off-by: Michal Minar <[email protected]>

Add support for custom routes and custom auth records per route.

…r pruning Signed-off-by: Michal Minar <[email protected]>

Remove this commit once the admin routes are gone. Signed-off-by: Michal Minar <[email protected]>

- Repository components 1 character long allowed - Use new docker/distribution/registry/api/errcode package Signed-off-by: Michal Minar <[email protected]>

Stored in `/openshift.io/imagestreamdeletions` it contains references to deleted image streams that needs to be purged from internal docker registry. It's not namespaced. Signed-off-by: Michal Minar <[email protected]>

Allow to update status of Images Signed-off-by: Michal Minar <[email protected]>

Allow to list images of imagestreams from inside of registry. Signed-off-by: Michal Minar <[email protected]> Signed-off-by: Michal Minar <[email protected]>

Renamed ImportController to ImageStreamController since now apart from importing of images/tags it handles image stream's finalization as well. Signed-off-by: Michal Minar <[email protected]>

Conflicts: pkg/cmd/admin/prune/images.go pkg/image/prune/imagepruner.go

- Don't add layers and blobs from graph. - Marking image deleted instead of deleting it. Signed-off-by: Michal Minar <[email protected]>

`POST admin/prune` route triggers a garbage collection in docker registry where orphaned repositories, manifest revisions, layers, signatures and blobs are deleted. Signed-off-by: Michal Minar <[email protected]>

smarterclayton · 2015-10-22T17:13:37Z

Is this targeted to 3.1? It's really, really late to be making changes here.

ncdc · 2015-10-22T17:17:09Z

These are huge changes. I think we need to consider this for 3.2 instead.

liggitt · 2015-10-22T17:31:49Z

agree 😨

miminar · 2015-10-23T07:47:35Z

Yeah, that's what I was afraid of happening once we decided to go with finalization and watching its scope extending over the code base. But I agree with you. I'd rather not rush with these changes and give them a second thought.

I'll break up the changes into smaller, tested pieces to make it easier to digest and review.

openshift-bot · 2015-11-18T15:45:19Z

Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

soltysh · 2015-11-24T15:04:00Z

@miminar are you going to pick it up now that we're post 1.1? I'd love to see this in.

miminar · 2015-11-25T07:52:37Z

@soltysh I'd love to. However, it's still blocked on #4339 which in turn is block on upstream. They respond neither to comments nor on irc. It worries me. I don't know what to do about it.

danmcp · 2016-01-19T18:36:03Z

@miminar What are the next steps here?

miminar · 2016-01-20T07:49:26Z

@danmcp rebase it, split it into smaller pieces for review and write proper tests. This can be closed anyway.

miminar force-pushed the registry-refactored branch 2 times, most recently from fd785cd to f8610c3 Compare October 12, 2015 17:59

Michal Minar and others added 3 commits October 16, 2015 10:19

bump(github.com/docker/distribution): 44c7fb9e6e7e58e88d38d93b5f1f6da…

a203bfb

…6a54d28ff Signed-off-by: Michal Minar <[email protected]>

bump(github.com/stevvooe/resumable): 51ad44105773cafcbe91927f70ac68e1…

85a77b5

…bf78f8b4 New dependency for github.com/docker/distribution Signed-off-by: Michal Minar <[email protected]>

UPSTREAM: docker/distribution: <carry>: custom routes/auth

df2aa1d

Add support for custom routes and custom auth records per route.

pweil- mentioned this pull request Oct 16, 2015

OpenStack Swift support for docker-registry #4672

Closed

Michal Minar added 3 commits October 17, 2015 21:04

UPSTREAM: docker/distribution: 1050: Exported API functions needed fo…

3a7aa4a

…r pruning Signed-off-by: Michal Minar <[email protected]>

UPSTREAM: docker/distribution: <carry>: export app.Namespace

291d646

Remove this commit once the admin routes are gone. Signed-off-by: Michal Minar <[email protected]>

Refactored OSO's code depending on docker/distribution

86c9ca9

- Repository components 1 character long allowed - Use new docker/distribution/registry/api/errcode package Signed-off-by: Michal Minar <[email protected]>

miminar force-pushed the registry-refactored branch 3 times, most recently from eb32338 to 53a34bf Compare October 19, 2015 14:04

Michal Minar and others added 7 commits October 20, 2015 19:48

Added new type ImageStreamDeletions

f78dbaf

Stored in `/openshift.io/imagestreamdeletions` it contains references to deleted image streams that needs to be purged from internal docker registry. It's not namespaced. Signed-off-by: Michal Minar <[email protected]>

Extended Image for Status and Finalizers

95a506a

Allow to update status of Images Signed-off-by: Michal Minar <[email protected]>

Added ImageStreamImageList type

bf3939a

Allow to list images of imagestreams from inside of registry. Signed-off-by: Michal Minar <[email protected]> Signed-off-by: Michal Minar <[email protected]>

Allow to finalize ImageStreams

505274c

Renamed ImportController to ImageStreamController since now apart from importing of images/tags it handles image stream's finalization as well. Signed-off-by: Michal Minar <[email protected]>

prune images: Don't prune manifests, layers, blobs

72be61e

Conflicts: pkg/cmd/admin/prune/images.go pkg/image/prune/imagepruner.go

Simplified image pruning

67d5403

- Don't add layers and blobs from graph. - Marking image deleted instead of deleting it. Signed-off-by: Michal Minar <[email protected]>

Added prune route to docker registry

f598ee8

`POST admin/prune` route triggers a garbage collection in docker registry where orphaned repositories, manifest revisions, layers, signatures and blobs are deleted. Signed-off-by: Michal Minar <[email protected]>

miminar force-pushed the registry-refactored branch from 53a34bf to f598ee8 Compare October 20, 2015 17:49

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 22, 2015

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 13, 2015

soltysh mentioned this pull request Nov 16, 2015

Reenabled registry dc's liveness probe #5892

Closed

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2015

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2015

miminar closed this Jan 20, 2016

miminar deleted the registry-refactored branch November 10, 2016 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Registry image pruning reworked #5073

WIP: Registry image pruning reworked #5073

miminar commented Oct 12, 2015

miminar commented Oct 12, 2015

ncdc commented Oct 12, 2015

ncdc commented Oct 12, 2015

ncdc commented Oct 12, 2015

0xmichalis commented Oct 12, 2015

miminar commented Oct 12, 2015

liggitt commented Oct 15, 2015

liggitt commented Oct 15, 2015

smarterclayton commented Oct 22, 2015

ncdc commented Oct 22, 2015

liggitt commented Oct 22, 2015

miminar commented Oct 23, 2015

openshift-bot commented Nov 18, 2015

soltysh commented Nov 24, 2015

miminar commented Nov 25, 2015

danmcp commented Jan 19, 2016

miminar commented Jan 20, 2016

WIP: Registry image pruning reworked #5073

WIP: Registry image pruning reworked #5073

Conversation

miminar commented Oct 12, 2015

miminar commented Oct 12, 2015

ncdc commented Oct 12, 2015

ncdc commented Oct 12, 2015

ncdc commented Oct 12, 2015

0xmichalis commented Oct 12, 2015

miminar commented Oct 12, 2015

liggitt commented Oct 15, 2015

liggitt commented Oct 15, 2015

smarterclayton commented Oct 22, 2015

ncdc commented Oct 22, 2015

liggitt commented Oct 22, 2015

miminar commented Oct 23, 2015

openshift-bot commented Nov 18, 2015

soltysh commented Nov 24, 2015

miminar commented Nov 25, 2015

danmcp commented Jan 19, 2016

miminar commented Jan 20, 2016