kubeadm: set pod-infra-container-image for the kubelet #70603

chuckha · 2018-11-02T22:42:20Z

The kubelet allows you to set --pod-infra-container-image
(also called PodSandboxImage in the kubelet config),
which can be a custom location to the "pause" image in the case
of Docker. Other CRIs are not supported.

Set the CLI flag for the Docker case in flags.go using
WriteKubeletDynamicEnvFile().

This PR also cleans up some unwrapped errors.

What type of PR is this?

Uncomment only one, leave it on its own line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
This PR properly allows image repository to be passed to the kubelet for the sandbox image.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes kubernetes/kubeadm#1003

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Kubeadm now respects the custom image registry configuration across joins and upgrades. Kubeadm passes the custom registry to the kubelet for a custom pause container.

chuckha · 2018-11-02T22:43:33Z

/priority critical-urgent

(copying prio from bug kubernetes/kubeadm#1003)

chuckha · 2018-11-02T22:44:17Z

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews @mruepp

k8s-ci-robot · 2018-11-02T22:44:18Z

@chuckha: GitHub didn't allow me to request PR reviews from the following users: mruepp.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews @mruepp

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.

chuckha · 2018-11-02T22:45:39Z

docker images after an init:

812083501456.dkr.ecr.us-west-2.amazonaws.com/kube-proxy                v1.12.2             15e9da1ca195        9 days ago          96.5MB
812083501456.dkr.ecr.us-west-2.amazonaws.com/kube-apiserver            v1.12.2             51a9c329b7c5        9 days ago          194MB
812083501456.dkr.ecr.us-west-2.amazonaws.com/kube-controller-manager   v1.12.2             15548c720a70        9 days ago          164MB
812083501456.dkr.ecr.us-west-2.amazonaws.com/etcd                      3.2.24              3cab8e1b9802        6 weeks ago         220MB
812083501456.dkr.ecr.us-west-2.amazonaws.com/pause                     3.1                 da86e6ba6ca1        10 months ago       742kB

docker images after a join:

812083501456.dkr.ecr.us-west-2.amazonaws.com/pause        3.1                 da86e6ba6ca1        10 months ago       742kB

neolit123

@chuckha thank you fixing this.
added a couple of comments about possible small changes.

neolit123 · 2018-11-02T23:01:19Z

cmd/kubeadm/app/constants/constants.go

@@ -250,6 +250,9 @@ const (
 	// PauseVersion indicates the default pause image version for kubeadm
 	PauseVersion = "3.1"

+	// DefaultPauseImageVersion indicates the default version for the pause image
+	DefaultPauseImageVersion = "3.1"


seems to be duplicate of the above PauseVersion constant.

lol yes, it does 🤦‍♂️

neolit123 · 2018-11-02T23:02:34Z

cmd/kubeadm/app/images/images_test.go

+			cfg: &kubeadmapi.ClusterConfiguration{
+				ImageRepository: "test.repo",
+			},
+			expected: "test.repo/pause:" + constants.DefaultPauseImageVersion,


probably should be PauseVersion. same as GetPauseImage().

neolit123 · 2018-11-02T23:03:40Z

cmd/kubeadm/app/util/config/cluster.go

@@ -93,7 +91,7 @@ func getInitConfigurationFromCluster(kubeconfigDir string, client clientset.Inte
 	// Also, the config map really should be KubeadmConfigConfigMap...
 	configMap, err := client.CoreV1().ConfigMaps(metav1.NamespaceSystem).Get(constants.KubeadmConfigConfigMap, metav1.GetOptions{})
 	if err != nil {
-		return nil, err
+		return nil, errors.Wrap(err, "failed to get config map")


these Wrap calls are possibly outside of the scope of the PR?
but SGTM still.

Oh yeah, I added these because when I wanted them when I was debugging. not strictly necessary, but imo an improvement

I will never complain about this...

neolit123 · 2018-11-02T23:09:02Z

cmd/kubeadm/app/cmd/phases/kubelet.go

@@ -96,7 +97,8 @@ func runKubeletStart(c workflow.RunData) error {
 	// Write env file with flags for the kubelet to use. We do not need to write the --register-with-taints for the master,
 	// as we handle that ourselves in the markmaster phase
 	// TODO: Maybe we want to do that some time in the future, in order to remove some logic from the markmaster phase?
-	if err := kubeletphase.WriteKubeletDynamicEnvFile(&data.Cfg().NodeRegistration, data.Cfg().FeatureGates, false, data.KubeletDir()); err != nil {
+	pauseImage := images.GetPauseImage(data.Cfg().ClusterConfiguration.ImageRepository)
+	if err := kubeletphase.WriteKubeletDynamicEnvFile(&data.Cfg().NodeRegistration, data.Cfg().FeatureGates, pauseImage, false, data.KubeletDir()); err != nil {


something that @rosti suggested in the original PR was that we should pass ClusterConfiguration instead of feature gates and pause image to WriteKubeletDynamicEnvFile:
https://github.com/kubernetes/kubernetes/pull/66798/files#r206508620

In fact, we can replace the first three parameters with InitConfiguration here and move the GetPauseImage call to WriteKubeletDynamicEnvFile. This will have the benefit of tidying up the code and removing duplicate logic between here and postupgrade.go

neolit123 · 2018-11-02T23:10:54Z

cmd/kubeadm/app/phases/kubelet/flags.go

@@ -56,6 +57,7 @@ func WriteKubeletDynamicEnvFile(nodeRegOpts *kubeadmapi.NodeRegistrationOptions,
 	flagOpts := kubeletFlagsOpts{
 		nodeRegOpts:              nodeRegOpts,
 		featureGates:             featureGates,
+		pauseImage:               pauseImage,


i guess we can keep featureGates and pauseImage as part of kubeletFlagsOpts, but obtain them from a ClusterConfiguration argument.

rosti

Overall looks OK. A few errors.Wrap and some empty lines removed from imports look like not belonging to this change.

rosti · 2018-11-05T10:55:53Z

cmd/kubeadm/app/cmd/join.go

@@ -532,11 +533,20 @@ func (j *Join) BootstrapKubelet(tlsBootstrapCfg *clientcmdapi.Config) error {
 		return err
 	}

+	imageRegistry := kubeadmapiv1beta1.DefaultImageRepository


Yes, I know that "registry" is a more accurate term here, but I think, that we should be consistent with the reset of kubeadm and use the term "repository" here.

hah, ok, that's reasonable, I will continue down the repository road

rosti · 2018-11-05T11:00:44Z

cmd/kubeadm/app/images/images.go

@@ -53,16 +53,19 @@ func GetEtcdImage(cfg *kubeadmapi.ClusterConfiguration) string {
 	return GetGenericImage(cfg.ImageRepository, constants.Etcd, etcdImageTag)
 }

+// GetPauseImage returns the image for the "pause" container
+func GetPauseImage(registry string) string {


My vision for the funcs in the images files is for them to take ClusterConfiguration and then figure out from there how to generate the image string. This way we can keep future changes to the way we generate the images to this file only.
I know, that this will look like an overkill for GetPauseImage, but I think, that this may have benefits in the future and will keep things consistent with the rest of the bunch here.

rosti · 2018-11-05T11:07:03Z

cmd/kubeadm/app/cmd/phases/kubelet.go

@@ -96,7 +97,8 @@ func runKubeletStart(c workflow.RunData) error {
 	// Write env file with flags for the kubelet to use. We do not need to write the --register-with-taints for the master,
 	// as we handle that ourselves in the markmaster phase
 	// TODO: Maybe we want to do that some time in the future, in order to remove some logic from the markmaster phase?
-	if err := kubeletphase.WriteKubeletDynamicEnvFile(&data.Cfg().NodeRegistration, data.Cfg().FeatureGates, false, data.KubeletDir()); err != nil {
+	pauseImage := images.GetPauseImage(data.Cfg().ClusterConfiguration.ImageRepository)
+	if err := kubeletphase.WriteKubeletDynamicEnvFile(&data.Cfg().NodeRegistration, data.Cfg().FeatureGates, pauseImage, false, data.KubeletDir()); err != nil {


In fact, we can replace the first three parameters with InitConfiguration here and move the GetPauseImage call to WriteKubeletDynamicEnvFile. This will have the benefit of tidying up the code and removing duplicate logic between here and postupgrade.go

chuckha · 2018-11-07T19:48:57Z

I'm not sure why github is still showing me the outdated comments but I think this is ready for another review @rosti @neolit123.

neolit123 · 2018-11-07T19:58:37Z

cmd/kubeadm/app/images/images_test.go:147:28: cannot use tc.cfg.ImageRepository (variable of type string) as *k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm.ClusterConfiguration value in argument to GetPauseImage

images_test needs an update.

neolit123

thanks for the update @chuckha
LGTM added one minor comment to fix CI.

neolit123 · 2018-11-07T19:59:30Z

cmd/kubeadm/app/images/images_test.go

+	}
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			actual := GetPauseImage(tc.cfg.ImageRepository)


neolit123 · 2018-11-07T21:35:59Z

/retest

chuckha · 2018-11-08T15:19:13Z

/assign @timothysc

chuckha · 2018-11-09T16:40:28Z

/test pull-kubernetes-integration

chuckha · 2018-11-09T16:41:01Z

/test pull-kubernetes-integration

chuckha · 2018-11-09T17:26:38Z

/retest

chuckha · 2018-11-09T18:51:48Z

/test pull-kubernetes-e2e-kops-aws

The kubelet allows you to set `--pod-infra-container-image` (also called `PodSandboxImage` in the kubelet config), which can be a custom location to the "pause" image in the case of Docker. Other CRIs are not supported. Set the CLI flag for the Docker case in flags.go using WriteKubeletDynamicEnvFile().

neolit123 · 2018-11-13T18:40:42Z

/retest

neolit123 · 2018-11-13T19:18:59Z

/retest

timothysc

Nice work!
/lgtm
/approve

timothysc · 2018-11-13T20:12:42Z

cmd/kubeadm/app/util/config/cluster.go

@@ -93,7 +91,7 @@ func getInitConfigurationFromCluster(kubeconfigDir string, client clientset.Inte
 	// Also, the config map really should be KubeadmConfigConfigMap...
 	configMap, err := client.CoreV1().ConfigMaps(metav1.NamespaceSystem).Get(constants.KubeadmConfigConfigMap, metav1.GetOptions{})
 	if err != nil {
-		return nil, err
+		return nil, errors.Wrap(err, "failed to get config map")


I will never complain about this...

k8s-ci-robot · 2018-11-13T20:13:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha, 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:

~~cmd/kubeadm/OWNERS~~ [timothysc]

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

timothysc · 2018-11-13T20:14:11Z

@chuckha I would revise the release notes to denote the behavior on the kubelet as well.

chuckha · 2018-11-13T20:31:46Z

@timothysc updated

kevinbin · 2019-12-16T08:52:01Z

not support contained CRI?

rosti · 2019-12-16T09:42:48Z

not support contained CRI?

Currently, this option is only for Docker. If you use any other CRI you have to check with its documentation and pass an equivalent option manually.

k8s-ci-robot requested review from detiber and xiangpengzhao November 2, 2018 22:42

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 2, 2018

k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 2, 2018

neolit123 approved these changes Nov 2, 2018

View reviewed changes

rosti reviewed Nov 5, 2018

View reviewed changes

chuckha force-pushed the pod-infra-container-image branch 3 times, most recently from 2900bb5 to 36f65e3 Compare November 7, 2018 19:47

neolit123 approved these changes Nov 7, 2018

View reviewed changes

chuckha force-pushed the pod-infra-container-image branch from 36f65e3 to 570457c Compare November 7, 2018 20:22

k8s-ci-robot assigned timothysc Nov 8, 2018

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2018

chuckha force-pushed the pod-infra-container-image branch from 570457c to 650714d Compare November 9, 2018 16:11

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2018

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2018

chuckha force-pushed the pod-infra-container-image branch from 650714d to 8a5d21b Compare November 13, 2018 15:19

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2018

chuckha force-pushed the pod-infra-container-image branch from 8a5d21b to 9a37f2d Compare November 13, 2018 18:10

timothysc approved these changes Nov 13, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2018

timothysc added this to the v1.13 milestone Nov 13, 2018

k8s-ci-robot merged commit 3cba75e into kubernetes:master Nov 13, 2018

neolit123 mentioned this pull request Jun 21, 2019

kubeadm: apply taints on non-control-plane node join #79107

Merged

alex-vmw mentioned this pull request Nov 19, 2019

Kubeadm: upgrade apply & upgrade node always overwrite Kubeadm-env file #85412

Merged

alex-vmw mentioned this pull request Dec 3, 2019

Write kubelet env file on kubeadm upgrade node kubernetes/kubeadm#1916

Closed

kubeadm: set pod-infra-container-image for the kubelet #70603

kubeadm: set pod-infra-container-image for the kubelet #70603

Conversation

chuckha commented Nov 2, 2018 • edited Loading

chuckha commented Nov 2, 2018

chuckha commented Nov 2, 2018

k8s-ci-robot commented Nov 2, 2018

chuckha commented Nov 2, 2018

neolit123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosti left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckha commented Nov 7, 2018

neolit123 commented Nov 7, 2018

neolit123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 commented Nov 7, 2018

chuckha commented Nov 8, 2018

chuckha commented Nov 9, 2018 • edited Loading

chuckha commented Nov 9, 2018

chuckha commented Nov 9, 2018

chuckha commented Nov 9, 2018

neolit123 commented Nov 13, 2018

neolit123 commented Nov 13, 2018

timothysc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 13, 2018

timothysc commented Nov 13, 2018

chuckha commented Nov 13, 2018

kevinbin commented Dec 16, 2019

rosti commented Dec 16, 2019

chuckha commented Nov 2, 2018 •

edited

Loading

neolit123 Nov 2, 2018 •

edited

Loading

rosti left a comment •

edited

Loading

chuckha commented Nov 9, 2018 •

edited

Loading