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.

Sign up for GitHub

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

Jump to bottom

rkt: Force rkt fetch to fetch from remote to conform the image pull policy. #31378

Merged
merged 1 commit into from
Aug 26, 2016
Merged

rkt: Force rkt fetch to fetch from remote to conform the image pull policy. #31378

merged 1 commit into from
Aug 26, 2016

Conversation

yifan-gu
Copy link
Contributor

@yifan-gu yifan-gu commented Aug 24, 2016 •
edited
Loading

Fix #27646

Use --no-store option for rkt fetch to force it to fetch from remote.
However, --no-store will fetch the remote image regardless of whether the content of the image has changed or not.
This causes performance downgrade when the image tag is ':latest' and the image pull policy is 'always'.
The issue is tracked in rkt/rkt#2937.

This change is Reviewable

@yifan-gu yifan-gu added this to the rktnetes-v1.1 milestone Aug 24, 2016
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Aug 24, 2016
@yifan-gu yifan-gu added release-note-none Denotes a PR that doesn't merit a release note. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 24, 2016
@yifan-gu yifan-gu modified the milestones: v1.4, rktnetes-v1.1 Aug 24, 2016
@yifan-gu yifan-gu changed the title rkt: Force rkt fetch to fetch from remote to conform the fetch policy. rkt: Force rkt fetch to fetch from remote to conform the image pull policy. Aug 24, 2016
@k8s-github-robot k8s-github-robot mentioned this pull request Aug 24, 2016
Closed
@euank
Copy link
Contributor

euank commented Aug 25, 2016

Since kubelet only calls PullImage when it intends to try to update an image (based on the ImagePullPolicy stuff) this indeed seems like the logically correct thing to do and fixes the bug I reported.

Unfortunately, because it appears rkt fetch --no-store docker://... images do not respect existing caches, this operation will not only check for a new image, but always download it even if it has not changed. However, there's an upstream issue to fix this in a future version of rkt, and while this change in behaviour is slower in some (many) cases, it's definitely more correct.

We should update the known issues to note this new and less-egregious known-issue as well.

:lgtm:

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@yifan-gu yifan-gu force-pushed the rkt_fetch_no_store branch from 2076516 to 4deb1f4 Compare August 25, 2016 21:41
@yifan-gu
Copy link
Contributor Author

Link rkt/rkt#2937 in the comments.

@yifan-gu yifan-gu force-pushed the rkt_fetch_no_store branch from 4deb1f4 to de402ac Compare August 25, 2016 21:44
@yifan-gu yifan-gu added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2016
@yifan-gu
Copy link
Contributor Author

@k8s-bot test this please, issue #IGNORE

@k8s-bot
Copy link

k8s-bot commented Aug 26, 2016

GCE e2e build/test passed for commit de402ac.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 9deb18b into kubernetes:master Aug 26, 2016
@yifan-gu yifan-gu deleted the rkt_fetch_no_store branch August 26, 2016 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@euank euank

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
6 participants
@yifan-gu @euank @k8s-bot @k8s-github-robot @googlebot @wojtek-t