Skip to content

remotes: support cross-repo-push#3218

Merged
estesp merged 1 commit intocontainerd:masterfrom
fuweid:me-cross-push
Jun 13, 2019
Merged

remotes: support cross-repo-push#3218
estesp merged 1 commit intocontainerd:masterfrom
fuweid:me-cross-push

Conversation

@fuweid
Copy link
Copy Markdown
Member

@fuweid fuweid commented Apr 16, 2019

With distribution source label in content store, select the longest
common prefix components as condidate mount blob source and try to push
with mount blob.

Fix #2964

Signed-off-by: Wei Fu fuweid89@gmail.com

@fuweid fuweid force-pushed the me-cross-push branch 2 times, most recently from 1a89d37 to e35d261 Compare April 16, 2019 08:38
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably be a const

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 16, 2019

Codecov Report

Merging #3218 into master will decrease coverage by 0.09%.
The diff coverage is 47.96%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #3218     +/-   ##
=========================================
- Coverage   44.85%   44.75%   -0.1%     
=========================================
  Files         113      113             
  Lines       12361    12513    +152     
=========================================
+ Hits         5544     5600     +56     
- Misses       5964     6057     +93     
- Partials      853      856      +3
Flag Coverage Δ
#linux 48.65% <49.71%> (-0.13%) ⬇️
#windows 40.2% <47.96%> (-0.05%) ⬇️
Impacted Files Coverage Δ
remotes/docker/handler.go 22.47% <0%> (-10.87%) ⬇️
remotes/docker/pusher.go 0% <0%> (ø) ⬆️
remotes/docker/scope.go 65.85% <61.9%> (-5.12%) ⬇️
remotes/docker/authorizer.go 70.52% <79.48%> (+2.54%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 545e79a...dd7c0aa. Read the comment docs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, oops, I missed the mis-spell; note that I didn't do a real review, I just noticed this PR and spotted the var/const, and now the typo; I'm not super-familiar with this area of the code, so you might want to wait for "real" reviewers to do their review 😅

Suggested change
const defaultExpiredIn = 60
const defaultExpiresIn = 60

@estesp
Copy link
Copy Markdown
Member

estesp commented Apr 16, 2019

this is an alternative to #3053, right?

@fuweid
Copy link
Copy Markdown
Member Author

fuweid commented Apr 17, 2019

this is an alternative to #3053, right?

@estesp yeah. kind of alternative. But this pr supports to do auth per request, instead of host name. The change is to support mount from feature provided by registry. It can save ton of time during pushing.

@fuweid fuweid changed the title remotes: support mount blob push [WIP] remotes: support mount blob push Apr 17, 2019
@fuweid fuweid force-pushed the me-cross-push branch 4 times, most recently from 84b7778 to 6d8eb5c Compare April 17, 2019 14:54
@fuweid fuweid changed the title [WIP] remotes: support mount blob push remotes: support mount blob push Apr 17, 2019
@fuweid fuweid changed the title remotes: support mount blob push remotes: support cross-repo-push Apr 17, 2019
@fuweid fuweid changed the title remotes: support cross-repo-push [WIP] remotes: support cross-repo-push Apr 17, 2019
@fuweid
Copy link
Copy Markdown
Member Author

fuweid commented Apr 17, 2019

because of the wall, I failed to vendor the package. will update it soon....

@fuweid fuweid changed the title [WIP] remotes: support cross-repo-push remotes: support cross-repo-push Apr 18, 2019
@fuweid
Copy link
Copy Markdown
Member Author

fuweid commented Apr 18, 2019

ping @dmcgowan PTAL. Thanks

@fuweid fuweid force-pushed the me-cross-push branch 2 times, most recently from 462bd81 to 57c7022 Compare April 27, 2019 14:23
@fuweid
Copy link
Copy Markdown
Member Author

fuweid commented Apr 27, 2019

ping @dmcgowan I have updated it and use lock and waitgroup to handle auth fetch job: authHandler will support to lock by the resource scope and limit one fetch job per resource scope. PTAL. Thanks

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if l != idx? A check seems cheaper than assignment considering the common case is l == idx

Copy link
Copy Markdown
Member Author

@fuweid fuweid May 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check the objdump and the if uses less instruction 😂

if there has one duplicate item, there always goes with if and assignment. So, I think it is ok here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does a new scope get added in? For example if I first requested for read the token options only has a pull scope, but now I add a response which push scope, do the token options get updated?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, for one registry host, both the realm and service are the same. The common option will maintain the original resource scope, for instance, repository:foo/bar:pull. If the following request wants to use different scope, like repository:foo/bar:push, the request handler should put the scope into the context and the auth handler will retrieve it and combine with original scope. I think we don't need to update the common options here.

@dmcgowan dmcgowan self-assigned this May 22, 2019
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Jun 12, 2019

Build succeeded.

With distribution source label in content store, select the longest
common prefix components as condidate mount blob source and try to push
with mount blob.

Fix containerd#2964

Signed-off-by: Wei Fu <fuweid89@gmail.com>
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Jun 13, 2019

Build succeeded.

Copy link
Copy Markdown
Member

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@estesp estesp merged commit 40b17e9 into containerd:master Jun 13, 2019
@fuweid fuweid deleted the me-cross-push branch August 22, 2019 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cross Repository Push

5 participants