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

kubeadm: imagePullPolicy option in init config #58960

Merged
merged 1 commit into from Feb 11, 2018

Conversation

@rosti
Contributor

rosti commented Jan 29, 2018

What this PR does / why we need it:
This PR adds imagePullPolicy option to the kubeadm init configuration file.

The new imagePullPolicy option is forwarded to the generated kubelet static pods for etcd, kube-apiserver, kube-controller-manager and kube-scheduler. This option allows for precise image pull policy specification for master nodes and thus for more tight control over images. It is useful in CI environments and in environments, where the user has total control over master VM templates (thus, the master VM templates can be preloaded with the required Docker images for the control plane services).

Special notes for your reviewer:

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/assign @luxas

Release note:

kubeadm: New "imagePullPolicy" option in the init configuration file, that gets forwarded to kubelet static pods to control pull policy for etcd and control plane images.
@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Jan 29, 2018

@rosti: Reiterating the mentions to trigger a notification:
@kubernetes/sig-cluster-lifecycle-pr-reviews

In response to this:

What this PR does / why we need it:
This PR adds imagePullPolicy option to the kubeadm init configuration file.

The new imagePullPolicy option is forwarded to the generated kubelet static pods for etcd, kube-apiserver, kube-controller-manager and kube-scheduler. This option allows for precise image pull policy specification for master nodes and thus for more tight control over images. It is useful in CI environments and in environments, where the user has total control over master VM templates (thus, the master VM templates can be preloaded with the required Docker images for the control plane services).

Special notes for your reviewer:

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/assign @luxas

Release note:

kubeadm: New "imagePullPolicy" option in the init configuration file, that gets forwarded to kubelet static pods to control pull policy for etcd and control plane images.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@rosti

This comment has been minimized.

Contributor

rosti commented Jan 29, 2018

/assign @timothysc

@jamiehannaford

What happens if someone sets ImagePullPolicy=Never on a node without the images? The control plane pods will be stuck in ErrImageNeverPull right?

If we want to allow Never, we should allow users to find out what images will be pulled, and then verify they exist locally first.

