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

kubeadm: Remove .ImagePullPolicy from the v1alpha2 API #64096

Merged

Conversation

luxas
Copy link
Member

@luxas luxas commented May 21, 2018

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:

[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

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 21, 2018
@k8s-ci-robot k8s-ci-robot added area/kubeadm release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 21, 2018
Copy link
Contributor

@rosti rosti left a comment

Choose a reason for hiding this comment

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

I don't rely on this config option anymore, so it's OK by me.

- {{ .APIServerImage }}
- {{ .ControllerManagerImage }}
- {{ .SchedulerImage }}
- {{ .EtcdImage }} (only if no external etcd endpoints are configured)
- You can check or miligate this in beforehand with "kubeadm config images pull" to make sure the images
Copy link
Contributor

Choose a reason for hiding this comment

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

typo? "mitigate"? :)

@@ -76,7 +76,7 @@ func GetStaticPodSpecs(cfg *kubeadmapi.MasterConfiguration, k8sVersion *version.
kubeadmconstants.KubeAPIServer: staticpodutil.ComponentPod(v1.Container{
Name: kubeadmconstants.KubeAPIServer,
Image: images.GetCoreImage(kubeadmconstants.KubeAPIServer, cfg.GetControlPlaneImageRepository(), cfg.KubernetesVersion, cfg.UnifiedControlPlaneImage),
ImagePullPolicy: cfg.ImagePullPolicy,
ImagePullPolicy: v1.PullIfNotPresent,
Copy link
Contributor

Choose a reason for hiding this comment

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

kubelet defaults to IfNotPresent if this is not set in the static pod YAML. Thus setting it here explicitly can be omitted (although it doesn't do any harm to be set explicitly here).

Copy link
Contributor

@chuckha chuckha left a comment

Choose a reason for hiding this comment

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

I think this makes sense and cleans up a field in the API 👍.

One concern I have with this path (which admittedly is beyond this PR) is that the scope of preflight checks will expand when we start pulling images with checks. To me, that feels like they are no longer checks but preflight actions. It might be that checks are already allowed to be mutative actions, but my assertion based on the name is that they were seeing if the system was in the right state and reporting errors, not fixing it for the user.

- {{ .APIServerImage }}
- {{ .ControllerManagerImage }}
- {{ .SchedulerImage }}
- {{ .EtcdImage }} (only if no external etcd endpoints are configured)
- You can check or miligate this in beforehand with "kubeadm config images pull" to make sure the images
Copy link
Contributor

Choose a reason for hiding this comment

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

typo in 'miligate', I assume you mean 'mitigate'. Also drop the word 'in'

Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

ImagePullPolicy is useful, some folks have strict requirements to pull from their own registry (always).

I'm ok with this feature addition.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2018
Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

k, we just need to document the change as part of the workflow.

/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 May 21, 2018
@timothysc timothysc removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: luxas, timothysc

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

@luxas
Copy link
Member Author

luxas commented May 21, 2018

/retest

@luxas
Copy link
Member Author

luxas commented May 21, 2018

ImagePullPolicy is useful, some folks have strict requirements to pull from their own registry (always).

We discussed this with @timothysc, and kubeadm config images pull && kubeadm init equals Always, so with that flow (which we will document), we don't need this and can remove to get a cleaner struct.

Also noteworthy is that kubernetes/kubeadm#825 is coming to prepull images in the preflight checks if images don't exist locally already. This will help us get from the place where we wait for a long time for the kubelet to pull the images, with poor UX as kubeadm can hang if there's no internet connection. These are slightly related, so hence the mentioned here.

@k8s-github-robot
Copy link

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 here.

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. area/kubeadm 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. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants