Add KubeletPreferredAddressTypes #1083

vjm · 2016-12-07T04:03:02Z

This change is

k8s-ci-robot · 2016-12-07T04:03:03Z

Hi @vjm. 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.

If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2016-12-07T04:03:05Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

If you have questions or suggestions related to this bot's behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

k8s-ci-robot · 2016-12-07T04:03:05Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

If you have questions or suggestions related to this bot's behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

vjm · 2016-12-07T04:07:21Z

@k8s-ci-robot I signed it!

justinsb

LGTM once typo is fixed - thanks!

justinsb · 2016-12-07T14:24:33Z

pkg/apis/kops/componentconfig.go

+	TokenAuthFile         		string   `json:"tokenAuthFile,omitempty" flag:"token-auth-file"`
+	AllowPrivileged       		*bool    `json:"allowPrivileged,omitempty" flag:"allow-privileged"`
+	APIServerCount        		*int     `json:"apiServerCount,omitempty" flag:"apiserver-count"`
+	KubletPreferredAddressTypes	[]string `json:"kubletPreferredAddressTypes,omitempty" flag:"kubelet-preferred-address-types"`


Typo: Kublet vs Kubelet (in the field but more importantly in the JSON)

justinsb · 2016-12-07T14:24:49Z

upup/models/config/components/kube-apiserver/kube-apiserver.options

@@ -16,3 +16,7 @@ KubeAPIServer:
  LogLevel: 2
  AllowPrivileged: true
  Image: {{ Image "kube-apiserver" }}
+  KubletPreferredAddressTypes:


Will need to update this one also

chrislovecnm · 2016-12-07T16:05:15Z

@vjm need you to sign the CLA in order to merge your code. Once yah update @justinsb's recommendations :)

vjm · 2016-12-07T16:43:19Z

@chrislovecnm I signed the CLA, all set there.

@justinsb I fixed these typos, but am still struggling to verify locally. Any guidance on how to check that these flags are being passed? When I add them to the YAML options using kops -v 5 edit cluster I don't see them being persisted. Is there a better way to test?

vjm · 2016-12-09T03:44:34Z

@justinsb @chrislovecnm any updates on this?

krisnova · 2016-12-11T19:44:12Z

pkg/apis/kops/componentconfig.go

@@ -374,20 +374,21 @@ type KubeAPIServerConfig struct {

 	LogLevel int `json:"logLevel,omitempty" flag:"v"`



What's with the formatting here? Not that it should matter if it's just formatting - but do we need this in the PR?

It's gofmt. The alternative is an extra line break to separate the fields.

justinsb

Sorry - I realized this flag isn't supported on 1.4 so needs more special treatment.

justinsb · 2016-12-15T20:49:33Z

pkg/apis/kops/componentconfig.go

@@ -374,20 +374,21 @@ type KubeAPIServerConfig struct {

 	LogLevel int `json:"logLevel,omitempty" flag:"v"`



It's gofmt. The alternative is an extra line break to separate the fields.

justinsb · 2016-12-15T20:53:22Z

upup/models/config/components/kube-apiserver/kube-apiserver.options

@@ -16,3 +16,7 @@ KubeAPIServer:
  LogLevel: 2
  AllowPrivileged: true
  Image: {{ Image "kube-apiserver" }}
+  KubeletPreferredAddressTypes:


I've realized you can only pass this in 1.5 or later.

I think you need to put this in a _k8s_1_5 magic directory (like we do for ConfigureCBR0, though the "other way round"). Or - if these are the defaults - simply not specify them unless you are overriding.

Are we changing the defaults here?

followed the same pattern as AnonymousAuth

chrislovecnm · 2016-12-19T00:52:52Z

Just to let you know this #1183 will probably impact this PR. I appreciate your patience, and help! This PR is a big change that has some huge benefits.

chrislovecnm · 2016-12-19T22:41:36Z

@justinsb recommended some changes :)

#1206

vjm · 2016-12-20T15:02:55Z

@chrislovecnm thanks for the response -- anything I need to be doing?

chrislovecnm · 2016-12-20T15:21:28Z

@vjm we good to go?

vjm · 2016-12-20T17:20:49Z

@chrislovecnm now we are! updated and working

chrislovecnm

We need documentation for this in a markdown please. Also you have two choices :)

