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

Pull image on container creation #496

Merged
merged 1 commit into from
Aug 5, 2019

Conversation

saschagrunert
Copy link
Member

I'm not sure if there were already some discussions if we want this or not, so I'd like to propose this one: We now try to pull the image from the container manifest before actually creating the container. This makes the overall usability a bit nicer.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 2, 2019
@k8s-ci-robot k8s-ci-robot requested review from mrunalp and runcom August 2, 2019 11:57
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 2, 2019
@@ -432,13 +446,19 @@ func CreateContainer(client pb.RuntimeServiceClient, opts createOptions) (string
}
}

// Try to pull the image before container creation
image := config.GetImage().GetImage()
if _, err := PullImage(iClient, image, nil); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea, but if local image is used, the pull operation would fail depending on the runtime implementations.

So, I think we'd better add an option here, so that the pull can be skipped in some cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I agree. I added the option to no-pull as well as the pull authentication related options (this should work too I guess).

Copy link
Member

Choose a reason for hiding this comment

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

thanks

Signed-off-by: Sascha Grunert <[email protected]>
Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2019
@k8s-ci-robot k8s-ci-robot merged commit 6f02b8c into kubernetes-sigs:master Aug 5, 2019
@saschagrunert saschagrunert deleted the pull-image branch August 5, 2019 12:39
@mikebrow
Copy link
Contributor

mikebrow commented Jun 9, 2020

But the command is create container not pull image. Oh man.. this is a "potential" performance hit and a "potential" behavior change, by default. Kublet has image pull policies.. did they add --no-pull?

quick check.. nope..

but that's is ok because kubelet calls the container runtime directly..

edited to reduce severity

@saschagrunert
Copy link
Member Author

But the command is create container not pull image. Oh man.. this is a huge performance hit and a huge behavior change, by default. Kublet has image pull policies.. did they add --no-pull?

quick check.. nope..

Hm okay to which "they" are you referring?

@mikebrow
Copy link
Contributor

mikebrow commented Jun 9, 2020

kublet, the maintainers of kublet...

@mikebrow
Copy link
Contributor

mikebrow commented Jun 9, 2020

@mikebrow
Copy link
Contributor

mikebrow commented Jun 9, 2020

sry just trying to figure out what this means...

@saschagrunert
Copy link
Member Author

saschagrunert commented Jun 9, 2020

Okay, the main intention of such PRs, like this one or the one added crictl run, is to mimic parts of the kubelet to be able to use crictl in a more convenient fashion. If any of those behavioral changes introducing regressions, then we can find a way to turn them off by default.

Sometimes avoiding breaking changes is not that easily achievable in conjunction with semver because we're just following k8s versions. :-/

@mikebrow
Copy link
Contributor

mikebrow commented Jun 9, 2020

perhaps a new config flag to use the old default.. eg by setting the default to no pull..

@mikebrow
Copy link
Contributor

mikebrow commented Jun 9, 2020

crictl run.. yeah makes sense there as trying to mimic docker run.. but most of the time I think kubelet is trying to follow pull policy in this regard. Some users want to run disconnected for example, others don't want to update their images regardless of if connected.

@saschagrunert
Copy link
Member Author

crictl run.. yeah makes sense there as trying to mimic docker run.. but most of the time I think kubelet is trying to follow pull policy in this regard. Some users want to run disconnected for example, others don't want to update their images regardless of if connected.

Yeah I see the point. We probably should turn it off by default. 👍

@mikebrow
Copy link
Contributor

mikebrow commented Jun 9, 2020

suggest adding the new default config flag... and pushing that backwards via service. Sorry didn't see this when it was pushed. So busy so many projects :-) Thx @saschagrunert

@mikebrow
Copy link
Contributor

crictl isn't being used by kublet for create so this isn't as important as I thought it might be

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants