Alternate lookups - #1084
Alternate lookups#1084
Conversation
| return []reference.DockerImageReference{r.locator.ref}, false, nil | ||
| } | ||
|
|
||
| r.lock.Lock() |
There was a problem hiding this comment.
This is concerning - why did you make this change? This prevents multiple FirstRequests being called, so I didn't expect it to be removed.
There was a problem hiding this comment.
yeah, I think that's coming from the fact that Sally added errorRepos below but analyzing that again, it doesn't make sense, since we have higher-level methods responsible for calling errorRepos, I'll also update the comments with that information
There was a problem hiding this comment.
right, calling errorRepos from initialRepos there was a deadlock-there was limited context with use of FirstRequest, it's simply returned/not used w/ oc & image-registry, so i left the lock in errorRepos since that's where oc required it- wasn't foreseeing multiple FirstRequests - not calling errorRepos & returning the method's ref instead has the same result and is more appropriate.
| // Repository returns a properly authenticated distribution.Repository for the given registry, repository | ||
| // name, and insecure toleration behavior. | ||
| Repository(ctx context.Context, registry *url.URL, repoName string, insecure bool) (distribution.Repository, error) | ||
| Repository(ctx context.Context, registry *url.URL, repoName string, insecure bool) (RepositoryWithLocation, error) |
There was a problem hiding this comment.
Can you link to where this is required (use case in oc)?
There was a problem hiding this comment.
I went through all the possible places and I'm not seeing any particular cases, yet. I'll revert that bit back as well.
There was a problem hiding this comment.
this was requested by image-registry (@ricardomaraschini ) they use this
There was a problem hiding this comment.
Image registry uses the RepositoryRetriever interface.
There was a problem hiding this comment.
Right, but from what I'm looking around this means keeping the interface untouched which is what I did in my last commit. We can always re-submit that change, if necessary in a followup.
Implements the core of the discussion around supporting multiple sources of input. Each wrapped method in the mirroredRepository structure that needs to check alternates (methods that invoke digest lookups) will use that structure. Methods that cannot be retried will use the first alternative that has a working client (such as ServeBlob). Methods that must always use the source (mutable calls or those that don't deal with a digest) will only look at the original source, although the logic is structured so we can use data about these requests in the future. The strategy is clarified to call either FirstRequest and OnFailure, or just FirstRequest. Each repository will make a single call to the strategy, and it's up to the strategy to include the original source when invoking FirstRequest.
0390103 to
3f8f617
Compare
|
Can you squash the last commit into the previous one? |
3f8f617 to
2d81863
Compare
Done |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smarterclayton, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
5 participants
Simplified version of #939 as discussed with @smarterclayton
/assign @smarterclayton
The initial take in oc lives in openshift/oc#829