Create volumeMount and hostPath for cloud config file #56535

dims · 2017-11-29T02:45:38Z

We have a way to specify the cloudProvider in kubeadm.conf. We also
add --cloud-config /etc/kubernetes/cloud-config to both the
kubernetes api server and controller manager yaml files if one exists
on the box. However we fail to make that file available to the
process running in the container. We need to make this cloud-config
file available to both processes similar to how controller-manager.conf
is passed to controller manager.

What this PR does / why we need it:
Fixes kubernetes/kubeadm#576

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 #

Special notes for your reviewer:

Release note:

NONE

dims · 2017-11-29T02:45:48Z

/kind bug

xiangpengzhao

just one nit. otherwise LGTM.

xiangpengzhao · 2017-11-29T04:11:36Z

cmd/kubeadm/app/phases/controlplane/volumes.go

+	// Read-only mount of the cloud config file if present
+	if cfg.CloudProvider != "" {
+		if _, err := os.Stat(DefaultCloudConfigPath); err == nil {
+			mounts.NewHostPathMount(kubeadmconstants.KubeAPIServer, kubeadmconstants.KubeCloudConfigVolumeName, DefaultCloudConfigPath, DefaultCloudConfigPath, true, &hostPathFileOrCreate)


Should hostPathFileOrCreate be hostPathDirectoryOrCreate?
or, if we use hostPathFileOrCreate, we should mount the cloud config file, not the directory.

After taking another looking at the code, seems like DefaultCloudConfigPath = "/etc/kubernetes/cloud-config" is a file? Hmm, the file name is a bit confusing...

should be FileOrCreate yes

Is this REALLY needed for the API server? Is there actually anything in the API server that can consume cloud code anymore...? I don't think so

xiangpengzhao · 2017-11-29T06:22:01Z

Oh, just find that this overlaps with https://github.com/kubernetes/kubernetes/pull/56513/files#diff-d4a6251d485cc68027cd68acd4398701R80. (But this PR is more reasonable than the overlapped part in that one.)

xiangpengzhao · 2017-11-29T06:22:20Z

/cc @luxas

dims · 2017-11-29T11:18:05Z

Ah thanks for the pointer @xiangpengzhao yes, i'd prefer a focused PR like this get in first as it targets a fresh deployment which we need to support better as well. I have to update the docs too

dims · 2017-11-29T11:18:37Z

@luxas would you consider this PR for 1.9 as it is clearly a bug for fresh installs?

luxas

Please make sure the unit test always passes and copy over the change in #56513 touching volumes_test.go

Small nits only, I approve this bug fix for v1.9

luxas · 2017-11-29T14:36:26Z

cmd/kubeadm/app/phases/controlplane/volumes.go

+	// Read-only mount of the cloud config file if present
+	if cfg.CloudProvider != "" {
+		if _, err := os.Stat(DefaultCloudConfigPath); err == nil {
+			mounts.NewHostPathMount(kubeadmconstants.KubeAPIServer, kubeadmconstants.KubeCloudConfigVolumeName, DefaultCloudConfigPath, DefaultCloudConfigPath, true, &hostPathFileOrCreate)


should be FileOrCreate yes

luxas · 2017-11-29T14:37:13Z

cmd/kubeadm/app/phases/controlplane/volumes.go

+	// Read-only mount of the cloud config file if present
+	if cfg.CloudProvider != "" {
+		if _, err := os.Stat(DefaultCloudConfigPath); err == nil {
+			mounts.NewHostPathMount(kubeadmconstants.KubeAPIServer, kubeadmconstants.KubeCloudConfigVolumeName, DefaultCloudConfigPath, DefaultCloudConfigPath, true, &hostPathFileOrCreate)


Is this REALLY needed for the API server? Is there actually anything in the API server that can consume cloud code anymore...? I don't think so

luxas · 2017-11-29T14:37:34Z

cmd/kubeadm/app/constants/constants.go

@@ -189,6 +189,9 @@ const (
 	// KubeConfigVolumeName specifies the name for the Volume that is used for injecting the kubeconfig to talk securely to the api server for a control plane component if applicable
 	KubeConfigVolumeName = "kubeconfig"

+	// KubeCloudConfigVolumeName specifies the name for the Volume that is used to inject the cloud configuration for kube-apiserver and kube-controller-manager component
+	KubeCloudConfigVolumeName = "kube-cloud-config"


please define the constant in the same volumes.go file as I did in #56513

k8s-github-robot · 2017-11-29T14:40:04Z

[MILESTONENOTIFIER] Milestone Pull Request Current

@dims @luxas @xiangpengzhao

Note: This pull request is marked as priority/critical-urgent, and must be updated every 1 day during code freeze.

Example update:

ACK.  In progress
ETA: DD/MM/YYYY
Risks: Complicated fix required

Pull Request Labels

sig/cluster-lifecycle: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/bug: Fixes a bug discovered during the current release.

Help

We have a way to specify the cloudProvider in kubeadm.conf. We also add `--cloud-config /etc/kubernetes/cloud-config` to both the kubernetes api server and controller manager yaml files if one exists on the box. However we fail to make that file available to the process running in the container. We need to make this `cloud-config` file available to both processes similar to how controller-manager.conf is passed to controller manager.

dims · 2017-11-29T20:31:35Z

/test pull-kubernetes-e2e-kubeadm-gce

luxas

/lgtm

k8s-github-robot · 2017-11-29T20:33:06Z

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 576

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

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kubeadm/OWNERS~~ [luxas]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-11-30T02:59:12Z

Automatic merge from submit-queue (batch tested with PRs 56400, 56535). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 29, 2017

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 29, 2017

k8s-github-robot assigned luxas and xiangpengzhao Nov 29, 2017

xiangpengzhao reviewed Nov 29, 2017

View reviewed changes

k8s-ci-robot requested a review from luxas November 29, 2017 06:22

xiangpengzhao mentioned this pull request Nov 29, 2017

kubeadm: Fix bugs in the codebase related to upgrades/downgrades #56513

Merged

luxas approved these changes Nov 29, 2017

View reviewed changes

luxas added this to the v1.9 milestone Nov 29, 2017

k8s-github-robot added the milestone/incomplete-labels label Nov 29, 2017

luxas added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. status/approved-for-milestone and removed milestone/incomplete-labels labels Nov 29, 2017

dims force-pushed the create-volume-mount-and-host-path-for-cloud-config branch from 32fe44a to 971deaf Compare November 29, 2017 14:52

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 29, 2017

dims force-pushed the create-volume-mount-and-host-path-for-cloud-config branch from 971deaf to a11f984 Compare November 29, 2017 20:28

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 29, 2017

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

luxas approved these changes Nov 29, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 29, 2017

k8s-github-robot merged commit 1602708 into kubernetes:master Nov 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create volumeMount and hostPath for cloud config file #56535

Create volumeMount and hostPath for cloud config file #56535

dims commented Nov 29, 2017

dims commented Nov 29, 2017

xiangpengzhao left a comment

xiangpengzhao Nov 29, 2017

xiangpengzhao Nov 29, 2017

luxas Nov 29, 2017

luxas Nov 29, 2017

xiangpengzhao commented Nov 29, 2017

xiangpengzhao commented Nov 29, 2017

dims commented Nov 29, 2017

dims commented Nov 29, 2017

luxas left a comment

luxas Nov 29, 2017

luxas Nov 29, 2017

luxas Nov 29, 2017

dims Nov 29, 2017

k8s-github-robot commented Nov 29, 2017

dims commented Nov 29, 2017

luxas left a comment

k8s-github-robot commented Nov 29, 2017

k8s-github-robot commented Nov 30, 2017

Create volumeMount and hostPath for cloud config file #56535

Create volumeMount and hostPath for cloud config file #56535

Conversation

dims commented Nov 29, 2017

dims commented Nov 29, 2017

xiangpengzhao 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

xiangpengzhao commented Nov 29, 2017

xiangpengzhao commented Nov 29, 2017

dims commented Nov 29, 2017

dims commented Nov 29, 2017

luxas 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

k8s-github-robot commented Nov 29, 2017

dims commented Nov 29, 2017

luxas left a comment

Choose a reason for hiding this comment

k8s-github-robot commented Nov 29, 2017

k8s-github-robot commented Nov 30, 2017