@@ -63,6 +64,9 @@ type MasterConfiguration struct {
// ImageRepository what container registry to pull control plane images from
ImageRepository string
// ImagePullPolicy that control plane images. Can be Always, IfNotPresent or Never.

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 29, 2018

Member

for control plane images

@@ -62,6 +63,8 @@ type MasterConfiguration struct {
// ImageRepository what container registry to pull control plane images from
ImageRepository string `json:"imageRepository"`
// ImagePullPolicy that control plane images. Can be Always, IfNotPresent or Never.
ImagePullPolicy v1.PullPolicy `json:"imagePullPolicy,omitempty" protobuf:"bytes,14,opt,name=imagePullPolicy,casttype=v1.PullPolicy"`

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 29, 2018

Member

we don't need the protobuf tag

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jan 29, 2018

@rosti thanks, this is useful. I agree with @jamiehannaford, as we still don't have an image lister command of any sort, have you considered this?

@timothysc

If we are going to do this we should be explicit about the defaults, also address other comments.

@rosti

This comment has been minimized.

Contributor

rosti commented Jan 30, 2018

Rebased on latest master and made the changes that @jamiehannaford suggested.

I checked the behavior with imagePullPolicy set to Never and no images present. Kubeadm times out (after ~30 minutes) with the very generic error message kubeletFailTempl. That message basically says "check with the kubelet service and logs to find what's wrong".
The kubelet logs, then, contain an error messages in the form Container image \"xyz:version\" is not present with pull policy of Never. This may be sufficient for now, but having error messages, that are immediate (as opposed to after some timeout) and more detailed on what went wrong is very nice.

Do you think, that we need to do the more comprehensive checks now or, possibly, leave this for another PR?

Having no value specified in the init config file for imagePullPolicy, leaves the exact decision to kubelet for this. It is the way things are done currently.

@timothysc

This comment has been minimized.

Member

timothysc commented Jan 31, 2018

@rosti - Just so I'm clear, if ImagePullPolicy is not specified it will default to nil and works as it is today?

@timothysc

please squash commits and verify minor question then lgtm

@rosti

This comment has been minimized.

Contributor

rosti commented Feb 1, 2018

Rebased on latest master and squashed commits (as per request).

@timothysc, you are right. Not setting imagePullPolicy in init config, will default it to nil and thus it won't serialize it to static pod YAMLs. Then kubelet will work just as it does now.
So not setting this option in init config won't change the current behavior.

@jamiehannaford

This comment has been minimized.

Member

jamiehannaford commented Feb 4, 2018

I'm still not comfortable with setting up users to fail with really unhelpful errors. How about we generate a warning if the user provides imagePullPolicy=Never? It could just list the image names listed here. That would be far better UX IMO and not too hard to implement. WDYT @rosti?

@rosti

This comment has been minimized.

Contributor

rosti commented Feb 4, 2018

@jamiehannaford, looks nice, will do that tomorrow.
I was also thinking about reducing the timeout for services to come up when imagePullPolicy == Never. Currently it's ~30 minutes, but that time includes possible download over the Internet (which we don't have when imagePullPolicy is Never). A couple of minutes should be enough.
What do you think?

@rosti

This comment has been minimized.

Contributor

rosti commented Feb 5, 2018

@jamiehannaford, @timothysc, can you take a look at this?

I expanded the kubeletFailTempl error message, to mention the possibility of missing images if imagePullPolicy is set to Never and to contain the image of etcd.
Also, reduced the timeout of the waiter, returned by getWaiter, from 30 minutes to 60 seconds if imagePullPolicy is Never.

And, of course, rebased on latest master.

@timothysc

I'll defer to @jamiehannaford on lgtm here, I don't have strong feelings b/c I think this will be an exception to the norm.

However please squash your commits.

/approve

@jamiehannaford

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 8, 2018

@fejta-bot

This comment has been minimized.

fejta-bot commented Feb 10, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 10, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@fejta-bot

This comment has been minimized.

fejta-bot commented Feb 10, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 10, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@fejta-bot

This comment has been minimized.

fejta-bot commented Feb 10, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 10, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@fejta-bot

This comment has been minimized.

fejta-bot commented Feb 10, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 10, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@fejta-bot

This comment has been minimized.

fejta-bot commented Feb 11, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 11, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@fejta-bot

This comment has been minimized.

fejta-bot commented Feb 11, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 11, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@fejta-bot

This comment has been minimized.

fejta-bot commented Feb 11, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 11, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@fejta-bot

This comment has been minimized.

fejta-bot commented Feb 11, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 11, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@fejta-bot

This comment has been minimized.

fejta-bot commented Feb 11, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 11, 2018

@fejta-bot: you can't request testing unless you are a kubernetes member.

In response to this:

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@fejta

This comment has been minimized.

Contributor

fejta commented Feb 11, 2018

@timothysc removing the needs-ok-to-test label does not make a PR ok-to-test.

Please use commands to mutate labels.

@jamiehannaford

This comment has been minimized.

Member

jamiehannaford commented Feb 11, 2018

/ok-to-test

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 11, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 11, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 19829a2 into kubernetes:master Feb 11, 2018

13 of 14 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation rosti authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce-canary Skipped
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@rosti rosti deleted the rosti:kubeadm-imagepullpolicy branch Feb 12, 2018

k8s-merge-robot added a commit that referenced this pull request May 21, 2018

Merge pull request #64096 from luxas/kubeadm_remove_imagepullpolicy
Automatic merge from submit-queue (batch tested with PRs 59414, 64096). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

kubeadm: Remove `.ImagePullPolicy` from the v1alpha2 API

**What this PR does / why we need it**:
So with `kubeadm config images list/pull` I don't think we need this anymore. Also I don't like this being in our API, as I think the purpose of why it's there can be achieved in other ways.
Instead, I propose to set this explicitely to `IfNotPresent`, and tell the user to prepull the images with `kubeadm config images pull` in case of an airgapped env (or `docker load` ofc) or he/she wants to achieve what `imagePullPolicy: Always` would do. If the images are already cached locally, `IfNotPresent` translates to the same as `Never`, i.e. don't pull (for ppl with no internet connection).


**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Part of cleaning up the API kubernetes/community#2131

**Special notes for your reviewer**:
This basically reverts: #58960

**Release note**:

```release-note
[action required] kubeadm: The `.ImagePullPolicy` field has been removed in the v1alpha2 API version. Instead it's set statically to `IfNotPresent` for all required images. If you want to always pull the latest images before cluster init (like what `Always` would do), run `kubeadm config images pull` before each `kubeadm init`. If you don't want the kubelet to pull any images at `kubeadm init` time, as you for instance don't have an internet connection, you can also run `kubeadm config images pull` before `kubeadm init` or side-load the images some other way (e.g. `docker load -i image.tar`). Having the images locally cached will result in no pull at runtime, which makes it possible to run without any internet connection.
```
@kubernetes/sig-cluster-lifecycle-pr-reviews @rosti @liztio @chuckha
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment