-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
doc/spec, registry: immutable manifest reference support #211
doc/spec, registry: immutable manifest reference support #211
Conversation
cc @ncdc |
@stevvooe awesome, thanks! I'll take a look tonight/tomorrow. |
looking over this, it LGTM Looks like there may be a few failures that need to be cleaned up though
|
@vbatts The build seems to be okay. Did you see this on your local version? Perhaps, you need to clear pkg in your GOPATH for the updated interfaces? |
I've been able to compile and run cmd/registry/main.go locally |
e0c410a
to
bba344c
Compare
func getTag(ctx context.Context) (tag string) { | ||
return ctxu.GetStringValue(ctx, "vars.tag") | ||
func getReference(ctx context.Context) (reference string) { | ||
return ctxu.GetStringValue(ctx, "vars.reference") |
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 still don't see this value getting set, if it is supposed to be set by mux it is not.
INFO[0386] response completed app.id=66e8ee98-cc08-46d8-a99f-7c1aeabf9e8d http.request.host=localhost:5001 http.request.id=1c182fa2-fa29-4fe1-8304-2a2cb41816a7 http.request.method=PUT http.request.remoteaddr=127.0.0.1:48976 http.request.uri=/v2/dmcgowan/hello/manifests/anything http.request.useragent=docker/1.5.0-dev go/go1.4.1 git-commit/ed64cc9 kernel/3.18.5-101.fc20.x86_64 os/linux arch/amd64 http.response.duration=8.457093ms http.response.written=0 vars.name=dmcgowan/hello version=v2.0.0-alpha.1+unknown
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.
Need to add an entry for vars.reference to https://github.com/docker/distribution/blob/master/registry/handlers/app.go#L278-L283
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.
Fixed. 👹
bba344c
to
b4dd565
Compare
t.Fatalf("unexpected error fetching manifest: %v", err) | ||
} | ||
|
||
if fetchedByManifest.Tag != fetchedByManifest.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.
I'm guessing you want to compare fetchedByManifest.Tag
with sm.Tag
?
Also, similar question on line 174 if fetched.Tag != fetched.Tag {
@stevvooe can you describe the issue with delete by tag? |
@stevvooe is there supposed to be a supported route for delete by digest? |
@ncdc At this time, there is no supported route for delete by tag. After looking into implementation, the behavior of deletes invite unexpected behavior. I attempted a few different approaches. The first deleted all manifests under a given tag. This poses conflicts with a detached tag model and could lead to inconsistent behavior. The deleted the latest version using a link-based stack. This didn't go very well either. In the interim, we'll support the ability to delete blobs, from an admin user, from the repository but it is the responsibility of the caller to maintain consistency of the manifests. This will support most use cases that require deleting older data. In response to this, I've filed #210. Several issues are discussed there. My conclusion was that even with reference counting, one cannot be sure that a blob is unreferenced on an eventually consistent system. Erring on the side of never deleting unintentionally has resulted in deferring support for rich deletes. That said, we are working on an alternate solution that will help us to get deletion support. Either way, my intention was to not hold up this PR on deletions. |
Manifests are now fetched by a field called "reference", which may be a tag or a digest. When using digests to reference a manifest, the data is immutable. The routes and specification have been updated to allow this. There are a few caveats to this approach: 1. It may be problematic to rely on data format to differentiate between a tag and a digest. Currently, they are disjoint but there may modifications on either side that break this guarantee. 2. The caching characteristics of returned content are very different for digest versus tag-based references. Digest urls can be cached forever while tag urls cannot. Both of these are minimal caveats that we can live with in the future. Signed-off-by: Stephen J Day <stephen.day@docker.com>
This changeset implements immutable manifest references via the HTTP API. Most of the changes follow from modifications to ManifestService. Once updates were made across the repo to implement these changes, the http handlers were change accordingly. The new methods on ManifestService will be broken out into a tagging service in a later PR. Unfortunately, due to complexities around managing the manifest tag index in an eventually consistent manner, direct deletes of manifests have been disabled. Signed-off-by: Stephen J Day <stephen.day@docker.com>
b4dd565
to
40273b1
Compare
@stevvooe what about delete by digest? I couldn't get it to work, as it appears that images.go DeleteImageManifest always returns unsupported, without checking to see if you're trying to delete by digest. |
@ncdc The above explanation applies to this case, as well: if a manifest is deleted by digest, what happens to the tag index? Does the value of the tag pointer revert to a previous value or does it force a delete of all tags? These are simple problems on a transactionally consistent backend. However, with eventually consistent storage, the implementation is non-trivial. Once we have a stable release, we need to revisit the backend implementations. The real solution lies in first decoupling tags from the manifests, then building a consistent strategy for managing the tags. |
Thanks for the explanation. Makes sense. |
LGTM! |
LGTM |
doc/spec, registry: immutable manifest reference support
…references doc/spec, registry: immutable manifest reference support
…references doc/spec, registry: immutable manifest reference support
Manifests are now fetched by a field called "reference", which may be a tag or a digest. When using digests to reference a manifest, the data is immutable. The routes and specification have been updated to allow this. There are a few caveats to this approach:
Both of these are minimal caveats that we can live with in the future.
Most of the changes follow from modifications to ManifestService. Once updates were made across the repo to implement these changes, the http handlers were change accordingly. The new methods on ManifestService will be broken out into tagging service in a later PR.
Unfortunately, due to complexities around managing the manifest tag index in an eventually consistent manner, direct deletes of manifests have been disabled.
Closes #46. Implements the registry portion of moby/moby#10740.