include unit tests
file an issue for a unit test ~ and do it before your next PR.

Thanks

Chris

chrislovecnm · 2016-12-20T17:22:47Z

Makefile

@@ -251,7 +251,7 @@ examples:

 apimachinery:
 	#go install ./cmd/libs/go2idl/conversion-gen
-	~/k8s/bin/conversion-gen  --input-dirs k8s.io/kops/pkg/apis/kops/v1alpha1 --v=8  --output-file-base=zz_generated.conversion
+	${GOPATH}/bin/conversion-gen --skip-unsafe=true --input-dirs k8s.io/kops/pkg/apis/kops/v1alpha1 --v=8  --output-file-base=zz_generated.conversion


Can we back this out? If needed can we put it in a separate commit at least?

Not a PR a commit

chrislovecnm · 2016-12-20T17:23:19Z

pkg/apis/kops/componentconfig.go

@@ -380,6 +380,8 @@ type KubeAPIServerConfig struct {
 	RuntimeConfig map[string]string `json:"runtimeConfig,omitempty" flag:"runtime-config"`

 	AnonymousAuth *bool `json:"anonymousAuth,omitempty" flag:"anonymous-auth"`
+
+	KubeletPreferredAddressTypes []string `json:"kubeletPreferredAddressTypes,omitempty" flag:"kubelet-preferred-address-types"`


should we add documentation on wth this thing is?

Probably not worth it for these - they directly map to the flags. Eventually this will be part of the k8s API, and we'll be able to delete this one.

chrislovecnm · 2016-12-20T17:23:41Z

pkg/apis/kops/v1alpha1/componentconfig.go

@@ -377,6 +377,8 @@ type KubeAPIServerConfig struct {
 	RuntimeConfig         map[string]string `json:"runtimeConfig,omitempty" flag:"runtime-config"`

 	AnonymousAuth *bool `json:"anonymousAuth,omitempty" flag:"anonymous-auth"`
+
+	KubeletPreferredAddressTypes []string `json:"kubeletPreferredAddressTypes,omitempty" flag:"kubelet-preferred-address-types"`


pkg/apis/kops/v1alpha1/componentconfig.go I think we need pkg/apis/kops/v1alpha2/componentconfig.go

I think it's there?

chrislovecnm · 2016-12-20T17:24:14Z

pkg/apis/kops/v1alpha1/zz_generated.conversion.go

@@ -199,8 +199,7 @@ func autoConvert_v1alpha1_ClusterSpec_To_kops_ClusterSpec(in *ClusterSpec, out *
 	if in.Topology != nil {
 		in, out := &in.Topology, &out.Topology
 		*out = new(kops.TopologySpec)
-		// TODO: Inefficient conversion - can we improve it?


Can you file an issue please.

This is autogen

justinsb

LGTM. I presume that's a merge conflict on the Makefile change, but that a rebase will fix it

justinsb · 2016-12-20T18:18:33Z

pkg/apis/kops/componentconfig.go

@@ -380,6 +380,8 @@ type KubeAPIServerConfig struct {
 	RuntimeConfig map[string]string `json:"runtimeConfig,omitempty" flag:"runtime-config"`

 	AnonymousAuth *bool `json:"anonymousAuth,omitempty" flag:"anonymous-auth"`
+
+	KubeletPreferredAddressTypes []string `json:"kubeletPreferredAddressTypes,omitempty" flag:"kubelet-preferred-address-types"`


Probably not worth it for these - they directly map to the flags. Eventually this will be part of the k8s API, and we'll be able to delete this one.

justinsb · 2016-12-20T18:18:57Z

pkg/apis/kops/v1alpha2/componentconfig.go

@@ -133,6 +133,10 @@ type KubeAPIServerConfig struct {
 	// for KubeAPIServer, concatenated with commas. ex: `--runtime-config=key1=value1,key2=value2`.
 	// Use this to enable alpha resources on kube-apiserver
 	RuntimeConfig map[string]string `json:"runtimeConfig,omitempty" flag:"runtime-config"`
+
+	AnonymousAuth *bool `json:"anonymousAuth,omitempty" flag:"anonymous-auth"`


Oops - nice find :-)

justinsb · 2016-12-20T18:19:10Z

pkg/apis/kops/v1alpha1/zz_generated.conversion.go

@@ -199,8 +199,7 @@ func autoConvert_v1alpha1_ClusterSpec_To_kops_ClusterSpec(in *ClusterSpec, out *
 	if in.Topology != nil {
 		in, out := &in.Topology, &out.Topology
 		*out = new(kops.TopologySpec)
-		// TODO: Inefficient conversion - can we improve it?


This is autogen

justinsb · 2016-12-20T18:19:22Z

pkg/apis/kops/v1alpha1/componentconfig.go

@@ -377,6 +377,8 @@ type KubeAPIServerConfig struct {
 	RuntimeConfig         map[string]string `json:"runtimeConfig,omitempty" flag:"runtime-config"`

 	AnonymousAuth *bool `json:"anonymousAuth,omitempty" flag:"anonymous-auth"`
+
+	KubeletPreferredAddressTypes []string `json:"kubeletPreferredAddressTypes,omitempty" flag:"kubelet-preferred-address-types"`


I think it's there?

justinsb · 2016-12-20T18:40:06Z

LGTM - thanks :-)

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Dec 7, 2016

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 7, 2016

justinsb requested changes Dec 7, 2016

View reviewed changes

justinsb self-assigned this Dec 7, 2016

vjm force-pushed the master branch from 110ed56 to bf090c0 Compare December 8, 2016 05:10

krisnova reviewed Dec 11, 2016

View reviewed changes

justinsb requested changes Dec 15, 2016

View reviewed changes

vjm force-pushed the master branch from bf090c0 to 2e28bab Compare December 15, 2016 21:17

chrislovecnm mentioned this pull request Dec 19, 2016

Umbrella List of Test Issues and Test Cases #1206

Closed

27 tasks

vjm force-pushed the master branch from 10dab19 to 590148a Compare December 20, 2016 17:19

vjm added a commit to vjm/kops that referenced this pull request Dec 20, 2016

Add KubeletPreferredAddressTypes kubernetes#1083

590148a

chrislovecnm suggested changes Dec 20, 2016

View reviewed changes

justinsb approved these changes Dec 20, 2016

View reviewed changes

vjm added 2 commits December 20, 2016 12:28

Add KubeletPreferredAddressTypes kubernetes#1083 Documentation

b0a535c

Add KubeletPreferredAddressTypes kubernetes#1083

0fbd19a

vjm force-pushed the master branch from 590148a to 0fbd19a Compare December 20, 2016 18:33

justinsb merged commit 9e79aca into kubernetes:master Dec 20, 2016

chrislovecnm mentioned this pull request Dec 23, 2016

Implementing kubelet GC settings in kops #1243

Closed

justinsb mentioned this pull request Dec 27, 2016

We should test that we can name instances whatever we want now #1281

Closed

		@@ -374,20 +374,21 @@ type KubeAPIServerConfig struct {

		LogLevel int `json:"logLevel,omitempty" flag:"v"`

Add KubeletPreferredAddressTypes #1083

Add KubeletPreferredAddressTypes #1083

Conversation

vjm commented Dec 7, 2016 • edited by k8s-reviewable Loading

k8s-ci-robot commented Dec 7, 2016

k8s-ci-robot commented Dec 7, 2016

k8s-ci-robot commented Dec 7, 2016

vjm commented Dec 7, 2016

justinsb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrislovecnm commented Dec 7, 2016

vjm commented Dec 7, 2016

vjm commented Dec 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb 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

chrislovecnm commented Dec 19, 2016

chrislovecnm commented Dec 19, 2016

vjm commented Dec 20, 2016

chrislovecnm commented Dec 20, 2016

vjm commented Dec 20, 2016

chrislovecnm 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb 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

justinsb commented Dec 20, 2016

vjm commented Dec 7, 2016 •

edited by k8s-reviewable

Loading