Allow configurable etcd options #42246

jamiehannaford · 2017-02-28T13:37:45Z

What this PR does / why we need it:

Allows users to set the --listen-client-urls and --advertise-client-urls flags on etcd binaries for clusters set up with kubeadm.

Which issue this PR fixes:

As far as I can tell right now, other nodes in a cluster set up with kubeadm cannot communicate with the etcd static pod running on the master. This is needed in order to set up calico/canal SDN which needs access to a publicly addressable IPv4 before the overlay network and inter-cluster subnet is created.

Addresses kubernetes/enhancements#138 and kubernetes/enhancements#11.

Release note:

Users can now specify listen and advertise URLs for etcd in a kubeadm cluster

k8s-ci-robot · 2017-02-28T13:37:45Z

Hi @jamiehannaford. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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. I understand the commands that are listed here.

jamiehannaford · 2017-02-28T14:23:19Z

The overall solution to this problem I think is to create a NodePort etcd service (either manually or automatically with kubeadm) and have the binary bind to 0.0.0.0.

k8s-reviewable · 2017-03-02T02:20:07Z

This change is

pires · 2017-03-02T07:31:17Z

@kubernetes/sig-cluster-lifecycle-pr-reviews @luxas @lukemarsden

jamiehannaford · 2017-03-14T14:35:15Z

@pires @kubernetes/sig-cluster-lifecycle-pr-reviews @luxas @lukemarsden I know kubeadm is in code freeze due to moving over codebases - should I wait until that's happened or is there anything I can incorporate now?

luxas · 2017-03-14T15:09:05Z

@jamiehannaford Kubernetes as a project is in code freeze right now and no non-bugfix PRs can be merged at this point in the cycle. Hence the pending state...

kubeadm will eventually move to its own repo, but that will take some time yet before our machinery is able to do that.

jamiehannaford · 2017-03-14T15:27:28Z

@luxas I know but some of my other PRs have been reviewed but not merged. Maybe it will save time when the code freeze is lifted. I'd just like a general idea of whether this feature is approved/needed and if any of the implementation logic is off :)

pires

Overall this LGTM. I just don't like the flags.

pires · 2017-03-14T16:11:38Z

cmd/kubeadm/app/cmd/init.go

+	cmd.PersistentFlags().StringVar(
+		&cfg.Etcd.AdvertiseURL, "etcd-advertise-url", cfg.Etcd.AdvertiseURL,
+		"Specify a advertise-client-urls value to the etcd container",
+	)



Use configuration, not flags.

jamiehannaford · 2017-04-03T08:59:17Z

@pires Does this look okay now? I removed the flags

pires · 2017-04-03T11:35:58Z

cmd/kubeadm/app/master/manifests.go

+	if cfg.Etcd.AdvertiseURL != "" {
+		advertiseURL += fmt.Sprintf(",%s", cfg.Etcd.AdvertiseURL)
+	}
+


listenURL and advertiseURL should point to a same constant value.

I wasn't 100% sure what you meant, but I added a new default variable which both URLs point to. That DRYed it up a bit. Is that what you meant?

pires

/lgtm

pires · 2017-04-04T06:54:15Z

Ah, yes, the API changes break stuff. Please, check the test failure details and fix it.

luxas

There's nothing wrong with this PR per se, but I really think that if we do this, it should be generic from the get go.

Please align the logic here with what we have for all the other control plane components

luxas · 2017-04-05T11:08:25Z

cmd/kubeadm/app/apis/kubeadm/types.go

+	CAFile       string
+	CertFile     string
+	KeyFile      string
+	ListenURL    string


I think we really should go with a generic Args here instead of two hard-coded string fields.
Imagine I want to set the token authz mechanism. Eventually we would duplicate all etcd options, instead we should go with a more dynamic route with map[string]string as the other control plane components

luxas · 2017-04-05T11:09:13Z

cmd/kubeadm/app/master/manifests.go

