-
Notifications
You must be signed in to change notification settings - Fork 2.7k
add possibility to clean untagged manifests #2302
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
Conversation
|
Please sign your commits following these rules: $ git clone -b "oma" [email protected]:zetaab/distribution.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -fAmending updates the existing PR. You DO NOT need to open a new one. |
|
original discussion in #2212 |
Codecov Report
@@ Coverage Diff @@
## master #2302 +/- ##
==========================================
- Coverage 61.42% 51.61% -9.82%
==========================================
Files 128 128
Lines 11685 11727 +42
==========================================
- Hits 7178 6053 -1125
- Misses 3615 4916 +1301
+ Partials 892 758 -134
Continue to review full report at Codecov.
|
registry/root.go
Outdated
| RootCmd.AddCommand(ServeCmd) | ||
| RootCmd.AddCommand(GCCmd) | ||
| GCCmd.Flags().BoolVarP(&dryRun, "dry-run", "d", false, "do everything except remove the blobs") | ||
| GCCmd.Flags().BoolVarP(&manifestswithouttags, "delete-manifests", "m", false, "delete manifests which does not have tag attached") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete-untagged. The manifest still exist. What we are trying to delete is the manifests that are not currently referenced via tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, that these may not always be manifests.
registry/root.go
Outdated
| } | ||
|
|
||
| err = storage.MarkAndSweep(ctx, driver, registry, dryRun) | ||
| err = storage.MarkAndSweep(ctx, driver, registry, dryRun, manifestswithouttags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a new boolean subsequent to another will get confusing. Let's define an options struct here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to find way to do that, but could not find anything that works. Can you help me little bit where I should define that struct (it needs to be available in other files where markandsweep is referenced)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// registry/storage/garbagecollect.go
type GCOpts struct {
DryRun bool
ManifestsWithoutTags bool
}
...
err = storage.MarkAndSweep(ctx, driver, registry, storage.GCOpts{
DryRun: dryRun,
ManifestsWithoutTags: manifestsWithoutTags,
})
registry/storage/garbagecollect.go
Outdated
|
|
||
| // mark | ||
| markSet := make(map[digest.Digest]struct{}) | ||
| manifestSet := make(map[digest.Digest]string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
manifestDeleteSet
Make sure to have a comment here that we are storing in the target repository.
registry/storage/garbagecollect.go
Outdated
| } | ||
| if len(tags) == 0 { | ||
| emit("manifest has no tags %s@%s", repoName, dgst) | ||
| manifestSet[dgst] = repoName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because you are deleting the manifests after a shallow reference test (ie tags), I think you can get away with immediate deletion. Double check in paths.go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not fully understand this. How I should check it with paths.go? And you mean that I could delete manifest here already? (means that I need initialize vacuum in start of function)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zetaab The file paths.go documents the storage layout, including the relationships that must be maintained when modifying the storage layout. Any assumptions should be checked against the layout there.
I think you might be able to get away with issuing the delete right here, since the reference set is contained in the tags present here and nowhere else. Effectively, this list of tags is acting like a local reference count.
registry/storage/vacuum.go
Outdated
| } | ||
|
|
||
| // RemoveManifest removes a manifest from the filesystem | ||
| func (v Vacuum) RemoveManifest(dgst string, name string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These arguments are logically backwards. The name should come first.
registry/storage/vacuum.go
Outdated
| return err | ||
| } | ||
|
|
||
| context.GetLogger(v.ctx).Infof("Deleting manifest: %s", manifestPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't capitalize log messages.
registry/storage/vacuum.go
Outdated
| } | ||
|
|
||
| context.GetLogger(v.ctx).Infof("Deleting manifest: %s", manifestPath) | ||
| err = v.driver.Delete(v.ctx, manifestPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return v.driver.Delete(v.ctx, manifestPath)
registry/storage/vacuum.go
Outdated
| } | ||
|
|
||
| // RemoveManifest removes a manifest from the filesystem | ||
| func (v Vacuum) RemoveManifest(dgst string, name string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, type of dgst should be digest.Digest. No need to re-parse it.
|
@zetaab Thanks for the submission! I have done some review in line. The logic looks correct but there are few nits and I think we can avoid the extra map, since we don't have to actually calculate reachability. |
hinshun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Design looks good! Thanks for the contribution. I've left some comments.
registry/root.go
Outdated
| } | ||
|
|
||
| var dryRun bool | ||
| var manifestswithouttags bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should camelCase local variables
registry/root.go
Outdated
| } | ||
|
|
||
| err = storage.MarkAndSweep(ctx, driver, registry, dryRun) | ||
| err = storage.MarkAndSweep(ctx, driver, registry, dryRun, manifestswithouttags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// registry/storage/garbagecollect.go
type GCOpts struct {
DryRun bool
ManifestsWithoutTags bool
}
...
err = storage.MarkAndSweep(ctx, driver, registry, storage.GCOpts{
DryRun: dryRun,
ManifestsWithoutTags: manifestsWithoutTags,
})
registry/storage/garbagecollect.go
Outdated
|
|
||
| err = manifestEnumerator.Enumerate(ctx, func(dgst digest.Digest) error { | ||
| if manifestswithouttags { | ||
| tags, errr := repository.Tags(ctx).Lookup(ctx, distribution.Descriptor{Digest: dgst}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/errr/err/
|
is there way to rebuild that circleci check? does not look like it is because of my code? |
|
@zetaab We need to restart the build. Let's make sure to address #2302 (comment) before merging. |
|
@stevvooe not sure is it now how it should be. at least I am checking is pathfor returning error or not. |
registry/storage/garbagecollect.go
Outdated
| emit("manifest has no tags %s@%s", repoName, dgst) | ||
| emit("manifest eligible for deletion: %s", dgst) | ||
| // counter | ||
| manifestDeleteSet[dgst] = repoName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This set is no longer needed.
registry/storage/garbagecollect.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| emit("manifest has no tags %s@%s", repoName, dgst) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only a single message is required here.
|
It looks like the latest official image for the registry is 2.6.2, released ~1 year(!) ago. |
|
Also still waiting for a new release. |
|
+1 here |
|
@dahuether @boianmihailov New pre-release https://github.com/docker/distribution/releases/tag/v2.7.0-rc.0 is out. Please test and let us know if it works. |
|
@manishtomar Built an image from that tag, to make it work the entry point had to be changed to ENTRYPOINT ["./bin/registry"] and in my case garbage collector didn't delete anything. Thanks! |
|
@boianmihailov you need to use Like in above comments: |
|
@zetaab Thanks for the input! adding |
|
I can confirm that this is working! |
|
Where I can find out the docker image of registry 2.7.0? There is no 2.7.0 in docker hub https://hub.docker.com/r/vmware/registry/tags. Thanks! |
|
@wwyhy It should appear in https://hub.docker.com/r/_/registry/ in a few days time. |
|
Ok, thank you! hope the it can GC the unused manifest. |
|
@manishtomar is there an eta of the release to docker hub? |
|
@johan-smits The whole docker hub repo got nuked it seems. 😭 Getting a 404 @ https://hub.docker.com/r/_/registry/ currently. |
|
@gertvdijk you should use https://hub.docker.com/r/distribution/registry/ instead. |
Nope. See the warning there:
|
|
It looks like the paths changed; https://hub.docker.com/r/_/registry/ gives me a 404, but https://hub.docker.com/_/registry works. (Still no 2.7.0, though...) |
|
Interestingly, the source repo already has 2.7.0 changes: https://github.com/docker/distribution-library-image/ Maybe some build process has to be triggered? @dmcgowan @stevvooe do you know when 2.7.0 will be available on docker hub? |
|
@ffdybuster #2784 (comment) |
|
2.7.0 is now available on Docker hub (with this PR being merged). https://hub.docker.com/_/registry?tab=tags -> lists |
|
Wonderful work! |
|
What am I overseeing here:
Neither -m option nor --dry-run option here, although clearly version 2.7.1. Tried the same with 2.7.0 image. Same issue. |
|
@scrwr inside the docker you can run: |
|
@johan-smits
|
fixes issue #2301
now the situation is that there is two manifests, and only one has active tag.
New flag added 'm'
old behaviour:
disk in this point:
new feature:
disk after this: