docker registry: add support for mounts#3053
docker registry: add support for mounts#3053simonferquel wants to merge 7 commits intocontainerd:masterfrom
Conversation
9e097b6 to
90dba5c
Compare
Codecov Report
@@ Coverage Diff @@
## master #3053 +/- ##
==========================================
+ Coverage 43.87% 43.94% +0.07%
==========================================
Files 102 102
Lines 10903 10990 +87
==========================================
+ Hits 4784 4830 +46
- Misses 5384 5421 +37
- Partials 735 739 +4
Continue to review full report at Codecov.
|
Is this still true? Where is the potential |
ijc
left a comment
There was a problem hiding this comment.
I had a few comments, but overall this is looking good to me (but I'm not a maintainer).
remotes/docker/scope.go
Outdated
| } | ||
| return context.WithValue(ctx, tokenScopesKey{}, []string{s}), nil | ||
| scopes := []string{s} | ||
| if combineWithExistingScopes { |
There was a problem hiding this comment.
Would there be a downside to always doing this? It would mean simpler code here and in callers (no need for the new arg) and is what I think I would always expect of a function which takes a context and produces another one -- i.e. that they are cumulative.
| ctx, err := contextWithRepositoryScope(ctx, mountCandidate, false, true) | ||
| if err != nil { | ||
| // try next candidate | ||
| continue |
There was a problem hiding this comment.
This would imply that findMatchingOrigins returned something malformatted, I think? It could be argued that this should be fatal and the error returned, but I think at least it should log an error.
| } | ||
| i := strings.Index(mountCandidate.Locator, "/") | ||
| originRepoPath := mountCandidate.Locator[i+1:] | ||
| req, err = http.NewRequest(http.MethodPost, fmt.Sprintf("%s/?mount=%s&from=%s", p.url("blobs", "uploads"), desc.Digest, originRepoPath), nil) |
There was a problem hiding this comment.
Should mount and from not be added with somethign like:
q := req.URL.Query()
q.Add("digest", desc.Digest.String())
req.URL.RawQuery = q.Encode()(which I stole from further down in this function). It's still more roundabout than I would like but is better than opencoding the query construction I think.
| }, | ||
| }) | ||
| // mount succeeded, returning a nil writer, without error | ||
| return nil, errors.Wrapf(errdefs.ErrAlreadyExists, "content %v mounted from %s", desc.Digest, mountCandidate) |
There was a problem hiding this comment.
I know what you mean, but technically this is returning with an error. Maybe say "with canary error value"?
I also think this sort of subtlety is worthy of a godoc comment on the function itself.
| // OriginProvider returns a slice of refspecs where the Pusher may find candidates for mounting | ||
| // instead of pushing from local store. | ||
| // When the Pusher succeeds by mounting, it returns an AlreadyExists error | ||
| OriginProvider func(ocispec.Descriptor) []reference.Spec |
There was a problem hiding this comment.
It's an ErrAlreadyExists (Err prefix).
Like I commented above, I think this is worth repeating on the godoc for Push too.
I guess from the struck out stuff in the top comment that at some point |
a892346 to
6530636
Compare
Change the token scope merging to be grammar-aware. This features: - smarter deduplication (repository:foo/bar:pull,push+repository:foo/bar:push,pull -> repository:foo/bar:pull,push) - sort-based normalization (actions on a resources are always sorted alphabethically, returned string slice is always sorted alphabethically) - merging of scopes concerning the same resource (repository:foo/bar:pull+repository:foo/bar:push => repository:foo/bar:pull,push) Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
6530636 to
42c3614
Compare
Also, extracted token caching logic for better testability Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
This add a notion of OriginResolver, to the docker resolver, so that we can give the Pusher hints that it can actually mount a blob instead of having to push it from a local resource. If a Push operation succesfuly mount a blob, it returns a nil writer. Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
This makes existing caller act as if the blob already existed, instead of having to deal with a nil writer and no error Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
42c3614 to
b7418cd
Compare
|
Support for mount-on-push was merged in #3218. Thanks for your contribution, and feel free to let us know if any of the functionality and/or tests from your PR are still applicable. |
Based on #3052
Following @dmcgowan advice (#3045 (comment)), I wrote support for mounting blobs in a way that does not require a new interface, by just adding an OriginProvider field in the Resolver Options.
On the PRO side, it requires less changes to calling code to add mount capabilities.
On the CON side,<-mitigated that by returning a ErrAlreadyExist error. Would love to have feedback on this.Pusher.Pushcan now return a nil writer (in cases where the Push has been replaced by a mount) with no error.I slightly prefer my previous design as the new one can provoke nil-dereferences panics in existing callers code.