+func getEtcdCommand(cfg *kubeadmapi.MasterConfiguration) []string {
+	defaultURL := "http://127.0.0.1:2379"
+
+	listenURL := defaultURL


Please use the same logic as the other control plane components here with default and overridable arguments.

jamiehannaford · 2017-04-05T14:17:50Z

@k8s-bot bazel test this

k8s-ci-robot · 2017-04-05T14:18:05Z

@jamiehannaford: you can't request testing unless you are a kubernetes member.

In response to this comment:

@k8s-bot bazel test this

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. I understand the commands that are listed here.

jamiehannaford · 2017-04-05T14:19:03Z

@luxas Thanks for the feedback, I've made things a bit more generic so folks can easily extend. Would you mind kicking off the tests again?

pires · 2017-04-05T16:47:14Z

@k8s-bot bazel test this

jamiehannaford · 2017-04-06T10:35:39Z

@luxas Agree about the dataDir stuff. I've added a new config key and removed the hard-coded references. Can you comment with ok to test so I can re-run tests to get them green?

dims · 2017-04-06T10:37:44Z

@k8s-bot ok to test

jamiehannaford · 2017-04-06T14:57:24Z

@k8s-bot non-cri e2e test this

luxas · 2017-04-06T20:14:37Z

@k8s-bot non-cri e2e test this

jamiehannaford · 2017-04-06T22:08:43Z

Looks like this is all green now

pires · 2017-04-07T09:58:01Z

/lgtm

luxas

LGTM, only one nit.
Thanks for doing this!

luxas · 2017-04-07T14:40:50Z

cmd/kubeadm/app/apis/kubeadm/env.go

@@ -31,7 +33,7 @@ func SetEnvParams() *EnvParams {

 	envParams := map[string]string{
 		"kubernetes_dir":  "/etc/kubernetes",
-		"host_etcd_path":  "/var/lib/etcd",
+		"host_etcd_path":  v1alpha1.DefaultEtcdDataDir,


Sorry for being unclear earlier, I don't think this is used anymore (if it is, switch to the new method)
It could/should be removed

No problem, I appreciate the feedback 👍 does this commit address what you mean? 4b065f8

jamiehannaford · 2017-04-07T19:46:12Z

@k8s-bot non-cri e2e test this

pires · 2017-04-08T10:07:07Z

/lgtm

luxas · 2017-04-08T14:46:37Z

@jamiehannaford LGTM, but please squash (unless you really think the commit sequence makes a lot of sense)

jamiehannaford · 2017-04-10T12:50:33Z

@luxas Sure - I squashed those commits into 1

jamiehannaford · 2017-04-18T11:02:21Z

Any update on this?

luxas

Hi, thanks for bearing with us. I've been away last week, digging myself up now from all the emails.

Thanks for the ping!

LGTM

luxas · 2017-04-18T11:46:13Z

/lgtm
/approve

k8s-github-robot · 2017-04-18T11:46:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jamiehannaford, luxas, pires

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

jamiehannaford · 2017-04-18T13:07:08Z

@k8s-bot gce etcd3 e2e test this

k8s-github-robot · 2017-04-18T15:25:49Z

Automatic merge from submit-queue

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 28, 2017

k8s-github-robot assigned pires Feb 28, 2017

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 28, 2017

pires suggested changes Mar 14, 2017

View reviewed changes

jamiehannaford force-pushed the add-etcd-flags-kubeadm branch from ced7fa6 to f3c3232 Compare March 31, 2017 10:25

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2017

pires suggested changes Apr 3, 2017

View reviewed changes

jamiehannaford force-pushed the add-etcd-flags-kubeadm branch from f3c3232 to 9f13b38 Compare April 3, 2017 12:54

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

pires approved these changes Apr 4, 2017

View reviewed changes

jamiehannaford force-pushed the add-etcd-flags-kubeadm branch from 9f13b38 to 3df45d0 Compare April 4, 2017 09:33

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 4, 2017

luxas reviewed Apr 5, 2017

View reviewed changes

luxas self-assigned this Apr 5, 2017

jamiehannaford force-pushed the add-etcd-flags-kubeadm branch from 3df45d0 to e5f9345 Compare April 5, 2017 12:52

jamiehannaford force-pushed the add-etcd-flags-kubeadm branch 3 times, most recently from 8140f56 to 91e41e6 Compare April 6, 2017 12:03

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

luxas reviewed Apr 7, 2017

View reviewed changes

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2017

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

Allow configurable etcd options

7e82985

jamiehannaford force-pushed the add-etcd-flags-kubeadm branch from 4b065f8 to 7e82985 Compare April 10, 2017 10:00

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 10, 2017

luxas approved these changes Apr 18, 2017

View reviewed changes

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

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

k8s-github-robot merged commit 4e17230 into kubernetes:master Apr 18, 2017

xilabao mentioned this pull request Apr 27, 2017

remove abandoned environment variable kubernetes/website#3557

Merged

Allow configurable etcd options #42246

Allow configurable etcd options #42246

Conversation

jamiehannaford commented Feb 28, 2017

k8s-ci-robot commented Feb 28, 2017

jamiehannaford commented Feb 28, 2017

k8s-reviewable commented Mar 2, 2017

pires commented Mar 2, 2017

jamiehannaford commented Mar 14, 2017

luxas commented Mar 14, 2017

jamiehannaford commented Mar 14, 2017

pires left a comment

Choose a reason for hiding this comment

pires Mar 14, 2017

Choose a reason for hiding this comment

jamiehannaford commented Apr 3, 2017

pires Apr 3, 2017

Choose a reason for hiding this comment

jamiehannaford Apr 3, 2017

Choose a reason for hiding this comment

pires left a comment

Choose a reason for hiding this comment

pires commented Apr 4, 2017

luxas left a comment

Choose a reason for hiding this comment

luxas Apr 5, 2017

Choose a reason for hiding this comment

luxas Apr 5, 2017

Choose a reason for hiding this comment

jamiehannaford commented Apr 5, 2017

k8s-ci-robot commented Apr 5, 2017

jamiehannaford commented Apr 5, 2017

pires commented Apr 5, 2017

jamiehannaford commented Apr 6, 2017

dims commented Apr 6, 2017

jamiehannaford commented Apr 6, 2017

luxas commented Apr 6, 2017

jamiehannaford commented Apr 6, 2017

pires commented Apr 7, 2017

luxas left a comment

Choose a reason for hiding this comment

luxas Apr 7, 2017

Choose a reason for hiding this comment

jamiehannaford Apr 7, 2017

Choose a reason for hiding this comment

jamiehannaford commented Apr 7, 2017

pires commented Apr 8, 2017

luxas commented Apr 8, 2017

jamiehannaford commented Apr 10, 2017

jamiehannaford commented Apr 18, 2017

luxas left a comment

Choose a reason for hiding this comment

luxas commented Apr 18, 2017

k8s-github-robot commented Apr 18, 2017

jamiehannaford commented Apr 18, 2017

k8s-github-robot commented Apr 18, 2017