Skip to content
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

Support preheat when using registry as backend #147

Merged
merged 5 commits into from
May 30, 2019

Conversation

mmpei
Copy link
Contributor

@mmpei mmpei commented Apr 30, 2019

@CLAassistant
Copy link

CLAassistant commented Apr 30, 2019

CLA assistant check
All committers have signed the CLA.

@codygibb
Copy link
Contributor

codygibb commented May 2, 2019

Would you mind updating the PR description with a change summary, including:

  • What is changing?
  • Why is it changing?

@mmpei
Copy link
Contributor Author

mmpei commented May 5, 2019

#145
the origin should cache blobs when using registry as backend.
now kraken only support pushing to registry directly and this PR will make origin cache the blobs of pushed images.
also it will works well when integrating with other image repository like harbor.

changes:
the proxy will listen on an addtional port for registry notification events. and it will pull the manifest and blob metaInfos as soon as receving a manifest pushing event. and this will make origin cache the blobs locally and reduce the time of process of P2P pulling.
@codygibb

Copy link
Contributor

@codygibb codygibb left a comment

Choose a reason for hiding this comment

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

Overall the change looks pretty reasonable to me. I'm fine holding off on tests until the review stabilizes, but we should add some before this lands.

proxy/server/server.go Outdated Show resolved Hide resolved
proxy/server/server.go Outdated Show resolved Hide resolved
proxy/server/server.go Outdated Show resolved Hide resolved
proxy/server/server.go Outdated Show resolved Hide resolved
proxy/server/server.go Outdated Show resolved Hide resolved
proxy/server/server.go Outdated Show resolved Hide resolved
proxy/server/server.go Outdated Show resolved Hide resolved
proxy/server/registry_events.go Outdated Show resolved Hide resolved
@mmpei
Copy link
Contributor Author

mmpei commented May 10, 2019

@codygibb i have updated as your comment. please check again.
if it looks ok, i will do more test on this feature and raise a new PR with only one commit later.

Copy link
Contributor

@codygibb codygibb left a comment

Choose a reason for hiding this comment

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

Content looks good, just needs some clean up. Let's add some tests (I think agent/agentserver/server_test.go is a good example to work off of).

proxy/server/server.go Outdated Show resolved Hide resolved
proxy/server/preheat.go Outdated Show resolved Hide resolved
proxy/server/server.go Outdated Show resolved Hide resolved
proxy/server/preheat.go Outdated Show resolved Hide resolved
proxy/server/preheat.go Outdated Show resolved Hide resolved
proxy/server/preheat.go Outdated Show resolved Hide resolved
proxy/server/preheat.go Outdated Show resolved Hide resolved
proxy/server/preheat.go Outdated Show resolved Hide resolved
proxy/server/preheat.go Outdated Show resolved Hide resolved
proxy/server/preheat.go Outdated Show resolved Hide resolved
@mmpei
Copy link
Contributor Author

mmpei commented May 20, 2019

additional, i used to think the http server in proxy is mandatory.
making it to optional is due to the python UTs failed and config compatible.
and what is your opinion? @codygibb

@codygibb
Copy link
Contributor

additional, i used to think the http server in proxy is mandatory.
making it to optional is due to the python UTs failed and config compatible.
and what is your opinion? @codygibb

Agreed. Let's keep it optional in this diff, but I'll make it required in a follow up diff and fix anything that breaks.

mmpei added 2 commits May 20, 2019 17:35
Signed-off-by: peimingming <peimingming@corp.netease.com>
Signed-off-by: peimingming <peimingming@corp.netease.com>
@mmpei
Copy link
Contributor Author

mmpei commented May 21, 2019

it seems the fossa makes build failed. i have modified PR by your comments. and test is OK too including the log format. i will add some UT and raised a new PR later.

@codygibb
Copy link
Contributor

it seems the fossa makes build failed. i have modified PR by your comments. and test is OK too including the log format. i will add some UT and raised a new PR later.

Yeah the fossa integration is broken right now. Feel free to ignore for now.

Signed-off-by: peimingming <peimingming@corp.netease.com>
Copy link
Contributor

@codygibb codygibb left a comment

Choose a reason for hiding this comment

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

Tests look great! Thank you for putting this together!

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.

None yet

4 participants